From 17143ed94daae6d9b86c24b13e1d39329a17c624 Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Tue, 7 Jul 2026 01:09:46 -0700 Subject: [PATCH] fix(github): preserve colliding check-run failures --- src/github/backfill.ts | 83 +++++++++++++++---------- test/unit/backfill.test.ts | 38 ++++++++--- test/unit/graphql-status-rollup.test.ts | 36 +++++++---- 3 files changed, 104 insertions(+), 53 deletions(-) diff --git a/src/github/backfill.ts b/src/github/backfill.ts index ad47d6d9e8..d9a82e3268 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -2664,7 +2664,7 @@ export async function fetchNamedCheckRunConclusion( // Minimal structural shape the CI reducer needs from a check-run — a superset of the REST GitHubCheckRunPayload // (so REST payloads assign directly) AND buildable from the GraphQL CheckRun node (which has no `id`). `started_at` -// is carried specifically so the reducer can dedupe a re-run job's stale entry (see dedupeLatestCheckRunsByName) — +// is carried specifically so the reducer can dedupe a re-run job's stale entry (see dedupeLatestCheckRunsByIdentity) — // both the REST payload and the GraphQL query populate it, so it is the one recency signal available on EITHER path. type LiveCiCheckRun = { name: string; @@ -2674,45 +2674,59 @@ type LiveCiCheckRun = { started_at?: string | null; output?: { title?: unknown; summary?: unknown }; app?: { slug?: string | null } | null; + check_suite?: { id?: number | string | null; databaseId?: number | string | null } | null; }; type LiveCiStatus = { context?: string | null; state?: string | null; description?: string | null; target_url?: string | null }; type LiveCiSuite = { status?: string | null; app?: { slug?: string | null } | null }; /** - * Collapse re-run duplicates so classification only ever sees ONE entry per check-run `name`. GitHub's check-runs - * API (both `/check-runs` REST and the GraphQL `statusCheckRollup`) can return MULTIPLE entries with the same name - * when a job is re-run (e.g. "Re-run failed jobs" after a flake) — the stale run is NOT removed or replaced, it is - * left in the list alongside the new one. Without this step the classification loop below would push the stale - * run's failure into `failingDetails` even though the same-named job now currently passes, resolving `ciState` to - * "failed" on a commit whose CI is actually green (reproduced empirically against a real commit in this repo: a - * "Deploy UI preview version" check-run appeared twice, once `conclusion: "failure"` from the original run and once - * `conclusion: "skipped"` from the re-run, and the un-deduped reducer read it as failed). + * Collapse re-run duplicates so classification only ever sees ONE entry per logical check-run attempt stream. + * GitHub's check-runs API (both `/check-runs` REST and the GraphQL `statusCheckRollup`) can return MULTIPLE + * entries for the same logical check after a job is re-run (e.g. "Re-run failed jobs" after a flake) — the stale + * run is NOT removed or replaced, it is left in the list alongside the new one. Without this step the classification + * loop below would push the stale run's failure into `failingDetails` even though that exact check now currently + * passes. * - * Tiebreak by `started_at` (ISO-8601, string-comparable in chronological order) when BOTH candidates have one — - * GitHub sets `started_at` at check-run creation time, so it is a direct recency signal available on both the REST - * payload and (once queried) the GraphQL node; unlike a numeric `id`, it exists on the shared `LiveCiCheckRun` shape - * the GraphQL path can actually populate (GraphQL check-run nodes have no exposed `id`). When either candidate is - * missing `started_at` (a queued run that has not started yet has none), array order is the fallback: GitHub does - * not document a stable ordering contract for `/check-runs`, so this deliberately does not assume "returned last is - * newest" as a general rule — it only breaks a genuine tie, and last-standing is at least as good a default as - * first-standing when no timestamp is available. + * The identity key deliberately includes the check suite, not just `name`: check-run names are display labels, not a + * uniqueness boundary, and different apps/workflows/suites can legitimately publish the same name. If the suite id is + * absent, the run is left undeduped so an observed failure cannot be hidden by an unrelated later success. + * + * Tiebreak by `started_at` (ISO-8601, string-comparable in chronological order) when BOTH candidates have one. When + * either candidate is missing `started_at` (a queued run that has not started yet has none), array order is the + * fallback: GitHub does not document a stable ordering contract for `/check-runs`, so this deliberately does not + * assume "returned last is newest" as a general rule — it only breaks a genuine tie, and last-standing is at least + * as good a default as first-standing when no timestamp is available. */ -function dedupeLatestCheckRunsByName(checkRuns: ReadonlyArray): LiveCiCheckRun[] { - const latestByName = new Map(); +function checkRunDedupeKey(run: LiveCiCheckRun): string | null { + const suiteId = run.check_suite?.id ?? run.check_suite?.databaseId ?? null; + if (suiteId == null || suiteId === "") return null; + return `${run.name}\0${(run.app?.slug ?? "").toLowerCase()}\0${String(suiteId)}`; +} + +function dedupeLatestCheckRunsByIdentity(checkRuns: ReadonlyArray): LiveCiCheckRun[] { + const output: LiveCiCheckRun[] = []; + const indexByIdentity = new Map(); for (const run of checkRuns) { - const existing = latestByName.get(run.name); - if (!existing) { - latestByName.set(run.name, run); + const key = checkRunDedupeKey(run); + if (!key) { + output.push(run); + continue; + } + const existingIndex = indexByIdentity.get(key); + if (existingIndex == null) { + indexByIdentity.set(key, output.length); + output.push(run); continue; } + const existing = output[existingIndex]!; if (run.started_at && existing.started_at) { - if (run.started_at >= existing.started_at) latestByName.set(run.name, run); + if (run.started_at >= existing.started_at) output[existingIndex] = run; } else { // No comparable timestamp on one or both sides — keep the later array entry (see doc above). - latestByName.set(run.name, run); + output[existingIndex] = run; } } - return [...latestByName.values()]; + return output; } /** @@ -2722,8 +2736,8 @@ function dedupeLatestCheckRunsByName(checkRuns: ReadonlyArray): * differs (#1941), which is what keeps the flag-gated GraphQL path semantically equivalent to the proven REST one. * `fetchSuites` is invoked ONLY when the cheaper sources are fully settled (no failure, no pending, no incomplete * read), mirroring the REST path's conditional suites read so neither path pays for it on an already-decided PR; it - * returns the suite list, or null when that read is unreadable (fail-closed). Check-runs are deduped by name - * (`dedupeLatestCheckRunsByName`) before classification so a re-run job's stale duplicate can never masquerade as a + * returns the suite list, or null when that read is unreadable (fail-closed). Check-runs are deduped by check-run identity + * (`dedupeLatestCheckRunsByIdentity`) before classification so a re-run job's stale duplicate can never masquerade as a * current failure; classic commit-statuses are NOT deduped here because GitHub's Combined Status API is documented * to already return exactly one entry per unique context (the most recent), so this duplicate-name failure mode * does not apply to `statuses`. @@ -2752,10 +2766,10 @@ async function reduceLiveCiAggregate( let sawFirstPartyCheckRun = false; const seenContextNames = new Set(); - // 1) Check-runs (GitHub Actions jobs, CodeQL, app checks). Deduped by name FIRST (dedupeLatestCheckRunsByName) - // so a re-run job's stale duplicate entry can never contribute its own failingDetails/pending signal alongside - // the current one. - for (const run of dedupeLatestCheckRunsByName(checkRuns)) { + // 1) Check-runs (GitHub Actions jobs, CodeQL, app checks). Deduped by check-run identity first, so a + // re-run job's stale duplicate entry can never contribute its own failingDetails/pending signal alongside the + // current one, without collapsing unrelated checks that merely share a display name. + for (const run of dedupeLatestCheckRunsByIdentity(checkRuns)) { seenContextNames.add(run.name); // mark BEFORE bot-check skip: a bot-owned required context is "seen" if ((run.app?.slug ?? "").toLowerCase() === "github-actions") sawFirstPartyCheckRun = true; if (isOwnGitHubAppCheckRun(env, run)) continue; // never wait on the bot's own Gate/Context check-runs @@ -2952,7 +2966,7 @@ export async function fetchLiveCiAggregateViaGraphQl( if (!headSha || !token) return null; const [owner, name] = repoFullName.split("/"); if (!owner || !name) return null; - const query = `query GittensoryLiveCiRollup { repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { object(oid: ${JSON.stringify(headSha)}) { ... on Commit { statusCheckRollup { contexts(first: 100) { nodes { __typename ... on CheckRun { name conclusion status startedAt detailsUrl title summary checkSuite { app { slug } } } ... on StatusContext { context state description targetUrl } } pageInfo { hasNextPage } } } checkSuites(first: 100) { nodes { status app { slug } } } } } } }`; + const query = `query GittensoryLiveCiRollup { repository(owner: ${JSON.stringify(owner)}, name: ${JSON.stringify(name)}) { object(oid: ${JSON.stringify(headSha)}) { ... on Commit { statusCheckRollup { contexts(first: 100) { nodes { __typename ... on CheckRun { name conclusion status startedAt detailsUrl title summary checkSuite { databaseId app { slug } } } ... on StatusContext { context state description targetUrl } } pageInfo { hasNextPage } } } checkSuites(first: 100) { nodes { status app { slug } } } } } } }`; const result = await githubGraphQl<{ data?: { repository?: { @@ -2968,7 +2982,7 @@ export async function fetchLiveCiAggregateViaGraphQl( detailsUrl?: string | null; title?: string | null; summary?: string | null; - checkSuite?: { app?: { slug?: string | null } | null } | null; + checkSuite?: { databaseId?: number | null; app?: { slug?: string | null } | null } | null; context?: string | null; state?: string | null; description?: string | null; @@ -3007,7 +3021,7 @@ export async function fetchLiveCiAggregateViaGraphQl( // Field-name mapping only (detailsUrl→details_url, startedAt→started_at, title/summary→output.*, // checkSuite.app→app); the reducer lowercases GraphQL's UPPERCASE conclusion/status enums, so no case // handling is needed here. `started_at` is carried through so reduceLiveCiAggregate's re-run dedup - // (dedupeLatestCheckRunsByName) has the same recency signal on this path as it does on REST. + // (dedupeLatestCheckRunsByIdentity) has the same recency signal on this path as it does on REST. checkRuns.push({ name: node.name ?? "", conclusion: node.conclusion ?? null, @@ -3016,6 +3030,7 @@ export async function fetchLiveCiAggregateViaGraphQl( started_at: node.startedAt ?? null, output: { title: node.title ?? undefined, summary: node.summary ?? undefined }, app: { slug: node.checkSuite?.app?.slug ?? null }, + check_suite: { databaseId: node.checkSuite?.databaseId ?? null }, }); } else if (node.__typename === "StatusContext") { statuses.push({ context: node.context ?? null, state: node.state ?? null, description: node.description ?? null, target_url: node.targetUrl ?? null }); diff --git a/test/unit/backfill.test.ts b/test/unit/backfill.test.ts index d31d427d19..a5a9ce6c21 100644 --- a/test/unit/backfill.test.ts +++ b/test/unit/backfill.test.ts @@ -4848,8 +4848,8 @@ describe("GitHub backfill", () => { if (url.includes("/check-runs?")) { return Response.json({ check_runs: [ - { id: 85478132562, name: "Deploy UI preview version", status: "completed", conclusion: "failure", started_at: "2026-07-06T20:56:33Z" }, - { id: 85485221438, name: "Deploy UI preview version", status: "completed", conclusion: "skipped", started_at: "2026-07-06T21:34:29Z" }, + { id: 85478132562, name: "Deploy UI preview version", status: "completed", conclusion: "failure", started_at: "2026-07-06T20:56:33Z", check_suite: { id: 4401 } }, + { id: 85485221438, name: "Deploy UI preview version", status: "completed", conclusion: "skipped", started_at: "2026-07-06T21:34:29Z", check_suite: { id: 4401 } }, ], }); } @@ -4871,8 +4871,8 @@ describe("GitHub backfill", () => { if (url.includes("/check-runs?")) { return Response.json({ check_runs: [ - { id: 1, name: "Deploy UI preview version", status: "completed", conclusion: "success", started_at: "2026-07-06T20:56:33Z" }, - { id: 2, name: "Deploy UI preview version", status: "completed", conclusion: "failure", started_at: "2026-07-06T21:34:29Z" }, + { id: 1, name: "Deploy UI preview version", status: "completed", conclusion: "success", started_at: "2026-07-06T20:56:33Z", check_suite: { id: 4401 } }, + { id: 2, name: "Deploy UI preview version", status: "completed", conclusion: "failure", started_at: "2026-07-06T21:34:29Z", check_suite: { id: 4401 } }, ], }); } @@ -4896,8 +4896,8 @@ describe("GitHub backfill", () => { if (url.includes("/check-runs?")) { return Response.json({ check_runs: [ - { id: 2, name: "Deploy UI preview version", status: "completed", conclusion: "skipped", started_at: "2026-07-06T21:34:29Z" }, - { id: 1, name: "Deploy UI preview version", status: "completed", conclusion: "failure", started_at: "2026-07-06T20:56:33Z" }, + { id: 2, name: "Deploy UI preview version", status: "completed", conclusion: "skipped", started_at: "2026-07-06T21:34:29Z", check_suite: { id: 4401 } }, + { id: 1, name: "Deploy UI preview version", status: "completed", conclusion: "failure", started_at: "2026-07-06T20:56:33Z", check_suite: { id: 4401 } }, ], }); } @@ -4912,6 +4912,28 @@ describe("GitHub backfill", () => { expect(aggregate.failingDetails).toEqual([]); }); + it("does not discard failing same-name check-runs from a different suite", async () => { + const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = input.toString(); + if (url.includes("/check-runs?")) { + return Response.json({ + check_runs: [ + { id: 1, name: "security", status: "completed", conclusion: "failure", started_at: "2026-07-06T20:56:33Z", app: { slug: "required-security-ci" }, check_suite: { id: 9001 } }, + { id: 2, name: "security", status: "completed", conclusion: "success", started_at: "2026-07-06T21:34:29Z", app: { slug: "colliding-helper-ci" }, check_suite: { id: 9002 } }, + ], + }); + } + if (url.includes("/status?")) return Response.json({ statuses: [] }); + return new Response("not found", { status: 404 }); + }); + + const aggregate = await fetchLiveCiAggregate(env, "JSONbored/gittensory", "abc123", "public-token", new Set(["security"])); + + expect(aggregate.ciState).toBe("failed"); + expect(aggregate.failingDetails).toEqual([expect.objectContaining({ name: "security" })]); + }); + it("falls back to array order when neither duplicate has a started_at (queued runs have none)", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { @@ -4919,8 +4941,8 @@ describe("GitHub backfill", () => { if (url.includes("/check-runs?")) { return Response.json({ check_runs: [ - { id: 1, name: "flaky", status: "completed", conclusion: "failure", started_at: null }, - { id: 2, name: "flaky", status: "completed", conclusion: "success", started_at: null }, + { id: 1, name: "flaky", status: "completed", conclusion: "failure", started_at: null, check_suite: { id: 4401 } }, + { id: 2, name: "flaky", status: "completed", conclusion: "success", started_at: null, check_suite: { id: 4401 } }, ], }); } diff --git a/test/unit/graphql-status-rollup.test.ts b/test/unit/graphql-status-rollup.test.ts index a9fbf2da09..31228e5d3d 100644 --- a/test/unit/graphql-status-rollup.test.ts +++ b/test/unit/graphql-status-rollup.test.ts @@ -11,13 +11,13 @@ const REPO = "JSONbored/gittensory"; const SHA = "deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"; const TOKEN = "test-token"; -type Run = { name: string; conclusion?: string | null; status?: string | null; appSlug?: string | null; detailsUrl?: string | null; title?: string | null; summary?: string | null; startedAt?: string | null }; +type Run = { name: string; conclusion?: string | null; status?: string | null; appSlug?: string | null; detailsUrl?: string | null; title?: string | null; summary?: string | null; startedAt?: string | null; suiteId?: number | null }; type Status = { context: string; state: string; description?: string | null; targetUrl?: string | null }; type Suite = { status: string; appSlug: string }; // GraphQL statusCheckRollup nodes use UPPERCASE enums (SUCCESS/FAILURE/COMPLETED/IN_PROGRESS) — the reducer // lowercases them, so the fixtures below deliberately use GitHub's real GraphQL casing. -const runNode = (r: Run) => ({ __typename: "CheckRun", name: r.name, conclusion: r.conclusion ?? null, status: r.status ?? null, detailsUrl: r.detailsUrl ?? null, title: r.title ?? null, summary: r.summary ?? null, startedAt: r.startedAt ?? null, checkSuite: { app: { slug: r.appSlug ?? null } } }); +const runNode = (r: Run) => ({ __typename: "CheckRun", name: r.name, conclusion: r.conclusion ?? null, status: r.status ?? null, detailsUrl: r.detailsUrl ?? null, title: r.title ?? null, summary: r.summary ?? null, startedAt: r.startedAt ?? null, checkSuite: { databaseId: r.suiteId ?? null, app: { slug: r.appSlug ?? null } } }); const statusNode = (s: Status) => ({ __typename: "StatusContext", context: s.context, state: s.state, description: s.description ?? null, targetUrl: s.targetUrl ?? null }); function graphqlBody(opts: { runs?: Run[]; statuses?: Status[]; suites?: Suite[]; hasNextPage?: boolean; object?: unknown } = {}): unknown { @@ -45,7 +45,7 @@ function stubGraphql(body: unknown, opts: { status?: number } = {}): void { function stubRest(opts: { runs?: Run[]; statuses?: Status[]; suites?: Suite[] }): void { vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { const url = input.toString(); - if (url.includes("/check-runs")) return Response.json({ check_runs: (opts.runs ?? []).map((r) => ({ id: 1, name: r.name, status: (r.status ?? "").toLowerCase(), conclusion: r.conclusion ? r.conclusion.toLowerCase() : null, details_url: r.detailsUrl ?? null, started_at: r.startedAt ?? null, output: { title: r.title ?? null, summary: r.summary ?? null }, app: { slug: r.appSlug ?? null } })) }); + if (url.includes("/check-runs")) return Response.json({ check_runs: (opts.runs ?? []).map((r) => ({ id: 1, name: r.name, status: (r.status ?? "").toLowerCase(), conclusion: r.conclusion ? r.conclusion.toLowerCase() : null, details_url: r.detailsUrl ?? null, started_at: r.startedAt ?? null, output: { title: r.title ?? null, summary: r.summary ?? null }, app: { slug: r.appSlug ?? null }, check_suite: { id: r.suiteId ?? null } })) }); if (url.includes("/status")) return Response.json({ statuses: (opts.statuses ?? []).map((s) => ({ context: s.context, state: s.state.toLowerCase(), description: s.description ?? null, target_url: s.targetUrl ?? null })) }); if (url.includes("/check-suites")) return Response.json({ check_suites: (opts.suites ?? []).map((s) => ({ status: s.status.toLowerCase(), app: { slug: s.appSlug } })) }); return new Response("not found", { status: 404 }); @@ -198,8 +198,8 @@ describe("fetchLiveCiAggregateViaGraphQl — verdicts", () => { stubGraphql( graphqlBody({ runs: [ - { name: "Deploy UI preview version", conclusion: "FAILURE", status: "COMPLETED", startedAt: "2026-07-06T20:56:33Z" }, - { name: "Deploy UI preview version", conclusion: "SKIPPED", status: "COMPLETED", startedAt: "2026-07-06T21:34:29Z" }, + { name: "Deploy UI preview version", conclusion: "FAILURE", status: "COMPLETED", startedAt: "2026-07-06T20:56:33Z", suiteId: 4401 }, + { name: "Deploy UI preview version", conclusion: "SKIPPED", status: "COMPLETED", startedAt: "2026-07-06T21:34:29Z", suiteId: 4401 }, ], suites: [{ status: "COMPLETED", appSlug: "github-actions" }], }), @@ -209,12 +209,26 @@ describe("fetchLiveCiAggregateViaGraphQl — verdicts", () => { expect(agg?.failingDetails).toEqual([]); }); + it("does not dedupe colliding same-name check-runs from different suites", async () => { + stubGraphql( + graphqlBody({ + runs: [ + { name: "security", conclusion: "FAILURE", status: "COMPLETED", startedAt: "2026-07-06T20:56:33Z", suiteId: 9001, appSlug: "required-security-ci" }, + { name: "security", conclusion: "SUCCESS", status: "COMPLETED", startedAt: "2026-07-06T21:34:29Z", suiteId: 9002, appSlug: "colliding-helper-ci" }, + ], + }), + ); + const agg = await fetchLiveCiAggregateViaGraphQl(env, REPO, SHA, TOKEN, new Set(["security"])); + expect(agg?.ciState).toBe("failed"); + expect(agg?.failingDetails).toEqual([expect.objectContaining({ name: "security" })]); + }); + it("still fails when the LATEST duplicate-named check-run is the one that failed (order/recency-aware, not just duplicate-blind)", async () => { stubGraphql( graphqlBody({ runs: [ - { name: "Deploy UI preview version", conclusion: "SUCCESS", status: "COMPLETED", startedAt: "2026-07-06T20:56:33Z" }, - { name: "Deploy UI preview version", conclusion: "FAILURE", status: "COMPLETED", startedAt: "2026-07-06T21:34:29Z" }, + { name: "Deploy UI preview version", conclusion: "SUCCESS", status: "COMPLETED", startedAt: "2026-07-06T20:56:33Z", suiteId: 4401 }, + { name: "Deploy UI preview version", conclusion: "FAILURE", status: "COMPLETED", startedAt: "2026-07-06T21:34:29Z", suiteId: 4401 }, ], }), ); @@ -239,8 +253,8 @@ describe("fetchLiveCiAggregateViaGraphQl — equivalence with the REST path", () // paths must dedupe by name (keeping the newer `started_at`) and resolve "passed", not "failed" (the bug). name: "duplicate-named check-run from a re-run — newer passing entry wins", runs: [ - { name: "Deploy UI preview version", conclusion: "FAILURE", status: "COMPLETED", startedAt: "2026-07-06T20:56:33Z" }, - { name: "Deploy UI preview version", conclusion: "SKIPPED", status: "COMPLETED", startedAt: "2026-07-06T21:34:29Z" }, + { name: "Deploy UI preview version", conclusion: "FAILURE", status: "COMPLETED", startedAt: "2026-07-06T20:56:33Z", suiteId: 4401 }, + { name: "Deploy UI preview version", conclusion: "SKIPPED", status: "COMPLETED", startedAt: "2026-07-06T21:34:29Z", suiteId: 4401 }, ], required: ["Deploy UI preview version"], }, @@ -249,8 +263,8 @@ describe("fetchLiveCiAggregateViaGraphQl — equivalence with the REST path", () // the fix is recency-aware, not just "duplicates always pass"). name: "duplicate-named check-run from a re-run — newer failing entry wins", runs: [ - { name: "Deploy UI preview version", conclusion: "SUCCESS", status: "COMPLETED", startedAt: "2026-07-06T20:56:33Z" }, - { name: "Deploy UI preview version", conclusion: "FAILURE", status: "COMPLETED", startedAt: "2026-07-06T21:34:29Z" }, + { name: "Deploy UI preview version", conclusion: "SUCCESS", status: "COMPLETED", startedAt: "2026-07-06T20:56:33Z", suiteId: 4401 }, + { name: "Deploy UI preview version", conclusion: "FAILURE", status: "COMPLETED", startedAt: "2026-07-06T21:34:29Z", suiteId: 4401 }, ], required: ["Deploy UI preview version"], },