diff --git a/.github/tests/review-approval-policy.test.cjs b/.github/tests/review-approval-policy.test.cjs index 71214908..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,18 +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, /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/); } } }); @@ -176,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(); @@ -184,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')] @@ -197,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')] @@ -324,6 +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, /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, /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 changes only the recommendation, not merge eligibility/); + assert.match(contributing, /empty\s+bypass list/); + 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 68846acb..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 current labels and eligible approvals. Native three-approval protection remains required.' + 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: retain native three-approval protection.`.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}\nRetain native three-approval protection.`.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 ae9a0317..be30813f 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -149,23 +149,43 @@ 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: +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. + +The `core-reviewers` team remains an optional reviewer for these important paths, +with native `minimum_approvals` set to **zero**: + +```text +.github/** +AGENTS.md +CONTRIBUTING.md +GOVERNANCE.md +SECURITY.md +LICENSE +MAINTAINERS.md +CODE_OF_CONDUCT.md +``` + +Reviewer invitations and Copilot review remain enabled. The automation reports +these **label-based recommendations**: -- Ordinary changes: at least **one approval** from someone other than the author. -- Important changes with the `major-change` label: 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, 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 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 **Review approval policy** check run for each head commit instead of repeatedly @@ -176,22 +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. - -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. +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.