diff --git a/src/github/backfill.ts b/src/github/backfill.ts index b568e85385..34dd9d200d 100644 --- a/src/github/backfill.ts +++ b/src/github/backfill.ts @@ -2793,6 +2793,19 @@ async function reduceLiveCiAggregate( const { checkRuns, statuses, requiredContexts, checkRunsIncomplete, statusIncomplete, fetchSuites } = inputs; const enforceRequiredOnly = requiredContexts != null && requiredContexts.size > 0; const isRequired = (name: string): boolean => !enforceRequiredOnly || requiredContexts!.has(name); + // Deliberately the OPPOSITE unknown-case default from isRequired() above, and used ONLY for a third-party + // app's own action_required verdict (see isThirdPartyActionRequired below). isRequired()'s "assume required + // unless proven otherwise" is the right fail-safe for a genuine CI failure (an unconfirmed-required broken + // build should still block). But that same default silently reopened #4414 for any repo with NO + // branch-protection required-contexts configured at all (enforceRequiredOnly false, e.g. one that never set + // up required status checks): isRequired() returns true for every name in that mode, so a third-party + // advisory-only check-run (Superagent's "Contributor trust", never meant to gate anything on its own) got + // folded into failingDetails and auto-closed real contributor PRs again, on repos exactly like #4812's + // metagraphed (confirmed empty required_status_checks.contexts) -- despite #4414 believing it had already + // fixed this. A third-party app's action_required is a POLICY OPINION, not "your code is broken"; the + // failure mode of wrongly closing a real contributor's clean PR is worse than under-enforcing a check no + // maintainer ever formally required, so this one path needs POSITIVE evidence, not a fail-safe assumption. + const isConfirmedRequired = (name: string): boolean => enforceRequiredOnly && requiredContexts!.has(name); const failingDetails: LiveCiAggregate["failingDetails"] = []; const nonRequiredFailingDetails: LiveCiAggregate["nonRequiredFailingDetails"] = []; let total = 0; @@ -2824,14 +2837,18 @@ async function reduceLiveCiAggregate( // the same way conflates them (#4414 regressed exactly this -- a non-required advisory check started // auto-closing real contributor PRs). This is NOT the github-actions "awaiting maintainer Approve and run" // case the action_required exclusion above exists for: non-Actions apps use their own conclusion as a policy - // signal. Conservative: an unknown/absent app slug is NOT treated as third-party here. + // signal. Conservative: an unknown/absent app slug is NOT treated as third-party here. Gated on + // isConfirmedRequired (not isRequired, see its own doc comment above): a repo with NO branch-protection + // required-contexts configured at all must NOT fall back to treating this as required just because we + // don't know better -- that fail-safe belongs to genuine CI failures, not a third-party opinion check. const isThirdPartyActionRequired = conclusion === "action_required" && status === "completed" && appSlug !== "" && appSlug !== "github-actions"; - if (isThirdPartyActionRequired && isRequired(run.name)) { + if (isThirdPartyActionRequired && isConfirmedRequired(run.name)) { const summary = checkRunSummary(run); failingDetails.push({ name: run.name, ...(summary ? { summary } : {}), ...(run.details_url ? { detailsUrl: run.details_url } : {}) }); } else if (isThirdPartyActionRequired) { - // Non-required: visible (never silently folded into "passed" either, unlike the pre-#4414 behavior) but - // non-blocking -- routed to nonRequiredFailingDetails, which never feeds ciState or a close decision. + // Non-required (or required-contexts unconfirmed, e.g. no branch protection configured): visible (never + // silently folded into "passed" either, unlike the pre-#4414 behavior) but non-blocking -- routed to + // nonRequiredFailingDetails, which never feeds ciState or a close decision. const summary = checkRunSummary(run); nonRequiredFailingDetails.push({ name: run.name, ...(summary ? { summary } : {}), ...(run.details_url ? { detailsUrl: run.details_url } : {}) }); } else if (conclusion ? CI_FAILING_CONCLUSIONS.has(conclusion) : false) { diff --git a/test/unit/backfill.test.ts b/test/unit/backfill.test.ts index 1c3b12f2db..76c915f0de 100644 --- a/test/unit/backfill.test.ts +++ b/test/unit/backfill.test.ts @@ -4602,6 +4602,71 @@ describe("GitHub backfill", () => { ]); }); + it("REGRESSION (#4812): a third-party action_required check-run on a repo with NO branch-protection required contexts configured at all is still non-blocking, not folded into failingDetails by the 'assume required when unknown' fallback", async () => { + // Reproduces PR #4812 (JSONbored/metagraphed) exactly: the repo's real branch protection returns + // required_status_checks.contexts: [] (confirmed via the live GitHub API) -- fetchRequiredStatusContexts + // maps that to an EMPTY Set, not null, so enforceRequiredOnly is false. Before this fix, isRequired()'s + // "!enforceRequiredOnly || ..." made every name "required" in that mode, silently reopening #4414 for + // any repo that simply never configured GitHub-native required status checks -- Contributor trust + // (Superagent's advisory, never-should-block signal) got folded into failingDetails and auto-closed a + // real contributor's PR with every actual CI check (tests, coverage, ui) green. + 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: [ + { name: "test", status: "completed", conclusion: "success", app: { slug: "github-actions" } }, + { name: "ui", status: "completed", conclusion: "success", app: { slug: "github-actions" } }, + { + name: "Contributor trust", + status: "completed", + conclusion: "action_required", + app: { slug: "superagent-security" }, + output: { title: "Contributor flagged for review" }, + }, + ], + }); + } + if (url.includes("/status?")) return Response.json({ statuses: [{ context: "codecov/patch", state: "success" }] }); + 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/metagraphed", "sha4812", "public-token", new Set()); + + expect(aggregate.ciState).toBe("passed"); + expect(aggregate.failingDetails).toEqual([]); + expect(aggregate.nonRequiredFailingDetails).toEqual([{ name: "Contributor trust", summary: "Contributor flagged for review" }]); + }); + + it("REGRESSION (#4812): the same holds when required-status-context fetch outright failed (null), not just when it confirmed an empty list", async () => { + // A distinct origin from the empty-Set case above (a 403/fetch error rather than a confirmed-empty + // response), but must resolve the same way: no POSITIVE confirmation that Contributor trust is required + // means it stays advisory, never a close reason. + 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: [ + { name: "test", status: "completed", conclusion: "success", app: { slug: "github-actions" } }, + { name: "Contributor trust", status: "completed", conclusion: "action_required", app: { slug: "superagent-security" } }, + ], + }); + } + 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/metagraphed", "sha4812b", "public-token", null); + + expect(aggregate.ciState).toBe("passed"); + expect(aggregate.failingDetails).toEqual([]); + expect(aggregate.nonRequiredFailingDetails).toEqual([{ name: "Contributor trust" }]); + }); + it("a non-required third-party action_required check-run with no output/details_url still lands in nonRequiredFailingDetails, bare (name-only)", async () => { const env = createTestEnv({ GITHUB_PUBLIC_TOKEN: "public-token" }); vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {