From 35752b8656ba4cf984524ca5a135b1c184e773b9 Mon Sep 17 00:00:00 2001 From: Wei Lin Date: Sat, 14 Nov 2026 14:23:03 +0800 Subject: [PATCH 1/2] Document native tiered approval requirements Align advisory messages and policy tests with one approval for ordinary changes and three core-reviewer approvals for important file paths. Keep label-based advice separate from native enforcement. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/tests/review-approval-policy.test.cjs | 15 ++++- .github/workflows/review-approval-policy.yml | 6 +- CONTRIBUTING.md | 59 ++++++++++++++----- 3 files changed, 60 insertions(+), 20 deletions(-) diff --git a/.github/tests/review-approval-policy.test.cjs b/.github/tests/review-approval-policy.test.cjs index 71214908..febfc3f3 100644 --- a/.github/tests/review-approval-policy.test.cjs +++ b/.github/tests/review-approval-policy.test.cjs @@ -113,6 +113,7 @@ test('ordinary advisory threshold is exactly one eligible approval', async () => 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/); + assert.match(h.writes.at(-1).output.summary, /native file-based rules independently require one approval/); } }); @@ -324,6 +325,16 @@ test('advisory workflow has only base-controlled triggers and no PR notifier', ( 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/); + assert.match(contributing, /Ordinary changes: at least \*\*one approval\*\*/); + assert.match(contributing, /Important file changes: at least \*\*three approvals from `core-reviewers`\*\*/); + for (const pattern of [ + '.github/**', 'AGENTS.md', 'CONTRIBUTING.md', 'GOVERNANCE.md', + 'SECURITY.md', 'LICENSE', 'MAINTAINERS.md', 'CODE_OF_CONDUCT.md' + ]) { + assert.ok(contributing.split(/\r?\n/).includes(pattern), `Document important path: ${pattern}`); + } + assert.match(contributing, /The label does not change GitHub's native file-based requirements/); + assert.match(contributing, /empty\s+bypass list/); + assert.match(workflow, /Native rules require one approval, or three core-reviewer approvals for important paths/); + assert.doesNotMatch(workflow, /retain native three-approval protection|Native three-approval protection remains required/); }); diff --git a/.github/workflows/review-approval-policy.yml b/.github/workflows/review-approval-policy.yml index 68846acb..0896e814 100644 --- a/.github/workflows/review-approval-policy.yml +++ b/.github/workflows/review-approval-policy.yml @@ -63,7 +63,7 @@ jobs: 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.' + summary: 'Checking label-based advisory approvals. Native rules require one approval, or three core-reviewer approvals for important paths.' } }; if (check) { @@ -120,7 +120,7 @@ jobs: 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) + summary: `${summary}\n\nAdvisory only: native file-based rules independently require one approval, or three core-reviewer approvals for important paths.`.slice(0, 65535) } }); core.info(summary); @@ -137,7 +137,7 @@ jobs: conclusion: 'failure', output: { title: 'Advisory approval refresh failed', - summary: `Unable to evaluate current approvals: ${message}\nRetain native three-approval protection.`.slice(0, 65535) + summary: `Unable to evaluate current approvals: ${message}\nNative file-based approval requirements remain enforced independently.`.slice(0, 65535) } }); } catch (reportError) { diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ae9a0317..e1318291 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -149,11 +149,36 @@ 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: +GitHub's native rules enforce these approval requirements for PRs targeting `main`: + +- Ordinary changes: at least **one approval**. +- Important file changes: at least **three approvals from `core-reviewers`**. + These approvals also satisfy the ordinary one-approval minimum; a fourth + approval is not required. + +The important file patterns are: + +```text +.github/** +AGENTS.md +CONTRIBUTING.md +GOVERNANCE.md +SECURITY.md +LICENSE +MAINTAINERS.md +CODE_OF_CONDUCT.md +``` + +GitHub evaluates these patterns against the PR's changed files, not its size or +labels. Governance, licensing, and workflow changes therefore retain three-person +review, while ordinary rendering fixes and benchmark changes outside these paths +require one approval. The `core-reviewers` team must retain Write or higher +repository access. -- Ordinary changes: at least **one approval** from someone other than the author. -- Important changes with the `major-change` label: at least **three approvals**. +The automation separately reports these **label-based advisory** thresholds: + +- Without `major-change`: at least **one approval**. +- With `major-change`: 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 @@ -162,10 +187,11 @@ 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. +architecture changes, breaking changes, or other high-impact modifications. +The label does not change GitHub's native file-based requirements. In particular, +adding it to a PR outside the important paths raises only the advisory threshold; +removing it cannot bypass the native three-approval rule for important paths. +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 @@ -181,17 +207,20 @@ 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. +The advisory check is not the enforcement mechanism for important file changes; +GitHub's native required-reviewer rule enforces them without workflow timing or +PR-controlled code. Enforcing three approvals solely by label for changes outside +the important paths would require a separately deployed, trusted review-event +service and its own validation. + +Retain the native one-approval minimum, the three-approval required-reviewer rule +for important paths, 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. From 91459e35ca6bc69628ad73a825597687a994b599 Mon Sep 17 00:00:00 2001 From: Wei Lin Date: Sat, 14 Nov 2026 18:19:39 +0800 Subject: [PATCH 2/2] Make review approval counts non-blocking reminders Report insufficient approvals as neutral, preserve real automation errors, and document the maintainer-approved zero-approval native settings and optional reviewer requests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .github/tests/review-approval-policy.test.cjs | 36 ++++++---- .github/workflows/review-approval-policy.yml | 20 +++--- CONTRIBUTING.md | 71 ++++++++----------- 3 files changed, 63 insertions(+), 64 deletions(-) diff --git a/.github/tests/review-approval-policy.test.cjs b/.github/tests/review-approval-policy.test.cjs index febfc3f3..bd56070f 100644 --- a/.github/tests/review-approval-policy.test.cjs +++ b/.github/tests/review-approval-policy.test.cjs @@ -104,8 +104,8 @@ function harness(pulls, reviews = {}, options = {}) { 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']]) { +test('ordinary recommendation is neutral at zero approvals and succeeds at one', async () => { + for (const [reviews, expected] of [[[], 'neutral'], [[review(1, 'reviewer')], 'success']]) { const h = harness([pull()], { 1: reviews }); await h.run(); assert.equal(h.writes[0].status, 'in_progress'); @@ -113,19 +113,22 @@ test('ordinary advisory threshold is exactly one eligible approval', async () => 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/); - assert.match(h.writes.at(-1).output.summary, /native file-based rules independently require one approval/); + assert.match(h.writes.at(-1).output.summary, /maintainers may merge without waiting/); + assert.deepEqual(h.failed, []); } }); -test('major-change advisory check fails at two and passes at three approvals', async () => { +test('major-change recommendation stays neutral below 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'); + assert.equal(h.writes.at(-1).conclusion, count === 3 ? 'success' : 'neutral'); + assert.deepEqual(h.failed, []); if (count < 3) { assert.match(h.writes.at(-1).output.summary, new RegExp(`${count}/3`)); + assert.match(h.writes.at(-1).output.title, /Review reminder/); } } }); @@ -177,7 +180,7 @@ test('a new approval after a change request is counted', async () => { assert.equal(h.writes.at(-1).conclusion, 'success'); }); -test('a label change raises the requirement without trusting old success', async () => { +test('a label change raises the recommendation to a neutral reminder', async () => { const current = pull(); const h = harness([current], { 1: [review(1, 'reviewer')] }); await h.run(); @@ -185,12 +188,12 @@ test('a label change raises the requirement without trusting old success', async 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.equal(h.writes.at(-1).conclusion, 'neutral'); 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 () => { +test('PRs sharing a SHA preserve the higher recommendation without blocking', async () => { const h = harness([pull(1, [], 'shared'), pull(2, ['major-change'], 'shared')], { 1: [review(1, 'one')], 2: [review(2, 'one'), review(3, 'two')] @@ -198,11 +201,12 @@ test('PRs sharing a SHA cannot overwrite a stricter PR status with ordinary succ await h.run(); assert.equal(h.writes.length, 2); assert.equal(h.writes.at(-1).head_sha, 'shared'); + assert.equal(h.writes.at(-1).conclusion, 'neutral'); 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 () => { +test('all PRs sharing a SHA meeting recommendations produce success', 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')] @@ -325,16 +329,20 @@ test('advisory workflow has only base-controlled triggers and no PR notifier', ( 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, /Ordinary changes: at least \*\*one approval\*\*/); - assert.match(contributing, /Important file changes: at least \*\*three approvals from `core-reviewers`\*\*/); + assert.match(contributing, /approval counts are \*\*reminders only\*\*, not merge requirements/); + assert.match(contributing, /approval minimum for `main` is \*\*zero\*\*/); + assert.match(contributing, /native `minimum_approvals` set to \*\*zero\*\*/); + assert.match(contributing, /Without `major-change`: \*\*one approval recommended\*\*/); + assert.match(contributing, /With `major-change`: \*\*three approvals recommended\*\*/); for (const pattern of [ '.github/**', 'AGENTS.md', 'CONTRIBUTING.md', 'GOVERNANCE.md', 'SECURITY.md', 'LICENSE', 'MAINTAINERS.md', 'CODE_OF_CONDUCT.md' ]) { assert.ok(contributing.split(/\r?\n/).includes(pattern), `Document important path: ${pattern}`); } - assert.match(contributing, /The label does not change GitHub's native file-based requirements/); + assert.match(contributing, /The label changes only the recommendation, not merge eligibility/); assert.match(contributing, /empty\s+bypass list/); - assert.match(workflow, /Native rules require one approval, or three core-reviewer approvals for important paths/); - assert.doesNotMatch(workflow, /retain native three-approval protection|Native three-approval protection remains required/); + assert.match(workflow, /No approving reviews are required to merge/); + assert.match(workflow, /conclusion: waiting.length > 0 \? 'neutral' : 'success'/); + assert.doesNotMatch(workflow, /action_required|native three-approval protection|Native rules require one approval/); }); diff --git a/.github/workflows/review-approval-policy.yml b/.github/workflows/review-approval-policy.yml index 0896e814..fe83ed75 100644 --- a/.github/workflows/review-approval-policy.yml +++ b/.github/workflows/review-approval-policy.yml @@ -62,8 +62,8 @@ jobs: 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 label-based advisory approvals. Native rules require one approval, or three core-reviewer approvals for important paths.' + title: 'Checking review recommendations', + summary: 'Review counts are reminders only. No approving reviews are required to merge.' } }; if (check) { @@ -76,7 +76,7 @@ jobs: const results = []; for (const pull of group) { - const required = pull.labels.some(label => label.name.toLowerCase() === 'major-change') ? 3 : 1; + const recommended = 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, @@ -105,22 +105,22 @@ jobs: approvals++; } } - results.push({ number: pull.number, required, approvals }); + results.push({ number: pull.number, recommended, approvals }); } // Check runs are shared by PRs with the same head SHA. - const waiting = results.filter(result => result.approvals < result.required); + const waiting = results.filter(result => result.approvals < result.recommended); const summary = results - .map(result => `#${result.number}: ${result.approvals}/${result.required} eligible approvals`) + .map(result => `#${result.number}: ${result.approvals}/${result.recommended} recommended 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', + conclusion: waiting.length > 0 ? 'neutral' : 'success', output: { - title: waiting.length > 0 ? 'Advisory approval requirements not met' : 'Advisory approval requirements met', - summary: `${summary}\n\nAdvisory only: native file-based rules independently require one approval, or three core-reviewer approvals for important paths.`.slice(0, 65535) + title: waiting.length > 0 ? 'Review reminder: recommended approvals not yet reached' : 'Review recommendations met', + summary: `${summary}\n\nAdvisory only: maintainers may merge without waiting for these recommendations. This check must not be required for merging.`.slice(0, 65535) } }); core.info(summary); @@ -137,7 +137,7 @@ jobs: conclusion: 'failure', output: { title: 'Advisory approval refresh failed', - summary: `Unable to evaluate current approvals: ${message}\nNative file-based approval requirements remain enforced independently.`.slice(0, 65535) + summary: `Unable to evaluate current approvals: ${message}\nReview reminders are unavailable. Review approval counts do not gate merges.`.slice(0, 65535) } }); } catch (reportError) { diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e1318291..be30813f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -149,14 +149,13 @@ review requests fail the workflow explicitly. ### Approval Requirements -GitHub's native rules enforce these approval requirements for PRs targeting `main`: +Review approval counts are **reminders only**, not merge requirements. +Maintainers may merge PRs without waiting for approving reviews. The live native +approval minimum for `main` is **zero**, including the extra-approval setting for +unattributed changes, which is disabled. -- Ordinary changes: at least **one approval**. -- Important file changes: at least **three approvals from `core-reviewers`**. - These approvals also satisfy the ordinary one-approval minimum; a fourth - approval is not required. - -The important file patterns are: +The `core-reviewers` team remains an optional reviewer for these important paths, +with native `minimum_approvals` set to **zero**: ```text .github/** @@ -169,28 +168,23 @@ MAINTAINERS.md CODE_OF_CONDUCT.md ``` -GitHub evaluates these patterns against the PR's changed files, not its size or -labels. Governance, licensing, and workflow changes therefore retain three-person -review, while ordinary rendering fixes and benchmark changes outside these paths -require one approval. The `core-reviewers` team must retain Write or higher -repository access. - -The automation separately reports these **label-based advisory** thresholds: +Reviewer invitations and Copilot review remain enabled. The automation reports +these **label-based recommendations**: -- Without `major-change`: at least **one approval**. -- With `major-change`: at least **three approvals**. +- Without `major-change`: **one approval recommended**. +- With `major-change`: **three approvals recommended**. 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. +decision; a comment does not cancel an approval. These counts inform maintainers +but do not authorize or prevent a merge. Maintainers should add `major-change` before review for public API changes, architecture changes, breaking changes, or other high-impact modifications. -The label does not change GitHub's native file-based requirements. In particular, -adding it to a PR outside the important paths raises only the advisory threshold; -removing it cannot bypass the native three-approval rule for important paths. +The label changes only the recommendation, not merge eligibility. Important-file +review requests are also optional; neither the label nor a matching path requires +three approvals. 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 @@ -202,25 +196,22 @@ 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. - -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. -The advisory check is not the enforcement mechanism for important file changes; -GitHub's native required-reviewer rule enforces them without workflow timing or -PR-controlled code. Enforcing three approvals solely by label for changes outside -the important paths would require a separately deployed, trusted review-event -service and its own validation. - -Retain the native one-approval minimum, the three-approval required-reviewer rule -for important paths, 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. +PRs sharing a head commit also share its check. If any such PR has fewer than +the recommended approvals, the check finishes with a **neutral** reminder, not +`action_required` or failure. Meeting all recommendations produces success. +Neither outcome is a merge gate. API and permission failures remain explicit: +they fail the check and workflow without preventing other commits from refreshing. +They must not be disguised as successful review evaluations. + +Keep the **Review approval policy** check and reviewer-invitation workflow +**optional**, not required status checks. Missing organization credentials can +still make reviewer invitations fail; this is a configuration problem, not an +approval requirement. Merge conflicts and any separately configured required +checks must still be resolved. + +Retain the **Copilot review for default branch** ruleset's pull-request requirement, +empty bypass list, Copilot review, deletion protection, and force-push protection. +Do not restore approval-count gates without an explicit maintainer decision. Automatic team invitations can be enabled independently after the reviewer workflow is merged into `main` and its dedicated secret is configured.