diff --git a/src/github/backfill.ts b/src/github/backfill.ts index 4eecd4ffa3..ad47d6d9e8 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -2663,18 +2663,58 @@ 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`). +// (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) — +// 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; status?: string | null; conclusion?: string | null; details_url?: string | null; + started_at?: string | null; output?: { title?: unknown; summary?: unknown }; app?: { slug?: 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). + * + * 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. + */ +function dedupeLatestCheckRunsByName(checkRuns: ReadonlyArray): LiveCiCheckRun[] { + const latestByName = new Map(); + for (const run of checkRuns) { + const existing = latestByName.get(run.name); + if (!existing) { + latestByName.set(run.name, run); + continue; + } + if (run.started_at && existing.started_at) { + if (run.started_at >= existing.started_at) latestByName.set(run.name, run); + } else { + // No comparable timestamp on one or both sides — keep the later array entry (see doc above). + latestByName.set(run.name, run); + } + } + return [...latestByName.values()]; +} + /** * Pure reduction of a head SHA's check-runs + classic statuses (+ a lazily-fetched check-suite backstop) into the * gate's LiveCiAggregate. Extracted so the REST fetch path (fetchLiveCiAggregate) and the GraphQL rollup path @@ -2682,7 +2722,11 @@ type LiveCiSuite = { status?: string | null; app?: { slug?: string | null } | nu * 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). + * 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 + * 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`. */ async function reduceLiveCiAggregate( env: Env, @@ -2708,8 +2752,10 @@ async function reduceLiveCiAggregate( let sawFirstPartyCheckRun = false; const seenContextNames = new Set(); - // 1) Check-runs (GitHub Actions jobs, CodeQL, app checks). - for (const run of checkRuns) { + // 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)) { 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 @@ -2906,7 +2952,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 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 { app { slug } } } ... on StatusContext { context state description targetUrl } } pageInfo { hasNextPage } } } checkSuites(first: 100) { nodes { status app { slug } } } } } } }`; const result = await githubGraphQl<{ data?: { repository?: { @@ -2918,6 +2964,7 @@ export async function fetchLiveCiAggregateViaGraphQl( name?: string | null; conclusion?: string | null; status?: string | null; + startedAt?: string | null; detailsUrl?: string | null; title?: string | null; summary?: string | null; @@ -2957,13 +3004,16 @@ export async function fetchLiveCiAggregateViaGraphQl( const statuses: LiveCiStatus[] = []; for (const node of contexts?.nodes ?? []) { if (node.__typename === "CheckRun") { - // Field-name mapping only (detailsUrl→details_url, title/summary→output.*, checkSuite.app→app); the reducer - // lowercases GraphQL's UPPERCASE conclusion/status enums, so no case handling is needed here. + // 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. checkRuns.push({ name: node.name ?? "", conclusion: node.conclusion ?? null, status: node.status ?? null, details_url: node.detailsUrl ?? null, + started_at: node.startedAt ?? null, output: { title: node.title ?? undefined, summary: node.summary ?? undefined }, app: { slug: node.checkSuite?.app?.slug ?? null }, }); diff --git a/test/unit/backfill.test.ts b/test/unit/backfill.test.ts index 4f5cbb68ea..d31d427d19 100644 --- a/test/unit/backfill.test.ts +++ b/test/unit/backfill.test.ts @@ -4833,6 +4833,109 @@ describe("GitHub backfill", () => { expect(aggregate.ciCompletenessWarning).toMatch(/branch-protection required checks/i); }); }); + + describe("duplicate-named check-runs from a re-run (dedupeLatestCheckRunsByName)", () => { + // Reproduces a real commit's shape: GitHub's /check-runs endpoint returned "Deploy UI preview version" TWICE + // after a "Re-run failed jobs" — id 85478132562 (conclusion: failure, started_at 2026-07-06T20:56:33Z, the + // STALE original run) and id 85485221438 (conclusion: skipped, started_at 2026-07-06T21:34:29Z, the CURRENT + // re-run). Without dedup, the stale failure alone flipped ciState to "failed" even though the check now + // passes — which fed a TERMINAL close signal into planAgentMaintenanceActions for a contributor PR whose CI + // had legitimately gone green on re-run. + it("keeps the NEWER (passing) conclusion when a re-run leaves a stale failing duplicate by name", 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: 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" }, + ], + }); + } + if (url.includes("/status?")) return Response.json({ statuses: [] }); + if (url.includes("/check-suites?")) return Response.json({ check_suites: [{ status: "completed", app: { slug: "github-actions" } }] }); + return new Response("not found", { status: 404 }); + }); + + const aggregate = await fetchLiveCiAggregate(env, "JSONbored/gittensory", "7d145f032eb3b03b5ac5868aa3cecf3e002bb6e2", "public-token", new Set(["Deploy UI preview version"])); + + expect(aggregate.ciState).toBe("passed"); + expect(aggregate.failingDetails).toEqual([]); + }); + + it("still fails when the NEWER duplicate-named check-run is the one that failed (recency-aware, not duplicate-blind)", 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: "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" }, + ], + }); + } + 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(["Deploy UI preview version"])); + + expect(aggregate.ciState).toBe("failed"); + expect(aggregate.failingDetails).toEqual([expect.objectContaining({ name: "Deploy UI preview version" })]); + }); + + it("keeps the already-latest entry when a stale duplicate is listed OUT OF ORDER (appears second but started EARLIER)", async () => { + // GitHub does not document a stable ordering contract for /check-runs, so the comparison must genuinely + // compare timestamps rather than assume "later in the array is newer" — this fixture puts the STALE + // (older, failing) run SECOND to prove the earlier-started duplicate does not override the real latest. + 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: 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" }, + ], + }); + } + if (url.includes("/status?")) return Response.json({ statuses: [] }); + if (url.includes("/check-suites?")) return Response.json({ check_suites: [{ status: "completed", app: { slug: "github-actions" } }] }); + return new Response("not found", { status: 404 }); + }); + + const aggregate = await fetchLiveCiAggregate(env, "JSONbored/gittensory", "abc123", "public-token", new Set(["Deploy UI preview version"])); + + expect(aggregate.ciState).toBe("passed"); + expect(aggregate.failingDetails).toEqual([]); + }); + + 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) => { + const url = input.toString(); + 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 }, + ], + }); + } + if (url.includes("/status?")) return Response.json({ statuses: [] }); + if (url.includes("/check-suites?")) return Response.json({ check_suites: [{ status: "completed", app: { slug: "github-actions" } }] }); + return new Response("not found", { status: 404 }); + }); + + const aggregate = await fetchLiveCiAggregate(env, "JSONbored/gittensory", "abc123", "public-token", new Set(["flaky"])); + + // No timestamp to compare on either side → the later array entry wins (the documented tiebreak fallback). + expect(aggregate.ciState).toBe("passed"); + expect(aggregate.failingDetails).toEqual([]); + }); + }); }); describe("fetchLiveReviewThreadBlockers", () => { diff --git a/test/unit/graphql-status-rollup.test.ts b/test/unit/graphql-status-rollup.test.ts index a24d7a2def..a9fbf2da09 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 }; +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 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, 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: { 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, 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 } })) }); 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 }); @@ -190,6 +190,38 @@ describe("fetchLiveCiAggregateViaGraphQl — verdicts", () => { stubGraphql(graphqlBody({ runs: [], statuses: [], suites: [] })); expect((await fetchLiveCiAggregateViaGraphQl(env, REPO, SHA, TOKEN))?.ciState).toBe("unverified"); // fold-all, nothing seen }); + + it("dedupes a re-run's stale duplicate check-run by name, keeping the newer (passing) conclusion", async () => { + // Reproduces a real commit's shape: a job re-run leaves the ORIGINAL failing entry in the list alongside the + // NEW one instead of replacing it. Without name-based dedup, the stale "failure" would resolve ciState to + // "failed" even though the check currently passes. + 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" }, + ], + suites: [{ status: "COMPLETED", appSlug: "github-actions" }], + }), + ); + const agg = await fetchLiveCiAggregateViaGraphQl(env, REPO, SHA, TOKEN, new Set(["Deploy UI preview version"])); + expect(agg?.ciState).toBe("passed"); + expect(agg?.failingDetails).toEqual([]); + }); + + 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" }, + ], + }), + ); + const agg = await fetchLiveCiAggregateViaGraphQl(env, REPO, SHA, TOKEN, new Set(["Deploy UI preview version"])); + expect(agg?.ciState).toBe("failed"); + expect(agg?.failingDetails).toEqual([expect.objectContaining({ name: "Deploy UI preview version" })]); + }); }); describe("fetchLiveCiAggregateViaGraphQl — equivalence with the REST path", () => { @@ -202,6 +234,26 @@ describe("fetchLiveCiAggregateViaGraphQl — equivalence with the REST path", () { name: "pending non-required passes", runs: [{ name: "opt", conclusion: null, status: "IN_PROGRESS" }, { name: "build", conclusion: "SUCCESS", status: "COMPLETED" }], required: ["build"] }, { name: "missing required", runs: [{ name: "build", conclusion: "SUCCESS", status: "COMPLETED" }], required: ["build", "e2e"] }, { name: "suite in progress", runs: [{ name: "build", conclusion: "SUCCESS", status: "COMPLETED" }], suites: [{ status: "IN_PROGRESS", appSlug: "github-actions" }], required: ["build"] }, + { + // A re-run job: GitHub keeps the stale failing entry alongside the new one instead of replacing it. Both + // 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" }, + ], + required: ["Deploy UI preview version"], + }, + { + // Same duplicate-name shape, but this time the LATEST run is the one that failed — must still fail (proves + // 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" }, + ], + required: ["Deploy UI preview version"], + }, ]; for (const s of scenarios) {