diff --git a/docs/pr_policy.md b/docs/pr_policy.md index 20c5426..6de7c67 100644 --- a/docs/pr_policy.md +++ b/docs/pr_policy.md @@ -26,7 +26,7 @@ To keep review bandwidth focused on high-leverage improvements and prevent dupli Our automated workflow checks every pull request from external contributors when a PR is `opened`, `edited`, `reopened`, or marked `ready_for_review`: -- **Who is checked**: All external contributors (`FIRST_TIME_CONTRIBUTOR`, `CONTRIBUTOR`, `NONE`). Repository owners (`OWNER`), organization members (`MEMBER`), collaborators (`COLLABORATOR`), and bots (`Bot` / `[bot]`) are exempt. +- **Who is checked**: All external contributors (`FIRST_TIME_CONTRIBUTOR`, `CONTRIBUTOR`, `NONE`). Repository owners (`OWNER`), organization members (`MEMBER`, whether public or private), collaborators (`COLLABORATOR`, whether granted directly or through a team), and bots (`Bot` / `[bot]`) are exempt. - **How issues are matched**: - The workflow strips HTML comments (``) from the PR description and scans for issue references belonging to the same repository (`#xxx`, `owner/repo#xxx`, or `https://github.com/owner/repo/issues/xxx`). - Pull request numbers are ignored — the linked number must be an **issue** in the repository, and your GitHub username must be in the issue's **Assignees** list. diff --git a/scripts/pr-approved-issue.js b/scripts/pr-approved-issue.js index 92cd625..e6fa4bc 100644 --- a/scripts/pr-approved-issue.js +++ b/scripts/pr-approved-issue.js @@ -26,7 +26,10 @@ * - When the check fails: converts non-draft PRs to draft, posts a comment (on * `opened` or when converting to draft), and marks the check as failed. * - * Maintainers, collaborators and bots are skipped by the workflow `if:`. + * Maintainers, collaborators and bots are skipped by the workflow `if:`. All + * repository collaborators (including private org members and team members) are + * also skipped here, because the event payload's `author_association` only + * reports `MEMBER` when the org membership is public. */ const POLICY_DOC_URL = @@ -66,6 +69,14 @@ module.exports = async function prApprovedIssue({ github, context, core }) { return; } + // `author_association` only reports MEMBER when the org membership is + // public. Private org members whose access comes through the org or a team + // show up as CONTRIBUTOR, so check collaborator membership directly. + if (await isCollaborator(github, core, owner, repo, author)) { + core.info(`Skipping #${pr.number}: ${author} is a collaborator on ${owner}/${repo}.`); + return; + } + const body = pr.body || ""; // 1. Find issues referenced in the description that are assigned to the author. @@ -148,6 +159,26 @@ function findIssueNumbers(body, owner, repo) { return [...numbers]; } +/** + * True when `username` is a collaborator on the repo (204), including via + * organization or team membership at any role (admin, maintain, write, triage, + * or org-default read). External contributors return 404. + */ +async function isCollaborator(github, core, owner, repo, username) { + try { + await github.rest.repos.checkCollaborator({ owner, repo, username }); + return true; + } catch (err) { + const message = `Could not check collaborator status for ${username}: ${err.message}`; + if (err.status === 404) { + core.info(message); + } else { + core.warning(message); + } + return false; + } +} + async function convertToDraft(github, core, pr) { try { await github.graphql( diff --git a/scripts/pr-approved-issue.test.js b/scripts/pr-approved-issue.test.js index 7a5c7f2..cf0d100 100644 --- a/scripts/pr-approved-issue.test.js +++ b/scripts/pr-approved-issue.test.js @@ -28,12 +28,15 @@ describe('prApprovedIssue', () => { pr = null, payloadPr = undefined, issuesByNumber = {}, + collaborators = new Set(), + collaboratorError = null, graphqlError = null, }) { const infoMessages = []; const warningMessages = []; let failedMessage = null; const fetchedIssues = []; + const collaboratorLookups = []; const graphqlCalls = []; const createdComments = []; @@ -50,6 +53,17 @@ describe('prApprovedIssue', () => { pulls: { get: async () => ({ data: pr }), }, + repos: { + checkCollaborator: async ({ username }) => { + collaboratorLookups.push(username); + if (collaboratorError) { + throw collaboratorError; + } + if (!collaborators.has(username)) { + throw Object.assign(new Error('Not Found'), { status: 404 }); + } + }, + }, issues: { get: async ({ issue_number }) => { fetchedIssues.push(issue_number); @@ -86,6 +100,7 @@ describe('prApprovedIssue', () => { getWarningMessages: () => warningMessages, getFailedMessage: () => failedMessage, getFetchedIssues: () => fetchedIssues, + getCollaboratorLookups: () => collaboratorLookups, getGraphqlCalls: () => graphqlCalls, getCreatedComments: () => createdComments, }; @@ -120,9 +135,93 @@ describe('prApprovedIssue', () => { assert.strictEqual(harness.getFailedMessage(), null); assert.strictEqual(harness.getGraphqlCalls().length, 0); assert.strictEqual(harness.getCreatedComments().length, 0); + // The association fast path returns before the collaborator lookup. + assert.deepStrictEqual(harness.getCollaboratorLookups(), []); } }); + it('skips repository collaborators (including private org and team members) even when author_association is not bypassed', async () => { + // A private org member whose access comes through the org or a team is + // reported as CONTRIBUTOR (or NONE) in the event payload, not MEMBER, + // regardless of whether their role is admin, maintain, write, triage, or + // org-default read. `checkCollaborator` returns 204 for all of them. + for (const association of ['CONTRIBUTOR', 'FIRST_TIME_CONTRIBUTOR', 'NONE']) { + const harness = createHarness({ + action: 'opened', + pr: { + number: 20, + node_id: 'PR_20', + draft: false, + author_association: association, + user: { login: 'private-member', type: 'User' }, + body: 'No issue link needed', + }, + collaborators: new Set(['private-member']), + }); + await harness.run(); + assert.strictEqual(harness.getFailedMessage(), null); + assert.strictEqual(harness.getGraphqlCalls().length, 0); + assert.strictEqual(harness.getCreatedComments().length, 0); + assert.deepStrictEqual(harness.getCollaboratorLookups(), ['private-member']); + assert.ok( + harness + .getInfoMessages() + .includes('Skipping #20: private-member is a collaborator on keras-team/keras-hub.') + ); + } + }); + + it('still runs the check for non-collaborators (404) without emitting a warning', async () => { + const harness = createHarness({ + action: 'opened', + pr: { + number: 30, + node_id: 'PR_30', + draft: false, + author_association: 'CONTRIBUTOR', + user: { login: 'external-dev', type: 'User' }, + body: 'Here is my new feature.', + }, + }); + await harness.run(); + assert.deepStrictEqual(harness.getCollaboratorLookups(), ['external-dev']); + // A 404 is the expected non-collaborator path and must not warn. + assert.strictEqual(harness.getWarningMessages().length, 0); + assert.strictEqual(harness.getGraphqlCalls().length, 1); + assert.strictEqual(harness.getCreatedComments().length, 1); + assert.strictEqual( + harness.getFailedMessage(), + 'No approved issue assigned to external-dev is linked in #30.' + ); + }); + + it('warns but still runs the check when the collaborator lookup fails for a non-404 reason', async () => { + const harness = createHarness({ + action: 'opened', + pr: { + number: 31, + node_id: 'PR_31', + draft: false, + author_association: 'CONTRIBUTOR', + user: { login: 'external-dev', type: 'User' }, + body: 'Here is my new feature.', + }, + collaboratorError: Object.assign(new Error('Forbidden'), { status: 403 }), + }); + + await harness.run(); + + assert.strictEqual(harness.getWarningMessages().length, 1); + assert.ok( + harness + .getWarningMessages()[0] + .includes('Could not check collaborator status for external-dev: Forbidden') + ); + assert.strictEqual(harness.getGraphqlCalls().length, 1); + assert.strictEqual(harness.getCreatedComments().length, 1); + assert.notStrictEqual(harness.getFailedMessage(), null); + }); + it('fetches live PR state via pulls.get instead of relying on frozen event payload when re-run', async () => { const harness = createHarness({ action: 'opened',