Skip to content

Commit 8d48308

Browse files
committed
fix: address review feedback from Gourav
1. index.js: Exit with code 1 if errors occurred during PR processing so that the GitHub Action workflow fails instead of hiding failures behind a green checkmark. 2. labels.js: Tighten ready-to-merge logic to strictly require at least 2 core reviews (maintainer + write). Soft approvals (triage/read) no longer satisfy the second review requirement for merge status. 3. test-labels.js: Update tests to reflect the new strict requirement. Signed-off-by: darshit2308 <darshit2308@gmail.com>
1 parent 5b87b89 commit 8d48308

3 files changed

Lines changed: 10 additions & 6 deletions

File tree

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -57,20 +57,20 @@ 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 AND anyApproval >= 2 → status: ready-to-merge (CODEOWNERS + min reviews)
60+
* maintainerApproval >= 1 AND (maintainerApproval + writeApproval) >= 2 → status: ready-to-merge (CODEOWNERS + min core reviews)
6161
* writeApproval >= 1 OR maintainerApproval >= 1 → queue:maintainers (senior review present, needs more)
6262
* anyApproval >= 1 → queue:committers (has any approval, needs committer)
6363
* else → queue:junior-committer (no approvals yet)
6464
*
6565
* Note: status: 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 status: ready-to-merge when branch protection requires 2+ reviews.
66+
* total core reviews (maintainer or write). This prevents a single maintainer approval
67+
* + a soft approval from marking a PR as ready when branch protection requires 2+ core reviews.
6868
*
6969
* @param {{ maintainerApproval: number, writeApproval: number, softApproval: number, anyApproval: number }} approvals
7070
* @returns {object} The correct QUEUE_LABELS entry
7171
*/
7272
function determineLabel(approvals) {
73-
if (approvals.maintainerApproval >= 1 && approvals.anyApproval >= 2) {
73+
if (approvals.maintainerApproval >= 1 && (approvals.maintainerApproval + approvals.writeApproval) >= 2) {
7474
return QUEUE_LABELS.MERGE;
7575
}
7676
if (approvals.writeApproval >= 1 || approvals.maintainerApproval >= 1) {

.github/scripts/review-sync/index.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,4 +88,8 @@ module.exports = async ({ github, context, core }) => {
8888
console.log(` Labels changed: ${changed}`);
8989
console.log(` Labels already correct: ${skipped}`);
9090
console.log(` Errors: ${errors}`);
91+
92+
if (errors > 0) {
93+
process.exitCode = 1;
94+
}
9195
};

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,10 +54,10 @@ const unitTests = [
5454
},
5555
},
5656
{
57-
name: 'determineLabel: 1 maintainer + 1 soft → status: ready-to-merge (2 reviews satisfied)',
57+
name: 'determineLabel: 1 maintainer + 1 soft → queue:maintainers (soft approvals do not count as core review)',
5858
test: () => {
5959
const r = determineLabel({ maintainerApproval: 1, writeApproval: 0, softApproval: 1, anyApproval: 2 });
60-
return r.name === 'status: ready-to-merge';
60+
return r.name === 'queue:maintainers';
6161
},
6262
},
6363
{

0 commit comments

Comments
 (0)