Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions src/github/backfill.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down
65 changes: 65 additions & 0 deletions test/unit/backfill.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down