diff --git a/.github/scripts/contribution-gate.test.ts b/.github/scripts/contribution-gate.test.ts index a8292a3a35..7951de817a 100644 --- a/.github/scripts/contribution-gate.test.ts +++ b/.github/scripts/contribution-gate.test.ts @@ -1,20 +1,47 @@ -import { describe, expect, test } from "bun:test"; +import { afterEach, describe, expect, spyOn, test } from "bun:test"; import { evaluateAllOpenPrs, evaluateGate, + fetchAuthorPermission, GATE_LABEL, type GateIo, + isInternalAuthor, type LinkedIssue, type OpenPr, } from "./contribution-gate.ts"; const REPO = "supabase/cli"; +describe("isInternalAuthor", () => { + test.each(["OWNER", "MEMBER", "COLLABORATOR"])( + "treats internal association %s as internal without a permission lookup", + (authorAssociation) => { + expect(isInternalAuthor(authorAssociation, undefined)).toBe(true); + }, + ); + + test.each(["admin", "write"])("treats push-capable permission %s as internal", (permission) => { + // A private org member surfaces only as CONTRIBUTOR/NONE via + // author_association, but their effective repo permission gives them + // away as a maintainer. + expect(isInternalAuthor("CONTRIBUTOR", permission)).toBe(true); + expect(isInternalAuthor("NONE", permission)).toBe(true); + }); + + test.each(["read", "none", undefined])( + "treats external author with permission %s as external", + (permission) => { + expect(isInternalAuthor("CONTRIBUTOR", permission)).toBe(false); + expect(isInternalAuthor("NONE", permission)).toBe(false); + }, + ); +}); + describe("evaluateGate", () => { test("skips bot authors", () => { const result = evaluateGate({ repository: REPO, - authorAssociation: "NONE", + isInternal: false, isBot: true, linkedIssues: [], }); @@ -22,24 +49,21 @@ describe("evaluateGate", () => { expect(result.reason).toBe("bot"); }); - test.each(["OWNER", "MEMBER", "COLLABORATOR"])( - "skips internal author association %s", - (authorAssociation) => { - const result = evaluateGate({ - repository: REPO, - authorAssociation, - isBot: false, - linkedIssues: [], - }); - expect(result.pass).toBe(true); - expect(result.reason).toBe("internal"); - }, - ); + test("skips internal authors", () => { + const result = evaluateGate({ + repository: REPO, + isInternal: true, + isBot: false, + linkedIssues: [], + }); + expect(result.pass).toBe(true); + expect(result.reason).toBe("internal"); + }); test("fails when no issue is linked", () => { const result = evaluateGate({ repository: REPO, - authorAssociation: "NONE", + isInternal: false, isBot: false, linkedIssues: [], }); @@ -52,11 +76,9 @@ describe("evaluateGate", () => { test("fails when the linked issue is open but not labeled", () => { const result = evaluateGate({ repository: REPO, - authorAssociation: "CONTRIBUTOR", + isInternal: false, isBot: false, - linkedIssues: [ - { repository: REPO, number: 12, state: "OPEN", labels: ["🐛 Bug"] }, - ], + linkedIssues: [{ repository: REPO, number: 12, state: "OPEN", labels: ["🐛 Bug"] }], }); expect(result.pass).toBe(false); expect(result.reason).toBe("missing-label"); @@ -66,11 +88,9 @@ describe("evaluateGate", () => { test("fails when the only labeled issue is closed", () => { const result = evaluateGate({ repository: REPO, - authorAssociation: "NONE", + isInternal: false, isBot: false, - linkedIssues: [ - { repository: REPO, number: 7, state: "CLOSED", labels: [GATE_LABEL] }, - ], + linkedIssues: [{ repository: REPO, number: 7, state: "CLOSED", labels: [GATE_LABEL] }], }); expect(result.pass).toBe(false); expect(result.reason).toBe("issue-closed"); @@ -79,7 +99,7 @@ describe("evaluateGate", () => { test("passes when a linked issue is open and carries the gate label", () => { const result = evaluateGate({ repository: REPO, - authorAssociation: "NONE", + isInternal: false, isBot: false, linkedIssues: [ { @@ -97,7 +117,7 @@ describe("evaluateGate", () => { test("passes when any one of several linked issues qualifies", () => { const result = evaluateGate({ repository: REPO, - authorAssociation: "FIRST_TIME_CONTRIBUTOR", + isInternal: false, isBot: false, linkedIssues: [ { repository: REPO, number: 1, state: "CLOSED", labels: [GATE_LABEL] }, @@ -114,7 +134,7 @@ describe("evaluateGate", () => { // controlled by the contributor, so it must not satisfy the gate. const result = evaluateGate({ repository: REPO, - authorAssociation: "NONE", + isInternal: false, isBot: false, linkedIssues: [ { @@ -132,7 +152,7 @@ describe("evaluateGate", () => { test("matches the repository case-insensitively", () => { const result = evaluateGate({ repository: REPO, - authorAssociation: "NONE", + isInternal: false, isBot: false, linkedIssues: [ { @@ -152,61 +172,75 @@ describe("evaluateAllOpenPrs", () => { function makeIo( openPrs: OpenPr[], linkedByPr: Record, - ): { io: GateIo; closed: Array<{ number: number; message: string }> } { + permissionByLogin: Record = {}, + ): { + io: GateIo; + closed: Array<{ number: number; message: string }>; + permissionLookups: string[]; + } { const closed: Array<{ number: number; message: string }> = []; + const permissionLookups: string[] = []; const io: GateIo = { listOpenPrs: () => Promise.resolve(openPrs), - fetchLinkedIssues: (prNumber) => - Promise.resolve(linkedByPr[prNumber] ?? []), + fetchPermission: (login) => { + permissionLookups.push(login); + return Promise.resolve(permissionByLogin[login]); + }, + fetchLinkedIssues: (prNumber) => Promise.resolve(linkedByPr[prNumber] ?? []), closePr: (prNumber, message) => { closed.push({ number: prNumber, message }); return Promise.resolve(); }, }; - return { io, closed }; + return { io, closed, permissionLookups }; } test("closes only non-conforming external PRs and leaves the rest", async () => { - const { io, closed } = makeIo( + const { io, closed, permissionLookups } = makeIo( [ - { number: 1, authorAssociation: "NONE", isBot: false }, // no issue -> close - { number: 2, authorAssociation: "MEMBER", isBot: false }, // internal -> skip - { number: 3, authorAssociation: "NONE", isBot: true }, // bot -> skip - { number: 4, authorAssociation: "CONTRIBUTOR", isBot: false }, // conforming -> keep - { number: 5, authorAssociation: "NONE", isBot: false }, // missing label -> close + // no issue -> close + { number: 1, authorLogin: "ext-a", authorAssociation: "NONE", isBot: false }, + // public member -> skip (no permission lookup needed) + { number: 2, authorLogin: "maint", authorAssociation: "MEMBER", isBot: false }, + // bot -> skip + { number: 3, authorLogin: "dependabot", authorAssociation: "NONE", isBot: true }, + // conforming external -> keep + { number: 4, authorLogin: "ext-b", authorAssociation: "CONTRIBUTOR", isBot: false }, + // missing label -> close + { number: 5, authorLogin: "ext-c", authorAssociation: "NONE", isBot: false }, + // private-membership maintainer (CONTRIBUTOR + write access) -> skip + { number: 6, authorLogin: "hidden-maint", authorAssociation: "CONTRIBUTOR", isBot: false }, ], { - 4: [ - { repository: REPO, number: 40, state: "OPEN", labels: [GATE_LABEL] }, - ], - 5: [ - { repository: REPO, number: 50, state: "OPEN", labels: ["🐛 Bug"] }, - ], + 4: [{ repository: REPO, number: 40, state: "OPEN", labels: [GATE_LABEL] }], + 5: [{ repository: REPO, number: 50, state: "OPEN", labels: ["🐛 Bug"] }], }, + { "hidden-maint": "write" }, ); const entries = await evaluateAllOpenPrs(io, REPO); expect(closed.map((c) => c.number).sort((a, b) => a - b)).toEqual([1, 5]); - const byNumber = Object.fromEntries( - entries.map((entry) => [entry.number, entry.result]), - ); + const byNumber = Object.fromEntries(entries.map((entry) => [entry.number, entry.result])); expect(byNumber[1]?.reason).toBe("no-linked-issue"); expect(byNumber[2]?.pass).toBe(true); expect(byNumber[2]?.reason).toBe("internal"); expect(byNumber[3]?.reason).toBe("bot"); expect(byNumber[4]?.pass).toBe(true); expect(byNumber[5]?.reason).toBe("missing-label"); + expect(byNumber[6]?.pass).toBe(true); + expect(byNumber[6]?.reason).toBe("internal"); expect(closed.find((c) => c.number === 1)?.message).toContain(GATE_LABEL); + // Bots and public members are settled without a permission lookup. + expect(permissionLookups).not.toContain("maint"); + expect(permissionLookups).not.toContain("dependabot"); }); test("returns an entry per PR and closes none when all conform", async () => { const { io, closed } = makeIo( - [{ number: 9, authorAssociation: "NONE", isBot: false }], + [{ number: 9, authorLogin: "ext", authorAssociation: "NONE", isBot: false }], { - 9: [ - { repository: REPO, number: 90, state: "OPEN", labels: [GATE_LABEL] }, - ], + 9: [{ repository: REPO, number: 90, state: "OPEN", labels: [GATE_LABEL] }], }, ); @@ -217,3 +251,47 @@ describe("evaluateAllOpenPrs", () => { expect(entries[0]?.result.pass).toBe(true); }); }); + +describe("fetchAuthorPermission", () => { + const fetchSpy = spyOn(globalThis, "fetch"); + afterEach(() => { + fetchSpy.mockReset(); + }); + + function stubFetch(status: number, body: unknown): void { + // `typeof fetch` carries a static `preconnect`, so attach a no-op to keep + // the implementation assignable without casting. + fetchSpy.mockImplementation( + Object.assign(() => Promise.resolve(new Response(JSON.stringify(body), { status })), { + preconnect: () => {}, + }), + ); + } + + test("returns the effective permission for a collaborator", async () => { + stubFetch(200, { permission: "admin" }); + const permission = await fetchAuthorPermission("t", "supabase", "cli", "maint"); + expect(permission).toBe("admin"); + }); + + test("maps a 404 (non-collaborator fork author) to undefined", async () => { + // The endpoint 404s for users who are not collaborators — the common case + // for the external contributors the gate targets. It must map to external, + // not abort the run. + stubFetch(404, { message: "Not Found" }); + const permission = await fetchAuthorPermission("t", "supabase", "cli", "ext"); + expect(permission).toBeUndefined(); + }); + + test("throws on other API failures so the run aborts without closing PRs", async () => { + stubFetch(403, { message: "Forbidden" }); + await expect(fetchAuthorPermission("t", "supabase", "cli", "ext")).rejects.toThrow(/403/); + }); + + test("skips the network call for a blank login", async () => { + stubFetch(200, { permission: "admin" }); + const permission = await fetchAuthorPermission("t", "supabase", "cli", ""); + expect(permission).toBeUndefined(); + expect(fetchSpy).not.toHaveBeenCalled(); + }); +}); diff --git a/.github/scripts/contribution-gate.ts b/.github/scripts/contribution-gate.ts index ad599b3c81..a2b62ee08f 100644 --- a/.github/scripts/contribution-gate.ts +++ b/.github/scripts/contribution-gate.ts @@ -3,9 +3,11 @@ * OPEN pull requests opened by external contributors. * * A PR passes only when it links to an OPEN GitHub issue that carries the - * `open-for-contribution` label. Members/collaborators/owners and bots are - * exempt (they work from Linear tickets or automation). PRs that do not follow - * the process are commented on and closed. + * `open-for-contribution` label. Maintainers (recognised by their effective + * repository permission, so private org members count too — not just the + * `author_association` GitHub exposes for public members) and bots are exempt + * (they work from Linear tickets or automation). PRs that do not follow the + * process are commented on and closed. * * Two modes, both driven from `main()`: * - single-PR (default): reacts to one PR on `pull_request_target` @@ -25,8 +27,43 @@ export const GATE_LABEL = "open-for-contribution"; -/** Author associations treated as internal (exempt from the gate). */ -const INTERNAL_ASSOCIATIONS = new Set(["OWNER", "MEMBER", "COLLABORATOR"]); +/** + * Author associations treated as internal (exempt from the gate). + * + * NOTE: `author_association` only reports `MEMBER` when a user's organization + * membership is *public*. A private org member (or a team member who keeps + * their membership private) is reported as `CONTRIBUTOR`/`NONE`, so this set + * alone is not enough to identify internal maintainers — see + * `isInternalAuthor`, which also consults the author's effective repository + * permission. + */ +export const INTERNAL_ASSOCIATIONS = new Set(["OWNER", "MEMBER", "COLLABORATOR"]); + +/** + * Effective repository permissions that mark an author as internal. A user who + * can push to the repository (directly, or via a team/org grant that + * `author_association` does not surface) is a trusted maintainer, not an + * external contributor. The legacy REST `permission` field collapses the + * `maintain` role to `write`, so `admin`/`write` covers every push-capable + * role. + */ +const WRITE_PERMISSIONS = new Set(["admin", "write"]); + +/** + * Decide whether a PR author is internal (exempt from the gate). Combines the + * cheap `author_association` signal with the author's effective repository + * `permission` (undefined when it could not be resolved), so private org + * members whose association is merely `CONTRIBUTOR` are still recognised. + */ +export function isInternalAuthor( + authorAssociation: string, + permission: string | undefined, +): boolean { + return ( + INTERNAL_ASSOCIATIONS.has(authorAssociation) || + (permission !== undefined && WRITE_PERMISSIONS.has(permission)) + ); +} export interface LinkedIssue { /** `owner/name` of the repo the issue lives in (from GraphQL nameWithOwner). */ @@ -39,7 +76,8 @@ export interface LinkedIssue { export interface GateInput { /** `owner/name` of this repository (from GITHUB_REPOSITORY). */ repository: string; - authorAssociation: string; + /** Whether the author is a trusted maintainer (see `isInternalAuthor`). */ + isInternal: boolean; isBot: boolean; linkedIssues: LinkedIssue[]; } @@ -102,7 +140,7 @@ export function evaluateGate(input: GateInput): GateResult { if (input.isBot) { return { pass: true, reason: "bot" }; } - if (INTERNAL_ASSOCIATIONS.has(input.authorAssociation)) { + if (input.isInternal) { return { pass: true, reason: "internal" }; } @@ -110,9 +148,7 @@ export function evaluateGate(input: GateInput): GateResult { // (e.g. `Closes attacker/repo#1`) links an issue the contributor controls, // so it must never satisfy the gate. const repo = input.repository.toLowerCase(); - const repoIssues = input.linkedIssues.filter( - (issue) => issue.repository.toLowerCase() === repo, - ); + const repoIssues = input.linkedIssues.filter((issue) => issue.repository.toLowerCase() === repo); if (repoIssues.length === 0) { return { @@ -137,6 +173,7 @@ export function evaluateGate(input: GateInput): GateResult { /** Minimal open-PR shape the gate needs to decide exemption. */ export interface OpenPr { number: number; + authorLogin: string; authorAssociation: string; isBot: boolean; } @@ -145,6 +182,12 @@ export interface OpenPr { export interface GateIo { /** List every open pull request in the repository. */ listOpenPrs: () => Promise; + /** + * Resolve an author's effective repository permission (`admin`/`write`/ + * `read`/`none`), or `undefined` when it could not be determined. Used to + * recognise maintainers whose org membership is private. + */ + fetchPermission: (login: string) => Promise; /** Fetch the issues a PR closes (via `closingIssuesReferences`). */ fetchLinkedIssues: (prNumber: number) => Promise; /** Comment with `message` then close the PR. Called only for failing PRs. */ @@ -161,17 +204,22 @@ export interface SweepEntry { * orchestration over the injected `io`; returns each PR's decision so the * caller can log a summary. */ -export async function evaluateAllOpenPrs( - io: GateIo, - repository: string, -): Promise { +export async function evaluateAllOpenPrs(io: GateIo, repository: string): Promise { const openPrs = await io.listOpenPrs(); const entries: SweepEntry[] = []; for (const pr of openPrs) { + // Only pay for a permission lookup when the cheap signals are inconclusive: + // bots are exempt outright, and a public internal association already + // proves membership. + let isInternal = pr.isBot || INTERNAL_ASSOCIATIONS.has(pr.authorAssociation); + if (!isInternal) { + const permission = await io.fetchPermission(pr.authorLogin); + isInternal = isInternalAuthor(pr.authorAssociation, permission); + } const linkedIssues = await io.fetchLinkedIssues(pr.number); const result = evaluateGate({ repository, - authorAssociation: pr.authorAssociation, + isInternal, isBot: pr.isBot, linkedIssues, }); @@ -204,6 +252,8 @@ async function githubFetch( url: string, token: string, init: Omit = {}, + /** Non-OK statuses to return to the caller instead of throwing on. */ + allowStatuses: readonly number[] = [], ): Promise { const response = await fetch(url, { ...init, @@ -214,11 +264,9 @@ async function githubFetch( "Content-Type": "application/json", }, }); - if (!response.ok) { + if (!response.ok && !allowStatuses.includes(response.status)) { const body = await response.text(); - throw new Error( - `GitHub request failed (${response.status}) for ${url}: ${body}`, - ); + throw new Error(`GitHub request failed (${response.status}) for ${url}: ${body}`); } return response; } @@ -262,12 +310,9 @@ async function fetchLinkedIssues( }; }; if (payload.errors?.length) { - throw new Error( - `GraphQL errors: ${payload.errors.map((e) => e.message).join("; ")}`, - ); + throw new Error(`GraphQL errors: ${payload.errors.map((e) => e.message).join("; ")}`); } - const nodes = - payload.data?.repository?.pullRequest?.closingIssuesReferences?.nodes ?? []; + const nodes = payload.data?.repository?.pullRequest?.closingIssuesReferences?.nodes ?? []; return nodes.map((node) => ({ repository: node.repository.nameWithOwner, number: node.number, @@ -279,7 +324,7 @@ async function fetchLinkedIssues( interface RestPullRequest { number: number; author_association: string; - user: { type: string } | null; + user: { login: string; type: string } | null; } async function fetchOpenPullRequests( @@ -297,6 +342,7 @@ async function fetchOpenPullRequests( for (const pr of batch) { prs.push({ number: pr.number, + authorLogin: pr.user?.login ?? "", authorAssociation: pr.author_association, isBot: pr.user?.type === "Bot", }); @@ -308,6 +354,38 @@ async function fetchOpenPullRequests( return prs; } +/** + * Resolve an author's effective permission on the repository. Reflects access + * granted directly or through a team/org membership, so it recognises private + * org members that `author_association` reports only as `CONTRIBUTOR`. This + * endpoint needs just `Metadata: read` (covered by the workflow's + * `contents: read`). + * + * A 404 means the author is not a collaborator at all — the common case for + * external fork contributors, who are exactly who the gate targets — so it maps + * to `undefined` (external) rather than throwing. Other failures still throw so + * a transient API error aborts the run without wrongly closing PRs. + */ +export async function fetchAuthorPermission( + token: string, + owner: string, + repo: string, + login: string, +): Promise { + if (!login) { + return undefined; + } + const url = + `https://api.github.com/repos/${owner}/${repo}/collaborators/` + + `${encodeURIComponent(login)}/permission`; + const response = await githubFetch(url, token, {}, [404]); + if (response.status === 404) { + return undefined; + } + const payload = (await response.json()) as { permission?: string }; + return payload.permission; +} + /** * Build the "comment then close" action for a repo. In dry-run mode it logs the * intended action instead of mutating the PR. @@ -342,19 +420,31 @@ async function runSinglePr( ): Promise { const prNumber = Number(requireEnv("PR_NUMBER")); const authorAssociation = process.env.PR_AUTHOR_ASSOCIATION ?? "NONE"; + const authorLogin = process.env.PR_AUTHOR_LOGIN ?? ""; const isBot = (process.env.PR_AUTHOR_TYPE ?? "User") === "Bot"; + // Resolve maintainer status from the effective repository permission unless a + // cheaper signal already settles it. `author_association` only exposes public + // org membership, so a private member must be confirmed via permission. + let permission: string | undefined; + let isInternal = isBot || INTERNAL_ASSOCIATIONS.has(authorAssociation); + if (!isInternal) { + permission = await fetchAuthorPermission(token, owner, repo, authorLogin); + isInternal = isInternalAuthor(authorAssociation, permission); + } + const linkedIssues = await fetchLinkedIssues(token, owner, repo, prNumber); const result = evaluateGate({ repository, - authorAssociation, + isInternal, isBot, linkedIssues, }); console.log( `Contribution gate for PR #${prNumber}: pass=${result.pass} reason=${result.reason} ` + - `(author_association=${authorAssociation}, bot=${isBot}, linked_issues=${linkedIssues.length})`, + `(author_association=${authorAssociation}, permission=${permission ?? "n/a"}, ` + + `bot=${isBot}, linked_issues=${linkedIssues.length})`, ); if (result.pass || !result.message) { @@ -377,8 +467,8 @@ async function runSweep( const base = `https://api.github.com/repos/${owner}/${repo}`; const io: GateIo = { listOpenPrs: () => fetchOpenPullRequests(token, owner, repo), - fetchLinkedIssues: (prNumber) => - fetchLinkedIssues(token, owner, repo, prNumber), + fetchPermission: (login) => fetchAuthorPermission(token, owner, repo, login), + fetchLinkedIssues: (prNumber) => fetchLinkedIssues(token, owner, repo, prNumber), closePr: makeCloser(token, base, dryRun), }; @@ -389,9 +479,7 @@ async function runSweep( `${failing.length} ${dryRun ? "would be " : ""}closed.`, ); for (const entry of entries) { - console.log( - ` PR #${entry.number}: pass=${entry.result.pass} reason=${entry.result.reason}`, - ); + console.log(` PR #${entry.number}: pass=${entry.result.pass} reason=${entry.result.reason}`); } } @@ -400,8 +488,7 @@ async function main(): Promise { const repository = requireEnv("GITHUB_REPOSITORY"); const [owner, repo] = repository.split("/"); - const allMode = - process.env.GATE_MODE === "all" || process.argv.includes("--all"); + const allMode = process.env.GATE_MODE === "all" || process.argv.includes("--all"); if (allMode) { const dryRun = /^(1|true)$/i.test(process.env.DRY_RUN ?? ""); await runSweep(token, owner!, repo!, repository, dryRun); diff --git a/.github/workflows/contribution-gate.yml b/.github/workflows/contribution-gate.yml index 54582d7790..6dfeee2bf0 100644 --- a/.github/workflows/contribution-gate.yml +++ b/.github/workflows/contribution-gate.yml @@ -51,6 +51,7 @@ jobs: GITHUB_REPOSITORY: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} PR_AUTHOR_ASSOCIATION: ${{ github.event.pull_request.author_association }} + PR_AUTHOR_LOGIN: ${{ github.event.pull_request.user.login }} PR_AUTHOR_TYPE: ${{ github.event.pull_request.user.type }} run: bun .github/scripts/contribution-gate.ts