Skip to content

Commit 84a35d4

Browse files
committed
fix: require 2+ reviews for ready-to-merge, not just maintainer alone
Addresses maintainer feedback: a single maintainer approval with only 1 total review should not mark a PR as ready-to-merge, since branch protection requires 2+ reviews. Changes: - determineLabel() now requires maintainerApproval >= 1 AND anyApproval >= 2 for ready-to-merge - If a maintainer approves alone, the PR stays at queue:maintainers until a second reviewer also approves - Updated and added unit tests (38 total, all passing) Signed-off-by: darshit2308 <darshit2308@gmail.com>
1 parent e0be030 commit 84a35d4

2 files changed

Lines changed: 29 additions & 11 deletions

File tree

.github/scripts/review-sync/helpers/labels.js

Lines changed: 10 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -57,19 +57,23 @@ async function ensureLabel(github, owner, repo, label, dryRun) {
5757
* Determine the correct queue label for a PR based on approval counts.
5858
*
5959
* Phase 1 logic (4-stage pipeline):
60-
* maintainerApproval >= 1 → ready-to-merge (CODEOWNERS satisfied)
61-
* writeApproval >= 1 → queue:maintainers (committer approved, needs maintainer)
62-
* anyApproval >= 1 → queue:committers (has any approval, needs committer)
63-
* else → queue:junior-committer (no approvals yet)
60+
* maintainerApproval >= 1 AND anyApproval >= 2 → ready-to-merge (CODEOWNERS + min reviews)
61+
* writeApproval >= 1 OR maintainerApproval >= 1 → queue:maintainers (senior review present, needs more)
62+
* anyApproval >= 1 → queue:committers (has any approval, needs committer)
63+
* else → queue:junior-committer (no approvals yet)
64+
*
65+
* Note: ready-to-merge requires BOTH a maintainer approval AND at least 2
66+
* total reviews. This prevents a single maintainer approval from marking
67+
* a PR as ready-to-merge when branch protection requires 2+ reviews.
6468
*
6569
* @param {{ maintainerApproval: number, writeApproval: number, softApproval: number, anyApproval: number }} approvals
6670
* @returns {object} The correct QUEUE_LABELS entry
6771
*/
6872
function determineLabel(approvals) {
69-
if (approvals.maintainerApproval >= 1) {
73+
if (approvals.maintainerApproval >= 1 && approvals.anyApproval >= 2) {
7074
return QUEUE_LABELS.MERGE;
7175
}
72-
if (approvals.writeApproval >= 1) {
76+
if (approvals.writeApproval >= 1 || approvals.maintainerApproval >= 1) {
7377
return QUEUE_LABELS.MAINTAINERS;
7478
}
7579
if (approvals.anyApproval >= 1) {

.github/scripts/review-sync/tests/test-labels.js

Lines changed: 19 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -39,19 +39,27 @@ const unitTests = [
3939
},
4040
},
4141
{
42-
name: 'determineLabel: 1 maintainer ready-to-merge',
42+
name: 'determineLabel: 1 maintainer alone → queue:maintainers (NOT ready-to-merge, needs 2 reviews)',
4343
test: () => {
44+
// Sophie's edge case: maintainer approves first, only 1 total review
4445
const r = determineLabel({ maintainerApproval: 1, writeApproval: 0, softApproval: 0, anyApproval: 1 });
45-
return r.name === 'ready-to-merge';
46+
return r.name === 'queue:maintainers';
4647
},
4748
},
4849
{
49-
name: 'determineLabel: 1 maintainer + 1 write → ready-to-merge',
50+
name: 'determineLabel: 1 maintainer + 1 write → ready-to-merge (2 reviews satisfied)',
5051
test: () => {
5152
const r = determineLabel({ maintainerApproval: 1, writeApproval: 1, softApproval: 0, anyApproval: 2 });
5253
return r.name === 'ready-to-merge';
5354
},
5455
},
56+
{
57+
name: 'determineLabel: 1 maintainer + 1 soft → ready-to-merge (2 reviews satisfied)',
58+
test: () => {
59+
const r = determineLabel({ maintainerApproval: 1, writeApproval: 0, softApproval: 1, anyApproval: 2 });
60+
return r.name === 'ready-to-merge';
61+
},
62+
},
5563
{
5664
name: 'determineLabel: 3 soft, 0 write → queue:committers',
5765
test: () => {
@@ -103,8 +111,14 @@ const unitTests = [
103111
name: 'syncLabel: stale label → adds correct, removes stale',
104112
test: async () => {
105113
const mock = createMockGithub({
106-
roles: { sophie: { role_name: 'maintain', permission: 'write' } },
107-
reviews: [{ user: { login: 'sophie' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }],
114+
roles: {
115+
sophie: { role_name: 'maintain', permission: 'write' },
116+
bob: { role_name: 'write', permission: 'write' },
117+
},
118+
reviews: [
119+
{ user: { login: 'sophie' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' },
120+
{ user: { login: 'bob' }, state: 'APPROVED', submitted_at: '2026-01-02T00:00:00Z' },
121+
],
108122
});
109123
const changed = await syncLabel(mock, 'o', 'r', { number: 1, labels: [{ name: 'queue:junior-committer' }] }, false);
110124
return changed === true && mock.calls.labelsAdded.includes('ready-to-merge') && mock.calls.labelsRemoved.includes('queue:junior-committer');

0 commit comments

Comments
 (0)