From 140b79ed74d23cf107c0e5cd0e250d1a9ba8df1f Mon Sep 17 00:00:00 2001 From: Wei Lin Date: Thu, 12 Nov 2026 23:32:44 +0800 Subject: [PATCH 1/2] Automate core reviewer requests and tiered PR approvals Adapt MiniExcel reviewer workflows for main, add focused CI tests, and document safe activation without lowering existing protection. --- .github/tests/auto-reviewers.test.cjs | 188 ++++++++++++++++ .github/tests/review-approval-policy.test.cjs | 209 ++++++++++++++++++ .github/workflows/auto-reviewers.yml | 117 ++++++++++ .github/workflows/ci.yml | 16 ++ .github/workflows/review-approval-policy.yml | 105 +++++++++ .github/workflows/review-changed.yml | 14 ++ CONTRIBUTING.md | 90 +++++++- 7 files changed, 737 insertions(+), 2 deletions(-) create mode 100644 .github/tests/auto-reviewers.test.cjs create mode 100644 .github/tests/review-approval-policy.test.cjs create mode 100644 .github/workflows/auto-reviewers.yml create mode 100644 .github/workflows/review-approval-policy.yml create mode 100644 .github/workflows/review-changed.yml diff --git a/.github/tests/auto-reviewers.test.cjs b/.github/tests/auto-reviewers.test.cjs new file mode 100644 index 00000000..070ab2fd --- /dev/null +++ b/.github/tests/auto-reviewers.test.cjs @@ -0,0 +1,188 @@ +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.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 github = { + rest: { + pulls: { + get: async params => { + assert.ok(Number.isSafeInteger(params.pull_number)); + return { data: pull }; + }, + listRequestedReviewers: async () => ({ data: { users: requested } }), + 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) => { + assert.equal(method, listReviews); + assert.equal(params.per_page, 100); + 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 }; +} + +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('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..783c2f0d --- /dev/null +++ b/.github/tests/review-approval-policy.test.cjs @@ -0,0 +1,209 @@ +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', 'review-approval-policy.yml'), 'utf8'); +const signal = readFileSync(resolve(__dirname, '..', 'workflows', 'review-changed.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 statuses = []; + const logs = []; + const permissionCalls = []; + const list = async () => {}; + const listReviews = async () => {}; + const github = { + rest: { + pulls: { list, listReviews }, + repos: { + createCommitStatus: async status => { + statuses.push(status); + }, + 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; + } + assert.equal(method, listReviews); + if (options.reviewError) { + throw options.reviewError; + } + return [...(reviews[params.pull_number] ?? [])]; + } + }; + const context = { + repo: { owner: 'mini-software', repo: 'MiniPdf' }, + serverUrl: 'https://github.com', runId: 123, + payload: { workflow_run: { pull_requests: [], head_repository: { full_name: 'untrusted/fork' } } } + }; + const run = () => evaluate(github, context, { info: message => logs.push(message) }); + return { run, statuses, logs, permissionCalls }; +} + +test('ordinary PR requires exactly one eligible approval', async () => { + for (const [reviews, expected] of [[[], 'pending'], [[review(1, 'reviewer')], 'success']]) { + const h = harness([pull()], { 1: reviews }); + await h.run(); + assert.equal(h.statuses[0].state, 'pending'); + assert.equal(h.statuses.at(-1).state, expected); + assert.equal(h.statuses.at(-1).context, 'Review approval policy'); + } +}); + +test('major-change PR remains blocked 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.statuses.at(-1).state, count === 3 ? 'success' : 'pending'); + if (count < 3) { + assert.match(h.statuses.at(-1).description, 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.statuses.at(-1).description, /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.statuses.at(-1).state, '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.statuses.at(-1).description, /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.statuses.at(-1).description, /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.statuses.at(-1).state, '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.statuses.at(-1).state, 'success'); + current.labels.push({ name: 'major-change' }); + await h.run(); + assert.equal(h.statuses.at(-2).state, 'pending'); + assert.match(h.statuses.at(-1).description, /1\/3/); +}); + +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.statuses.length, 2); + assert.equal(h.statuses.at(-1).sha, 'shared'); + assert.match(h.statuses.at(-1).description, /#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.statuses.at(-1).state, 'success'); +}); + +test('review and permission API failures leave checks pending and propagate', 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 assert.rejects(h.run(), error); + assert.deepEqual(h.statuses.map(status => status.state), ['pending']); + } +}); + +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.statuses, []); + assert.match(h.logs[0], /No open/); + } +}); + +test('commit status descriptions obey the GitHub length limit', async () => { + const h = harness(Array.from({ length: 20 }, (_, index) => pull(index + 1, ['major-change'], 'shared'))); + await h.run(); + assert.equal(h.statuses.at(-1).description.length, 140); +}); + +test('trusted workflow uses no PR code, artifact content, team credential or write-capable review trigger', () => { + assert.match(workflow, /pull_request_target:/); + assert.match(workflow, /branches: \[main\]/); + assert.match(workflow, /workflow_run:/); + assert.match(workflow, /workflows: \[PR review changed\]/); + assert.match(workflow, /statuses: write/); + assert.match(workflow, /pull-requests: read/); + assert.doesNotMatch(workflow, /actions\/checkout|\$\{\{[^}]*head\.(?:sha|ref)|download-artifact|secrets\.|pull_request_review:/); + assert.match(signal, /pull_request_review:/); + assert.match(signal, /types: \[submitted, dismissed\]/); + assert.match(signal, /permissions: \{\}/); + assert.doesNotMatch(signal, /actions\/checkout|\$\{\{|secrets\.|write/); +}); diff --git a/.github/workflows/auto-reviewers.yml b/.github/workflows/auto-reviewers.yml new file mode 100644 index 00000000..f51e7805 --- /dev/null +++ b/.github/workflows/auto-reviewers.yml @@ -0,0 +1,117 @@ +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 { data: requested } = await github.rest.pulls.listRequestedReviewers(params); + const reviews = await github.paginate(github.rest.pulls.listReviews, { + ...params, + per_page: 100 + }); + const excluded = new Set([ + pull.user.login, + ...requested.users.map(user => user.login), + ...reviews.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 { data: updated } = await github.rest.pulls.listRequestedReviewers(params); + const updatedReviews = await github.paginate(github.rest.pulls.listReviews, { + ...params, + per_page: 100 + }); + const confirmed = new Set([ + ...updated.users.map(user => user.login), + ...updatedReviews.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..7899bf35 --- /dev/null +++ b/.github/workflows/review-approval-policy.yml @@ -0,0 +1,105 @@ +name: Review approval policy + +on: + pull_request_target: + branches: [main] + types: [opened, reopened, synchronize, ready_for_review, converted_to_draft, labeled, unlabeled, edited, closed] + workflow_run: + workflows: [PR review changed] + types: [completed] + workflow_dispatch: + +permissions: + pull-requests: read + statuses: 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 statusContext = '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 writeStatus = (sha, state, description) => github.rest.repos.createCommitStatus({ + ...context.repo, + sha, + state, + context: statusContext, + description: description.slice(0, 140), + target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}` + }); + for (const sha of groups.keys()) { + await writeStatus(sha, 'pending', 'Checking current labels and eligible approvals.'); + } + + const permissions = new Map(); + for (const [sha, group] of groups) { + 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 (['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 }); + } + + // Commit statuses are shared by PRs with the same head SHA. + const waiting = results.filter(result => result.approvals < result.required); + if (waiting.length > 0) { + const description = waiting + .map(result => `#${result.number}: ${result.approvals}/${result.required} eligible approvals`) + .join('; '); + await writeStatus(sha, 'pending', description); + core.info(description); + } else { + await writeStatus(sha, 'success', `Approval policy satisfied for ${group.length} open PR(s).`); + core.info(`Approval policy satisfied for ${group.map(pull => `#${pull.number}`).join(', ')}.`); + } + } diff --git a/.github/workflows/review-changed.yml b/.github/workflows/review-changed.yml new file mode 100644 index 00000000..5a9f7880 --- /dev/null +++ b/.github/workflows/review-changed.yml @@ -0,0 +1,14 @@ +name: PR review changed + +on: + pull_request_review: + types: [submitted, dismissed] + +permissions: {} + +jobs: + notify: + runs-on: ubuntu-latest + steps: + - name: Signal trusted approval refresh + run: echo "Review changed; the trusted approval policy workflow will refresh open PRs." diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 8b47535c..be1b36a8 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 + +The tiered review policy for PRs targeting `main`, once activated below, is: + +- 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) publishes +the commit status **Review approval policy**. It refreshes all open, non-draft +PRs on PR/label changes and after the +[read-only review notification workflow](.github/workflows/review-changed.yml) +completes. This two-workflow design supports fork reviews without granting the +notification workflow secrets or write permissions, or running PR code in the +privileged workflow. Neither approval workflow requires `CORE_REVIEWERS_TOKEN`. + +Changes are processed asynchronously. Wait for the approval workflow to finish +after changing labels or reviews, then check the status before merging. +Maintainers can manually run **Review approval policy** to refresh existing PRs. +PRs sharing a head commit also share its status, so all such open, non-draft PRs +must meet their individual approval requirements. + +To activate enforcement without a gap: + +1. Merge these workflows into `main` while retaining the current native + **three-approval** requirement for every PR. +2. Run **Review approval policy** and verify its live commit status on open PRs. +3. In the **Copilot review for default branch** ruleset, require the + **Review approval policy** status check from **GitHub Actions**, then lower the + native required approvals to **one** in the same ruleset update. Preserve the + empty bypass list, Copilot review, deletion protection, and force-push protection. + +Until this activation is complete, the native three-approval requirement +continues to apply to every PR. Do not lower it before the required status check +is configured. Team-based automatic invitations additionally require the secret +described above, but tiered approval enforcement does not. + +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 From 4dab3e0d07de8cb929a0fb83215e2482f1cabbf9 Mon Sep 17 00:00:00 2001 From: Wei Lin Date: Fri, 13 Nov 2026 14:49:34 +0800 Subject: [PATCH 2/2] Fix reviewer automation and keep approval checks advisory Paginate reviewer requests and handle deleted users. Reuse check runs with per-SHA failure reporting and continue refreshing independent commits. Remove the PR-controlled review notifier and unsafe activation guidance; retain native three-approval protection with scheduled advisory reconciliation. --- .github/tests/auto-reviewers.test.cjs | 37 ++- .github/tests/review-approval-policy.test.cjs | 214 ++++++++++++++---- .github/workflows/auto-reviewers.yml | 20 +- .github/workflows/review-approval-policy.yml | 150 +++++++----- .github/workflows/review-changed.yml | 14 -- CONTRIBUTING.md | 58 ++--- 6 files changed, 340 insertions(+), 153 deletions(-) delete mode 100644 .github/workflows/review-changed.yml diff --git a/.github/tests/auto-reviewers.test.cjs b/.github/tests/auto-reviewers.test.cjs index 070ab2fd..e36e406f 100644 --- a/.github/tests/auto-reviewers.test.cjs +++ b/.github/tests/auto-reviewers.test.cjs @@ -23,7 +23,7 @@ function harness(options = {}) { notice: message => logs.push(message) }; const requested = (options.requested ?? []).map(login => ({ login })); - const reviews = (options.reviewed ?? []).map(login => ({ user: { login } })); + const reviews = options.reviews ?? (options.reviewed ?? []).map(login => ({ user: { login } })); const batches = []; const pull = { state: 'open', @@ -33,6 +33,8 @@ function harness(options = {}) { ...options.pull }; const listReviews = async () => {}; + const listRequestedReviewers = async () => ({ data: { users: requested.slice(0, 30) } }); + const requestedPages = []; const github = { rest: { pulls: { @@ -40,7 +42,7 @@ function harness(options = {}) { assert.ok(Number.isSafeInteger(params.pull_number)); return { data: pull }; }, - listRequestedReviewers: async () => ({ data: { users: requested } }), + listRequestedReviewers, listReviews, requestReviewers: async ({ reviewers }) => { if (options.error) { @@ -58,9 +60,19 @@ function harness(options = {}) { } } }, - paginate: async (method, params) => { - assert.equal(method, listReviews); + 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]; } }; @@ -68,7 +80,7 @@ function harness(options = {}) { github, customContext, core, { env: { TEAM_REVIEWERS: JSON.stringify(members), INPUT_PR_NUMBER: input } } ); - return { run, logs, batches }; + return { run, logs, batches, requestedPages }; } test('team reader requires an explicit organization credential', async () => { @@ -111,6 +123,21 @@ test('requests all twenty eligible members in batches of at most fifteen', async 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']); diff --git a/.github/tests/review-approval-policy.test.cjs b/.github/tests/review-approval-policy.test.cjs index 783c2f0d..71214908 100644 --- a/.github/tests/review-approval-policy.test.cjs +++ b/.github/tests/review-approval-policy.test.cjs @@ -1,10 +1,9 @@ const assert = require('node:assert/strict'); -const { readFileSync } = require('node:fs'); +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 signal = readFileSync(resolve(__dirname, '..', 'workflows', 'review-changed.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; @@ -22,18 +21,47 @@ function review(id, login, state = 'APPROVED', type = 'User') { } function harness(pulls, reviews = {}, options = {}) { - const statuses = []; + 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 }, - repos: { - createCommitStatus: async status => { - statuses.push(status); + 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) { @@ -50,9 +78,15 @@ function harness(pulls, reviews = {}, options = {}) { 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); - if (options.reviewError) { - throw options.reviewError; + const error = options.reviewErrors?.[params.pull_number] ?? options.reviewError; + if (error) { + throw error; } return [...(reviews[params.pull_number] ?? [])]; } @@ -60,31 +94,37 @@ function harness(pulls, reviews = {}, options = {}) { const context = { repo: { owner: 'mini-software', repo: 'MiniPdf' }, serverUrl: 'https://github.com', runId: 123, - payload: { workflow_run: { pull_requests: [], head_repository: { full_name: 'untrusted/fork' } } } + payload: {} }; - const run = () => evaluate(github, context, { info: message => logs.push(message) }); - return { run, statuses, logs, permissionCalls }; + 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 PR requires exactly one eligible approval', async () => { - for (const [reviews, expected] of [[[], 'pending'], [[review(1, 'reviewer')], 'success']]) { +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.statuses[0].state, 'pending'); - assert.equal(h.statuses.at(-1).state, expected); - assert.equal(h.statuses.at(-1).context, 'Review approval policy'); + 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 PR remains blocked at two and passes at three approvals', async () => { +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.statuses.at(-1).state, count === 3 ? 'success' : 'pending'); + assert.equal(h.writes.at(-1).conclusion, count === 3 ? 'success' : 'action_required'); if (count < 3) { - assert.match(h.statuses.at(-1).description, new RegExp(`${count}/3`)); + assert.match(h.writes.at(-1).output.summary, new RegExp(`${count}/3`)); } } }); @@ -98,7 +138,7 @@ test('invited users, comments, author approvals, bots and Read users do not coun ] }, { permissions: { 'read-user': 'read' } }); await h.run(); - assert.match(h.statuses.at(-1).description, /1\/3/); + assert.match(h.writes.at(-1).output.summary, /1\/3/); assert.deepEqual(h.permissionCalls, ['read-user', 'valid']); }); @@ -107,7 +147,7 @@ test('counts Write, Maintain and Admin approvals', async () => { 1: [review(1, 'writer'), review(2, 'maintainer'), review(3, 'admin')] }, { permissions: { writer: 'write', maintainer: 'maintain', admin: 'admin' } }); await h.run(); - assert.equal(h.statuses.at(-1).state, 'success'); + assert.equal(h.writes.at(-1).conclusion, 'success'); }); test('later dismissed or changes-requested reviews supersede earlier approvals', async () => { @@ -116,7 +156,7 @@ test('later dismissed or changes-requested reviews supersede earlier approvals', 1: [review(2, 'REVIEWER', state), review(1, 'reviewer')] }); await h.run(); - assert.match(h.statuses.at(-1).description, /0\/1/); + assert.match(h.writes.at(-1).output.summary, /0\/1/); } }); @@ -125,7 +165,7 @@ test('comments do not cancel approvals and duplicate approvals count only once', 1: [review(1, 'reviewer'), review(2, 'REVIEWER'), review(3, 'Reviewer', 'COMMENTED')] }); await h.run(); - assert.match(h.statuses.at(-1).description, /1\/3/); + assert.match(h.writes.at(-1).output.summary, /1\/3/); }); test('a new approval after a change request is counted', async () => { @@ -133,18 +173,20 @@ test('a new approval after a change request is counted', async () => { 1: [review(1, 'reviewer', 'CHANGES_REQUESTED'), review(2, 'reviewer')] }); await h.run(); - assert.equal(h.statuses.at(-1).state, 'success'); + 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.statuses.at(-1).state, 'success'); + assert.equal(h.writes.at(-1).conclusion, 'success'); current.labels.push({ name: 'major-change' }); await h.run(); - assert.equal(h.statuses.at(-2).state, 'pending'); - assert.match(h.statuses.at(-1).description, /1\/3/); + 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 () => { @@ -153,9 +195,9 @@ test('PRs sharing a SHA cannot overwrite a stricter PR status with ordinary succ 2: [review(2, 'one'), review(3, 'two')] }); await h.run(); - assert.equal(h.statuses.length, 2); - assert.equal(h.statuses.at(-1).sha, 'shared'); - assert.match(h.statuses.at(-1).description, /#2: 2\/3/); + 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']); }); @@ -165,15 +207,19 @@ test('all PRs sharing a SHA must satisfy their own approval requirements', async 2: [review(2, 'one'), review(3, 'two'), review(4, 'three')] }); await h.run(); - assert.equal(h.statuses.at(-1).state, 'success'); + assert.equal(h.writes.at(-1).conclusion, 'success'); }); -test('review and permission API failures leave checks pending and propagate', async () => { +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 assert.rejects(h.run(), error); - assert.deepEqual(h.statuses.map(status => status.state), ['pending']); + 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); } }); @@ -183,27 +229,101 @@ test('draft PRs are skipped and no open PRs are reported explicitly', async () = for (const pulls of [[], [draft]]) { const h = harness(pulls); await h.run(); - assert.deepEqual(h.statuses, []); + assert.deepEqual(h.writes, []); assert.match(h.logs[0], /No open/); } }); -test('commit status descriptions obey the GitHub length limit', async () => { - const h = harness(Array.from({ length: 20 }, (_, index) => pull(index + 1, ['major-change'], 'shared'))); +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.statuses.at(-1).description.length, 140); + assert.equal(h.writes.at(-1).conclusion, 'success'); + assert.deepEqual(h.permissionCalls, ['valid']); }); -test('trusted workflow uses no PR code, artifact content, team credential or write-capable review trigger', () => { +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, /workflow_run:/); - assert.match(workflow, /workflows: \[PR review changed\]/); - assert.match(workflow, /statuses: write/); + 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:/); - assert.match(signal, /pull_request_review:/); - assert.match(signal, /types: \[submitted, dismissed\]/); - assert.match(signal, /permissions: \{\}/); - assert.doesNotMatch(signal, /actions\/checkout|\$\{\{|secrets\.|write/); + 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 index f51e7805..00f4cf1f 100644 --- a/.github/workflows/auto-reviewers.yml +++ b/.github/workflows/auto-reviewers.yml @@ -69,15 +69,19 @@ jobs: return; } - const { data: requested } = await github.rest.pulls.listRequestedReviewers(params); + 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.users.map(user => user.login), - ...reviews.map(review => review.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 = []; @@ -101,14 +105,18 @@ jobs: }); } - const { data: updated } = await github.rest.pulls.listRequestedReviewers(params); + 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.users.map(user => user.login), - ...updatedReviews.map(review => review.user.login) + ...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) { diff --git a/.github/workflows/review-approval-policy.yml b/.github/workflows/review-approval-policy.yml index 7899bf35..68846acb 100644 --- a/.github/workflows/review-approval-policy.yml +++ b/.github/workflows/review-approval-policy.yml @@ -4,14 +4,13 @@ on: pull_request_target: branches: [main] types: [opened, reopened, synchronize, ready_for_review, converted_to_draft, labeled, unlabeled, edited, closed] - workflow_run: - workflows: [PR review changed] - types: [completed] + schedule: + - cron: '7,22,37,52 * * * *' workflow_dispatch: permissions: pull-requests: read - statuses: write + checks: write concurrency: group: review-approval-policy @@ -25,7 +24,8 @@ jobs: uses: actions/github-script@v7 with: script: | - const statusContext = 'Review approval policy'; + const checkName = 'Review approval policy'; + const externalId = 'minipdf-review-approval-policy'; const pulls = await github.paginate(github.rest.pulls.list, { ...context.repo, state: 'open', @@ -42,64 +42,110 @@ jobs: core.info('No open, ready-for-review PRs target main.'); return; } - const writeStatus = (sha, state, description) => github.rest.repos.createCommitStatus({ - ...context.repo, - sha, - state, - context: statusContext, - description: description.slice(0, 140), - target_url: `${context.serverUrl}/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}` - }); - for (const sha of groups.keys()) { - await writeStatus(sha, 'pending', 'Checking current labels and eligible approvals.'); - } - const permissions = new Map(); + const failed = []; for (const [sha, group] of groups) { - 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, { + let check; + try { + const existing = await github.paginate(github.rest.checks.listForRef, { ...context.repo, - pull_number: pull.number, + ref: sha, + check_name: checkName, + filter: 'all', per_page: 100 }); - const latest = new Map(); - for (const review of reviews.sort((left, right) => left.id - right.id)) { - if (['APPROVED', 'CHANGES_REQUESTED', 'DISMISSED'].includes(review.state)) { - latest.set(review.user.login.toLowerCase(), review); + 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 + })); } - 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)); + + 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); + } } - if (permissions.get(login)) { - approvals++; + 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 }); } - results.push({ number: pull.number, required, approvals }); - } - // Commit statuses are shared by PRs with the same head SHA. - const waiting = results.filter(result => result.approvals < result.required); - if (waiting.length > 0) { - const description = waiting + // 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('; '); - await writeStatus(sha, 'pending', description); - core.info(description); - } else { - await writeStatus(sha, 'success', `Approval policy satisfied for ${group.length} open PR(s).`); - core.info(`Approval policy satisfied for ${group.map(pull => `#${pull.number}`).join(', ')}.`); + .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/.github/workflows/review-changed.yml b/.github/workflows/review-changed.yml deleted file mode 100644 index 5a9f7880..00000000 --- a/.github/workflows/review-changed.yml +++ /dev/null @@ -1,14 +0,0 @@ -name: PR review changed - -on: - pull_request_review: - types: [submitted, dismissed] - -permissions: {} - -jobs: - notify: - runs-on: ubuntu-latest - steps: - - name: Signal trusted approval refresh - run: echo "Review changed; the trusted approval policy workflow will refresh open PRs." diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index be1b36a8..ae9a0317 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -149,7 +149,8 @@ review requests fail the workflow explicitly. ### Approval Requirements -The tiered review policy for PRs targeting `main`, once activated below, is: +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**. @@ -166,34 +167,33 @@ 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) publishes -the commit status **Review approval policy**. It refreshes all open, non-draft -PRs on PR/label changes and after the -[read-only review notification workflow](.github/workflows/review-changed.yml) -completes. This two-workflow design supports fork reviews without granting the -notification workflow secrets or write permissions, or running PR code in the -privileged workflow. Neither approval workflow requires `CORE_REVIEWERS_TOKEN`. - -Changes are processed asynchronously. Wait for the approval workflow to finish -after changing labels or reviews, then check the status before merging. -Maintainers can manually run **Review approval policy** to refresh existing PRs. -PRs sharing a head commit also share its status, so all such open, non-draft PRs -must meet their individual approval requirements. - -To activate enforcement without a gap: - -1. Merge these workflows into `main` while retaining the current native - **three-approval** requirement for every PR. -2. Run **Review approval policy** and verify its live commit status on open PRs. -3. In the **Copilot review for default branch** ruleset, require the - **Review approval policy** status check from **GitHub Actions**, then lower the - native required approvals to **one** in the same ruleset update. Preserve the - empty bypass list, Copilot review, deletion protection, and force-push protection. - -Until this activation is complete, the native three-approval requirement -continues to apply to every PR. Do not lower it before the required status check -is configured. Team-based automatic invitations additionally require the secret -described above, but tiered approval enforcement does not. +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)