diff --git a/.github/tests/auto-reviewers.test.cjs b/.github/tests/auto-reviewers.test.cjs new file mode 100644 index 00000000..e36e406f --- /dev/null +++ b/.github/tests/auto-reviewers.test.cjs @@ -0,0 +1,215 @@ +const assert = require('node:assert/strict'); +const { readFileSync } = require('node:fs'); +const { resolve } = require('node:path'); +const { test } = require('node:test'); + +const workflow = readFileSync(resolve(__dirname, '..', 'workflows', 'auto-reviewers.yml'), 'utf8'); +const blocks = [...workflow.matchAll(/ script: \|\r?\n((?: [^\r\n]*(?:\r?\n|$)|\r?\n)+)/g)]; +assert.equal(blocks.length, 2, 'Expected the team-reading and reviewer-requesting scripts.'); +const AsyncFunction = Object.getPrototypeOf(async function () {}).constructor; +const scripts = blocks.map(match => new AsyncFunction( + 'github', 'context', 'core', 'process', + match[1].replace(/^ /gm, '') +)); +const context = { + repo: { owner: 'mini-software', repo: 'MiniPdf' }, + payload: { pull_request: { number: 1 } } +}; + +function harness(options = {}) { + const logs = []; + const core = { + info: message => logs.push(message), + notice: message => logs.push(message) + }; + const requested = (options.requested ?? []).map(login => ({ login })); + const reviews = options.reviews ?? (options.reviewed ?? []).map(login => ({ user: { login } })); + const batches = []; + const pull = { + state: 'open', + draft: false, + base: { ref: 'main' }, + user: { login: 'author' }, + ...options.pull + }; + const listReviews = async () => {}; + const listRequestedReviewers = async () => ({ data: { users: requested.slice(0, 30) } }); + const requestedPages = []; + const github = { + rest: { + pulls: { + get: async params => { + assert.ok(Number.isSafeInteger(params.pull_number)); + return { data: pull }; + }, + listRequestedReviewers, + listReviews, + requestReviewers: async ({ reviewers }) => { + if (options.error) { + throw options.error; + } + batches.push(reviewers); + if (options.omit) { + return; + } + if (options.reviewImmediately) { + reviews.push(...reviewers.map(login => ({ user: { login } }))); + } else { + requested.push(...reviewers.map(login => ({ login }))); + } + } + } + }, + paginate: async (method, params, map) => { + assert.equal(params.per_page, 100); + if (method === listRequestedReviewers) { + assert.equal(typeof map, 'function'); + const result = []; + for (let offset = 0; offset < requested.length || offset === 0; offset += params.per_page) { + const page = requested.slice(offset, offset + params.per_page); + requestedPages.push(page.length); + result.push(...map({ data: { users: page, teams: [] } })); + } + return result; + } + assert.equal(method, listReviews); + return [...reviews]; + } + }; + const run = (members, customContext = context, input = '') => scripts[1]( + github, customContext, core, + { env: { TEAM_REVIEWERS: JSON.stringify(members), INPUT_PR_NUMBER: input } } + ); + return { run, logs, batches, requestedPages }; +} + +test('team reader requires an explicit organization credential', async () => { + await assert.rejects(scripts[0]({}, context, {}, { env: {} }), /CORE_REVIEWERS_TOKEN/); +}); + +test('team reader dynamically paginates active members and excludes bots', async () => { + const listMembersInOrg = async () => {}; + const github = { + rest: { teams: { listMembersInOrg } }, + paginate: async (method, params) => { + assert.equal(method, listMembersInOrg); + assert.deepEqual(params, { + org: 'mini-software', team_slug: 'core-reviewers', per_page: 100 + }); + return [{ login: 'new-member', type: 'User' }, { login: 'bot', type: 'Bot' }]; + } + }; + const result = await scripts[0](github, context, { info() {} }, { + env: { CORE_REVIEWERS_TOKEN: 'test-only' } + }); + assert.deepEqual(JSON.parse(result), ['new-member']); + github.paginate = async () => []; + await assert.rejects(scripts[0](github, context, {}, { + env: { CORE_REVIEWERS_TOKEN: 'test-only' } + }), /no active human members/); +}); + +test('excludes the author, requested users, reviewed users and duplicate names case-insensitively', async () => { + const h = harness({ requested: ['requested'], reviewed: ['reviewed'] }); + await h.run(['AUTHOR', 'REQUESTED', 'REVIEWED', 'new-member', 'NEW-MEMBER']); + assert.deepEqual(h.batches, [['new-member']]); +}); + +test('requests all twenty eligible members in batches of at most fifteen', async () => { + const members = Array.from({ length: 20 }, (_, index) => `member-${index}`); + const h = harness(); + await h.run(members); + assert.deepEqual(h.batches.map(batch => batch.length), [15, 5]); + assert.deepEqual(h.batches.flat(), members); +}); + +test('initial and confirmation reads include every requested-reviewer page', async () => { + const requested = Array.from({ length: 130 }, (_, index) => `requested-${index}`); + const h = harness({ requested }); + await h.run([...requested, 'new-member']); + assert.deepEqual(h.batches, [['new-member']]); + assert.deepEqual(h.requestedPages, [100, 30, 100, 31]); +}); + +test('deleted review users do not break exclusion or confirmation', async () => { + const h = harness({ reviews: [{ user: null }, { user: { login: 'reviewed' } }] }); + await h.run(['reviewed', 'new-member']); + assert.deepEqual(h.batches, [['new-member']]); + assert.match(h.logs[0], /Requested 1/); +}); + +test('repeated events do not send duplicate review requests', async () => { + const h = harness(); + await h.run(['first', 'second']); + await h.run(['first', 'second', 'new-member']); + assert.deepEqual(h.batches, [['first', 'second'], ['new-member']]); +}); + +test('reports when no additional reviewer is eligible', async () => { + const h = harness(); + await h.run(['author']); + assert.deepEqual(h.batches, []); + assert.match(h.logs[0], /already been requested or reviewed/); +}); + +test('skips closed, draft and non-main PRs explicitly', async () => { + for (const pull of [{ state: 'closed' }, { draft: true }, { base: { ref: 'other' } }]) { + const h = harness({ pull }); + await h.run(['reviewer']); + assert.deepEqual(h.batches, []); + assert.match(h.logs[0], /Skipping/); + } +}); + +test('manual dispatch accepts a PR number and rejects invalid inputs', async () => { + const manualContext = { ...context, payload: {} }; + const h = harness(); + await h.run(['reviewer'], manualContext, '1'); + assert.deepEqual(h.batches, [['reviewer']]); + for (const input of ['', '-1', '0', '1.5', 'text', '9007199254740992']) { + await assert.rejects(h.run(['reviewer'], manualContext, input), /positive integer/); + } +}); + +test('API permission failures propagate instead of reporting success', async () => { + const error = new Error('Resource not accessible by integration'); + const h = harness({ error }); + await assert.rejects(h.run(['reviewer']), error); + assert.deepEqual(h.logs, []); +}); + +test('silently omitted API requests fail verification', async () => { + const h = harness({ omit: true }); + await assert.rejects(h.run(['reviewer']), /did not confirm review requests for: reviewer/); + assert.deepEqual(h.logs, []); +}); + +test('review completed during the request is counted as confirmed', async () => { + const h = harness({ reviewImmediately: true }); + await h.run(['reviewer']); + assert.match(h.logs[0], /Requested 1/); +}); + +test('malformed team output fails instead of treating it as an empty list', async () => { + await assert.rejects(scripts[1]( + { + rest: { pulls: { + get: async () => ({ data: { + state: 'open', draft: false, base: { ref: 'main' }, user: { login: 'author' } + } }), + listRequestedReviewers: async () => ({ data: { users: [] } }), + listReviews: async () => {} + } }, + paginate: async () => [] + }, + context, {}, { env: { TEAM_REVIEWERS: 'not-json' } } + ), SyntaxError); +}); + +test('fork-capable workflow never checks out or executes PR code', () => { + assert.match(workflow, /pull_request_target:/); + assert.match(workflow, /branches: \[main\]/); + assert.match(workflow, /pull-requests: write/); + assert.doesNotMatch(workflow, /actions\/checkout|head\.sha|head\.ref|pull_request\.title|pull_request\.body/); + assert.match(workflow, /github-token: \$\{\{ secrets\.CORE_REVIEWERS_TOKEN \}\}/); +}); diff --git a/.github/tests/review-approval-policy.test.cjs b/.github/tests/review-approval-policy.test.cjs new file mode 100644 index 00000000..71214908 --- /dev/null +++ b/.github/tests/review-approval-policy.test.cjs @@ -0,0 +1,329 @@ +const assert = require('node:assert/strict'); +const { existsSync, readFileSync } = require('node:fs'); +const { resolve } = require('node:path'); +const { test } = require('node:test'); + +const workflow = readFileSync(resolve(__dirname, '..', 'workflows', 'review-approval-policy.yml'), 'utf8'); +const block = workflow.match(/ script: \|\r?\n((?: [^\r\n]*(?:\r?\n|$)|\r?\n)+)/); +assert.ok(block, 'Expected an embedded approval policy script.'); +const AsyncFunction = Object.getPrototypeOf(async function () {}).constructor; +const evaluate = new AsyncFunction('github', 'context', 'core', block[1].replace(/^ /gm, '')); + +function pull(number = 1, labels = [], sha = `sha-${number}`) { + return { + number, draft: false, head: { sha }, + user: { login: 'author' }, labels: labels.map(name => ({ name })) + }; +} + +function review(id, login, state = 'APPROVED', type = 'User') { + return { id, state, user: { login, type } }; +} + +function harness(pulls, reviews = {}, options = {}) { + const runs = [...(options.existingChecks ?? [])]; + const writes = []; + const creates = []; + const logs = []; + const errors = []; + const failed = []; + const permissionCalls = []; + const list = async () => {}; + const listReviews = async () => {}; + const listForRef = async () => {}; + let nextId = Math.max(0, ...runs.map(run => run.id)) + 1; + const github = { + rest: { + pulls: { list, listReviews }, + checks: { + listForRef, + create: async params => { + if (options.createError) { + throw options.createError; + } + const run = { ...params, id: nextId++, app: { slug: 'github-actions' } }; + runs.push(run); + creates.push(params); + writes.push({ ...run }); + return { data: run }; + }, + update: async params => { + if (options.startError && params.status === 'in_progress') { + throw options.startError; + } + if (options.failureUpdateError && params.conclusion === 'failure') { + throw options.failureUpdateError; + } + const run = runs.find(run => run.id === params.check_run_id); + assert.ok(run, 'Only an existing check run may be updated.'); + Object.assign(run, params); + writes.push({ ...run }); + return { data: run }; + } + }, + repos: { + getCollaboratorPermissionLevel: async ({ username }) => { + permissionCalls.push(username); + if (options.permissionError) { + throw options.permissionError; + } + return { data: { permission: options.permissions?.[username] ?? 'write' } }; + } + } + }, + paginate: async (method, params) => { + assert.equal(params.per_page, 100); + if (method === list) { + assert.equal(params.base, 'main'); + assert.equal(params.state, 'open'); + return pulls; + } + if (method === listForRef) { + assert.equal(params.check_name, 'Review approval policy'); + assert.equal(params.filter, 'all'); + return runs.filter(run => run.head_sha === params.ref); + } + assert.equal(method, listReviews); + const error = options.reviewErrors?.[params.pull_number] ?? options.reviewError; + if (error) { + throw error; + } + return [...(reviews[params.pull_number] ?? [])]; + } + }; + const context = { + repo: { owner: 'mini-software', repo: 'MiniPdf' }, + serverUrl: 'https://github.com', runId: 123, + payload: {} + }; + const run = () => evaluate(github, context, { + info: message => logs.push(message), + error: message => errors.push(message), + setFailed: message => failed.push(message) + }); + return { run, writes, creates, runs, logs, errors, failed, permissionCalls }; +} + +test('ordinary advisory threshold is exactly one eligible approval', async () => { + for (const [reviews, expected] of [[[], 'action_required'], [[review(1, 'reviewer')], 'success']]) { + const h = harness([pull()], { 1: reviews }); + await h.run(); + assert.equal(h.writes[0].status, 'in_progress'); + assert.equal(h.writes.at(-1).status, 'completed'); + assert.equal(h.writes.at(-1).conclusion, expected); + assert.equal(h.writes.at(-1).name, 'Review approval policy'); + assert.match(h.writes.at(-1).output.summary, /Advisory only/); + } +}); + +test('major-change advisory check fails at two and passes at three approvals', async () => { + for (const count of [0, 1, 2, 3]) { + const h = harness([pull(1, ['MAJOR-CHANGE'])], { + 1: Array.from({ length: count }, (_, index) => review(index + 1, `reviewer-${index}`)) + }); + await h.run(); + assert.equal(h.writes.at(-1).conclusion, count === 3 ? 'success' : 'action_required'); + if (count < 3) { + assert.match(h.writes.at(-1).output.summary, new RegExp(`${count}/3`)); + } + } +}); + +test('invited users, comments, author approvals, bots and Read users do not count', async () => { + const h = harness([pull(1, ['major-change'])], { + 1: [ + review(1, 'AUTHOR'), review(2, 'bot', 'APPROVED', 'Bot'), + review(3, 'read-user'), review(4, 'commenter', 'COMMENTED'), + review(5, 'valid') + ] + }, { permissions: { 'read-user': 'read' } }); + await h.run(); + assert.match(h.writes.at(-1).output.summary, /1\/3/); + assert.deepEqual(h.permissionCalls, ['read-user', 'valid']); +}); + +test('counts Write, Maintain and Admin approvals', async () => { + const h = harness([pull(1, ['major-change'])], { + 1: [review(1, 'writer'), review(2, 'maintainer'), review(3, 'admin')] + }, { permissions: { writer: 'write', maintainer: 'maintain', admin: 'admin' } }); + await h.run(); + assert.equal(h.writes.at(-1).conclusion, 'success'); +}); + +test('later dismissed or changes-requested reviews supersede earlier approvals', async () => { + for (const state of ['DISMISSED', 'CHANGES_REQUESTED']) { + const h = harness([pull()], { + 1: [review(2, 'REVIEWER', state), review(1, 'reviewer')] + }); + await h.run(); + assert.match(h.writes.at(-1).output.summary, /0\/1/); + } +}); + +test('comments do not cancel approvals and duplicate approvals count only once', async () => { + const h = harness([pull(1, ['major-change'])], { + 1: [review(1, 'reviewer'), review(2, 'REVIEWER'), review(3, 'Reviewer', 'COMMENTED')] + }); + await h.run(); + assert.match(h.writes.at(-1).output.summary, /1\/3/); +}); + +test('a new approval after a change request is counted', async () => { + const h = harness([pull()], { + 1: [review(1, 'reviewer', 'CHANGES_REQUESTED'), review(2, 'reviewer')] + }); + await h.run(); + assert.equal(h.writes.at(-1).conclusion, 'success'); +}); + +test('a label change raises the requirement without trusting old success', async () => { + const current = pull(); + const h = harness([current], { 1: [review(1, 'reviewer')] }); + await h.run(); + assert.equal(h.writes.at(-1).conclusion, 'success'); + current.labels.push({ name: 'major-change' }); + await h.run(); + assert.equal(h.writes.at(-2).status, 'in_progress'); + assert.equal(h.writes.at(-1).conclusion, 'action_required'); + assert.match(h.writes.at(-1).output.summary, /1\/3/); + assert.equal(h.creates.length, 1); +}); + +test('PRs sharing a SHA cannot overwrite a stricter PR status with ordinary success', async () => { + const h = harness([pull(1, [], 'shared'), pull(2, ['major-change'], 'shared')], { + 1: [review(1, 'one')], + 2: [review(2, 'one'), review(3, 'two')] + }); + await h.run(); + assert.equal(h.writes.length, 2); + assert.equal(h.writes.at(-1).head_sha, 'shared'); + assert.match(h.writes.at(-1).output.summary, /#2: 2\/3/); + assert.deepEqual(h.permissionCalls, ['one', 'two']); +}); + +test('all PRs sharing a SHA must satisfy their own approval requirements', async () => { + const h = harness([pull(1, [], 'shared'), pull(2, ['major-change'], 'shared')], { + 1: [review(1, 'one')], + 2: [review(2, 'one'), review(3, 'two'), review(4, 'three')] + }); + await h.run(); + assert.equal(h.writes.at(-1).conclusion, 'success'); +}); + +test('review and permission API failures fail the check and workflow explicitly', async () => { + for (const option of ['reviewError', 'permissionError']) { + const error = new Error('GitHub API access failed'); + const h = harness([pull()], { 1: [review(1, 'reviewer')] }, { [option]: error }); + await h.run(); + assert.deepEqual(h.writes.map(check => check.status), ['in_progress', 'completed']); + assert.equal(h.writes.at(-1).conclusion, 'failure'); + assert.match(h.writes.at(-1).output.summary, /GitHub API access failed/); + assert.equal(h.errors.length, 1); + assert.equal(h.failed.length, 1); + } +}); + +test('draft PRs are skipped and no open PRs are reported explicitly', async () => { + const draft = pull(); + draft.draft = true; + for (const pulls of [[], [draft]]) { + const h = harness(pulls); + await h.run(); + assert.deepEqual(h.writes, []); + assert.match(h.logs[0], /No open/); + } +}); + +test('check summaries obey the 65535-character limit', async () => { + const h = harness(Array.from({ length: 3000 }, (_, index) => pull(index + 1, ['major-change'], 'shared'))); + await h.run(); + assert.equal(h.writes.at(-1).output.summary.length, 65535); +}); + +test('refreshing more than 1000 times updates one check instead of appending statuses', async () => { + const h = harness([pull()], { 1: [review(1, 'reviewer')] }); + for (let iteration = 0; iteration < 1001; iteration++) { + await h.run(); + } + assert.equal(h.creates.length, 1); + assert.equal(h.runs.length, 1); + assert.equal(h.writes.length, 2002); + assert.equal(h.writes.at(-1).conclusion, 'success'); +}); + +test('a failed SHA does not prevent other SHA groups from refreshing', async () => { + const h = harness([pull(1), pull(2)], { 2: [review(1, 'valid')] }, { + reviewErrors: { 1: new Error('First SHA reviews unavailable') } + }); + await h.run(); + assert.equal(h.runs.find(run => run.head_sha === 'sha-1').conclusion, 'failure'); + assert.equal(h.runs.find(run => run.head_sha === 'sha-2').conclusion, 'success'); + assert.match(h.failed[0], /1 SHA group/); +}); + +test('failure to reset an existing success attempts an explicit failure update', async () => { + const existing = { + id: 42, head_sha: 'sha-1', external_id: 'minipdf-review-approval-policy', + app: { slug: 'github-actions' }, status: 'completed', conclusion: 'success' + }; + const h = harness([pull()], {}, { + existingChecks: [existing], startError: new Error('Cannot start refresh') + }); + await h.run(); + assert.equal(h.creates.length, 0); + assert.equal(h.runs[0].conclusion, 'failure'); + assert.equal(h.failed.length, 1); +}); + +test('failure to publish a failure is logged and still fails the workflow', async () => { + const h = harness([pull()], {}, { + reviewError: new Error('Review API unavailable'), + failureUpdateError: new Error('Check API unavailable') + }); + await h.run(); + assert.equal(h.errors.length, 2); + assert.match(h.errors[1], /Failed to publish failure/); + assert.equal(h.runs[0].status, 'in_progress'); + assert.equal(h.failed.length, 1); +}); + +test('check creation failures are explicit and do not prevent other SHA groups', async () => { + const h = harness([pull(1), pull(2)], {}, { createError: new Error('Check API unavailable') }); + await h.run(); + assert.equal(h.errors.length, 2); + assert.match(h.failed[0], /2 SHA group/); + assert.deepEqual(h.writes, []); +}); + +test('checks belonging to another app or external ID are not updated', async () => { + const existingChecks = [ + { id: 1, head_sha: 'sha-1', external_id: 'minipdf-review-approval-policy', app: { slug: 'other-app' } }, + { id: 2, head_sha: 'sha-1', external_id: 'other-check', app: { slug: 'github-actions' } } + ]; + const h = harness([pull()], {}, { existingChecks }); + await h.run(); + assert.equal(h.creates.length, 1); + assert.ok(h.writes.every(run => run.id === 3)); +}); + +test('deleted review users never count as eligible approvals', async () => { + const h = harness([pull()], { 1: [{ id: 1, state: 'APPROVED', user: null }, review(2, 'valid')] }); + await h.run(); + assert.equal(h.writes.at(-1).conclusion, 'success'); + assert.deepEqual(h.permissionCalls, ['valid']); +}); + +test('advisory workflow has only base-controlled triggers and no PR notifier', () => { + assert.match(workflow, /pull_request_target:/); + assert.match(workflow, /branches: \[main\]/); + assert.match(workflow, /schedule:/); + assert.match(workflow, /cron: '7,22,37,52 \* \* \* \*'/); + assert.match(workflow, /workflow_dispatch:/); + assert.match(workflow, /checks: write/); + assert.match(workflow, /pull-requests: read/); + assert.doesNotMatch(workflow, /actions\/checkout|\$\{\{[^}]*head\.(?:sha|ref)|download-artifact|secrets\.|pull_request_review:|workflow_run:|statuses: write|createCommitStatus/); + assert.equal(existsSync(resolve(__dirname, '..', 'workflows', 'review-changed.yml')), false); + const contributing = readFileSync(resolve(__dirname, '..', '..', 'CONTRIBUTING.md'), 'utf8'); + assert.match(contributing, /Every PR targeting `main` must retain the native \*\*three-approval\*\* requirement/); + assert.match(contributing, /Do not use this advisory check to lower the native approval minimum to one/); +}); diff --git a/.github/workflows/auto-reviewers.yml b/.github/workflows/auto-reviewers.yml new file mode 100644 index 00000000..00f4cf1f --- /dev/null +++ b/.github/workflows/auto-reviewers.yml @@ -0,0 +1,125 @@ +name: Request core reviewers + +on: + pull_request_target: + branches: [main] + types: [opened, reopened, ready_for_review, synchronize] + workflow_dispatch: + inputs: + pull_request_number: + description: Open PR targeting main to refresh reviewers + required: true + type: string + +permissions: + pull-requests: write + +concurrency: + group: core-reviewers-${{ github.event.pull_request.number || inputs.pull_request_number }} + cancel-in-progress: false + +jobs: + request-reviewers: + if: github.event_name == 'workflow_dispatch' || !github.event.pull_request.draft + runs-on: ubuntu-latest + steps: + - name: Read current team members + id: team + uses: actions/github-script@v7 + env: + CORE_REVIEWERS_TOKEN: ${{ secrets.CORE_REVIEWERS_TOKEN }} + with: + github-token: ${{ secrets.CORE_REVIEWERS_TOKEN }} + result-encoding: string + script: | + if (!process.env.CORE_REVIEWERS_TOKEN) { + throw new Error('Configure CORE_REVIEWERS_TOKEN with organization Members: read permission.'); + } + + const members = await github.paginate(github.rest.teams.listMembersInOrg, { + org: context.repo.owner, + team_slug: 'core-reviewers', + per_page: 100 + }); + const reviewers = members + .filter(member => member.type !== 'Bot') + .map(member => member.login); + if (reviewers.length === 0) { + throw new Error('The core-reviewers team has no active human members.'); + } + core.info(`Loaded ${reviewers.length} active core reviewers.`); + return JSON.stringify(reviewers); + + - name: Request missing reviewers + uses: actions/github-script@v7 + env: + TEAM_REVIEWERS: ${{ steps.team.outputs.result }} + INPUT_PR_NUMBER: ${{ inputs.pull_request_number }} + with: + script: | + const input = context.payload.pull_request?.number ?? process.env.INPUT_PR_NUMBER; + const pull_number = Number(input); + if (!/^[1-9]\d*$/.test(String(input)) || !Number.isSafeInteger(pull_number)) { + throw new Error('A positive integer pull request number is required.'); + } + const params = { ...context.repo, pull_number }; + const { data: pull } = await github.rest.pulls.get(params); + if (pull.state !== 'open' || pull.draft || pull.base.ref !== 'main') { + core.notice('Skipping: the PR must be open, ready for review, and target main.'); + return; + } + + const requested = await github.paginate( + github.rest.pulls.listRequestedReviewers, + { ...params, per_page: 100 }, + response => response.data.users + ); + const reviews = await github.paginate(github.rest.pulls.listReviews, { + ...params, + per_page: 100 + }); + const excluded = new Set([ + pull.user.login, + ...requested.map(user => user.login), + ...reviews.filter(review => review.user).map(review => review.user.login) + ].map(login => login.toLowerCase())); + const reviewers = JSON.parse(process.env.TEAM_REVIEWERS); + const missing = []; + for (const login of reviewers) { + const key = login.toLowerCase(); + if (!excluded.has(key)) { + missing.push(login); + excluded.add(key); + } + } + if (missing.length === 0) { + core.info('All eligible team members have already been requested or reviewed.'); + return; + } + + // GitHub can omit reviewers from oversized requests, so limit each batch. + for (let offset = 0; offset < missing.length; offset += 15) { + await github.rest.pulls.requestReviewers({ + ...params, + reviewers: missing.slice(offset, offset + 15) + }); + } + + const updated = await github.paginate( + github.rest.pulls.listRequestedReviewers, + { ...params, per_page: 100 }, + response => response.data.users + ); + const updatedReviews = await github.paginate(github.rest.pulls.listReviews, { + ...params, + per_page: 100 + }); + const confirmed = new Set([ + ...updated.map(user => user.login), + ...updatedReviews.filter(review => review.user).map(review => review.user.login) + ].map(login => login.toLowerCase())); + const unconfirmed = missing.filter(login => !confirmed.has(login.toLowerCase())); + if (unconfirmed.length > 0) { + throw new Error(`GitHub did not confirm review requests for: ${unconfirmed.join(', ')}`); + } + core.info(`Requested ${missing.length} additional core reviewers.`); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a0810021..30f7a59e 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -25,6 +25,22 @@ permissions: pull-requests: read jobs: + review-workflow-tests: + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - name: Checkout + uses: actions/checkout@v4 + + - name: Setup Node.js + uses: actions/setup-node@v4 + with: + node-version: '20' + + - name: Test reviewer automation and approval policy + run: node --test .github/tests/auto-reviewers.test.cjs .github/tests/review-approval-policy.test.cjs + build: runs-on: ubuntu-latest diff --git a/.github/workflows/review-approval-policy.yml b/.github/workflows/review-approval-policy.yml new file mode 100644 index 00000000..68846acb --- /dev/null +++ b/.github/workflows/review-approval-policy.yml @@ -0,0 +1,151 @@ +name: Review approval policy + +on: + pull_request_target: + branches: [main] + types: [opened, reopened, synchronize, ready_for_review, converted_to_draft, labeled, unlabeled, edited, closed] + schedule: + - cron: '7,22,37,52 * * * *' + workflow_dispatch: + +permissions: + pull-requests: read + checks: write + +concurrency: + group: review-approval-policy + cancel-in-progress: false + +jobs: + check-approvals: + runs-on: ubuntu-latest + steps: + - name: Refresh approval requirements + uses: actions/github-script@v7 + with: + script: | + const checkName = 'Review approval policy'; + const externalId = 'minipdf-review-approval-policy'; + const pulls = await github.paginate(github.rest.pulls.list, { + ...context.repo, + state: 'open', + base: 'main', + per_page: 100 + }); + const groups = new Map(); + for (const pull of pulls.filter(pull => !pull.draft)) { + const group = groups.get(pull.head.sha) ?? []; + group.push(pull); + groups.set(pull.head.sha, group); + } + if (groups.size === 0) { + core.info('No open, ready-for-review PRs target main.'); + return; + } + const permissions = new Map(); + const failed = []; + for (const [sha, group] of groups) { + let check; + try { + const existing = await github.paginate(github.rest.checks.listForRef, { + ...context.repo, + ref: sha, + check_name: checkName, + filter: 'all', + per_page: 100 + }); + check = existing.find(run => + run.app?.slug === 'github-actions' && run.external_id === externalId + ); + const start = { + ...context.repo, + status: 'in_progress', + details_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}`, + output: { + title: 'Checking advisory approval requirements', + summary: 'Checking current labels and eligible approvals. Native three-approval protection remains required.' + } + }; + if (check) { + await github.rest.checks.update({ ...start, check_run_id: check.id }); + } else { + ({ data: check } = await github.rest.checks.create({ + ...start, name: checkName, head_sha: sha, external_id: externalId + })); + } + + const results = []; + for (const pull of group) { + const required = pull.labels.some(label => label.name.toLowerCase() === 'major-change') ? 3 : 1; + const reviews = await github.paginate(github.rest.pulls.listReviews, { + ...context.repo, + pull_number: pull.number, + per_page: 100 + }); + const latest = new Map(); + for (const review of reviews.sort((left, right) => left.id - right.id)) { + if (review.user && ['APPROVED', 'CHANGES_REQUESTED', 'DISMISSED'].includes(review.state)) { + latest.set(review.user.login.toLowerCase(), review); + } + } + let approvals = 0; + for (const [login, review] of latest) { + if (review.state !== 'APPROVED' || review.user.type === 'Bot' || + login === pull.user.login.toLowerCase()) { + continue; + } + if (!permissions.has(login)) { + const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ + ...context.repo, + username: review.user.login + }); + permissions.set(login, ['write', 'maintain', 'admin'].includes(data.permission)); + } + if (permissions.get(login)) { + approvals++; + } + } + results.push({ number: pull.number, required, approvals }); + } + + // Check runs are shared by PRs with the same head SHA. + const waiting = results.filter(result => result.approvals < result.required); + const summary = results + .map(result => `#${result.number}: ${result.approvals}/${result.required} eligible approvals`) + .join('\n'); + await github.rest.checks.update({ + ...context.repo, + check_run_id: check.id, + status: 'completed', + conclusion: waiting.length > 0 ? 'action_required' : 'success', + output: { + title: waiting.length > 0 ? 'Advisory approval requirements not met' : 'Advisory approval requirements met', + summary: `${summary}\n\nAdvisory only: retain native three-approval protection.`.slice(0, 65535) + } + }); + core.info(summary); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + core.error(`Approval refresh failed for ${sha}: ${message}`); + failed.push(sha); + if (check) { + try { + await github.rest.checks.update({ + ...context.repo, + check_run_id: check.id, + status: 'completed', + conclusion: 'failure', + output: { + title: 'Advisory approval refresh failed', + summary: `Unable to evaluate current approvals: ${message}\nRetain native three-approval protection.`.slice(0, 65535) + } + }); + } catch (reportError) { + core.error(`Failed to publish failure for ${sha}: ${String(reportError)}`); + } + } + } + } + if (failed.length > 0) { + core.setFailed(`Approval refresh failed for ${failed.length} SHA group(s): ${failed.join(', ')}`); + } diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8b47535c..ae9a0317 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -109,8 +109,7 @@ Pull requests should be small enough to review and should include: - the source and license of new third-party material; and - documentation updates for user-visible behavior. -At least one maintainer approval is currently required before merge. The -project aims to provide an initial review within seven days, but complex visual +The project aims to provide an initial review within seven days, but complex visual rendering and security changes may take longer. Authors should respond to review questions and avoid force-pushing after review begins unless rebasing is needed. @@ -118,6 +117,93 @@ Maintainers may request that a large change be split, discussed publicly, or reworked to preserve compatibility and project scope. Reviews assess the change, not the contributor. +### Automatic Review Requests + +PRs targeting `main` automatically request active human members of the +[`core-reviewers` team](https://github.com/orgs/mini-software/teams/core-reviewers) +when opened, reopened, marked ready for review, or updated. The PR author, +already requested reviewers, and anyone who has already submitted a review are +excluded. Draft PRs do not request reviews. Merge approval requirements are +configured separately from review invitations. + +Maintainers can update the reviewer list by adding or removing team members; +pending organization invitations must be accepted before members are included. +The team must have Write or higher repository access. Removing a member does +not revoke existing review requests or their separately granted permissions. + +Before enabling the [reviewer workflow](.github/workflows/auto-reviewers.yml), +configure a fine-grained personal access token with `mini-software` as the resource +owner and only the organization **Members: read** permission. Store it as the +repository Actions secret `CORE_REVIEWERS_TOKEN` and ensure any required +organization approval is complete. Rotate the token before it expires. Do not +reuse a local GitHub CLI login token. The dedicated token only reads team +membership; review requests use the workflow's `GITHUB_TOKEN` with +**Pull requests: write** permission. + +After the workflow is merged into `main`, use its **Run workflow** action with +a PR number to refresh reviewers on an existing PR. Team changes are otherwise +picked up on the next supported PR event, not immediately when membership changes. +The workflow uses `pull_request_target` to support fork PRs without checking out +or executing PR code. Missing credentials, team access errors, and unconfirmed +review requests fail the workflow explicitly. + +### Approval Requirements + +Every PR targeting `main` must retain the native **three-approval** requirement. +The automation reports these **advisory** thresholds: + +- Ordinary changes: at least **one approval** from someone other than the author. +- Important changes with the `major-change` label: at least **three approvals**. + +Approvals must come from users with Write or higher repository permissions. +Invitations, comments, author approvals, bot reviews, and dismissed approvals do +not count. A later approval or change request supersedes that user's earlier +decision; a comment does not cancel an approval. Existing native protections +still apply, including blocking unresolved change requests. + +Maintainers should add `major-change` before review for public API changes, +architecture changes, breaking changes, governance changes, or other high-impact +modifications. Do not remove the label merely to bypass the three-approval +requirement. This is a project policy, not an ASF-wide requirement for every +GitHub PR. + +The [approval workflow](.github/workflows/review-approval-policy.yml) updates a +**Review approval policy** check run for each head commit instead of repeatedly +appending commit statuses. It refreshes open, non-draft PRs on PR/label changes, +manual dispatch, and a default-branch-controlled schedule every 15 minutes. +Scheduled runs can be delayed by GitHub; review changes are not guaranteed to +refresh immediately. The workflow never checks out or executes PR code and does +not require `CORE_REVIEWERS_TOKEN`. + +Maintainers can manually run **Review approval policy** after review changes. +PRs sharing a head commit also share its check, so all such open, non-draft PRs +must meet their individual advisory thresholds for the check to succeed. +Failures are reported per head commit, set its check to failure when the API +allows, and fail the workflow without preventing other commits from refreshing. + +Do not use this advisory check to lower the native approval minimum to one. +GitHub's `pull_request_review` workflow runs from the PR merge ref, which a PR +can modify to disable dismissal notifications. It can also require maintainer +approval for first-time fork workflows. The former notifier is therefore removed; +scheduled reconciliation avoids relying on it but cannot eliminate stale results. +Enforcing ordinary-one/major-three would require a separately deployed, trusted +review-event service and its own validation before changing branch rules. + +Retain the native three-approval requirement and the **Copilot review for default +branch** ruleset's empty bypass list, Copilot review, deletion protection, and +force-push protection. The advisory check does not replace GitHub's merge gate. +Automatic team invitations can be enabled independently after the reviewer +workflow is merged into `main` and its dedicated secret is configured. + +The automation and its tests are adapted from +[mini-software/MiniExcel#1040](https://github.com/mini-software/MiniExcel/pull/1040) +under Apache License 2.0. CI runs both test files; run them locally with Node.js +20 or later: + +```powershell +node --test .github\tests\auto-reviewers.test.cjs .github\tests\review-approval-policy.test.cjs +``` + ## Earning More Responsibility Sustained contributions and sound judgment can lead to maintainer