fix: Harden review-sync pipeline and resolve reviewer feedback (hiero-ledger#2262)

Signed-off-by: darshit2308 <darshit2308@gmail.com>

Author: darshit2308

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

@@ -35,11 +35,20 @@ const QUEUE_LABELS = {
   MERGE: {
     name: 'status: ready-to-merge',
     color: '0e8a16',
-    description: 'PR has 1+ maintainer and 2+ total approvals, ready to merge',
+    description: 'PR has 1+ maintainer and 2+ core (committer/maintainer) approvals, ready to merge',
   },
 };
 
 /** All queue label names, used for cleanup operations. */
 const ALL_QUEUE_LABEL_NAMES = Object.values(QUEUE_LABELS).map((l) => l.name);
 
-module.exports = { RATE_LIMIT_FLOOR, QUEUE_LABELS, ALL_QUEUE_LABEL_NAMES };
+/**
+ * The permanent community review label appended to all open PRs.
+ */
+const COMMUNITY_REVIEW = {
+  name: 'open to community review',
+  color: '008672',
+  description: 'PR is open for community review and feedback',
+};
+
+module.exports = { RATE_LIMIT_FLOOR, QUEUE_LABELS, ALL_QUEUE_LABEL_NAMES, COMMUNITY_REVIEW };

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

@@ -10,7 +10,7 @@
 //   - syncLabel() adds the correct label FIRST, then removes stale ones
 //     (crash-safe: PR never has zero queue labels)
 
-const { QUEUE_LABELS, ALL_QUEUE_LABEL_NAMES } = require('./constants');
+const { QUEUE_LABELS, ALL_QUEUE_LABEL_NAMES, COMMUNITY_REVIEW } = require('./constants');
 const { countApprovals } = require('./permissions');
 
 /**
@@ -53,27 +53,82 @@ async function ensureLabel(github, owner, repo, label, dryRun) {
   }
 }
 
+/**
+ * Check if the latest CI runs for a given commit have any failures.
+ *
+ * We intentionally treat the following as failures:
+ * - 'failure', 'timed_out' (explicit test failures)
+ * - 'startup_failure' (e.g., invalid workflow YAML)
+ * - 'action_required' (e.g., waiting for maintainer approval for first-time contributors)
+ *
+ * We intentionally EXCLUDE 'cancelled':
+ * When a developer pushes a new commit, GitHub automatically cancels the currently
+ * running workflows. If we treated 'cancelled' as a failure, every re-push would
+ * instantly demote the PR to queue:junior-committer, frustrating contributors.
+ *
+ * @returns {boolean} true if any check run conclusion is a blocking failure.
+ */
+async function hasCIFailures(github, owner, repo, sha) {
+  try {
+    // We MUST use paginate, otherwise it silently truncates at 30 runs.
+    // Matrix builds often exceed 30 checks.
+    const checkRuns = await github.paginate(github.rest.checks.listForRef, {
+      owner,
+      repo,
+      ref: sha,
+      filter: 'latest'
+    });
+
+    return checkRuns.some(
+      run =>
+        run.conclusion === 'failure' ||
+        run.conclusion === 'timed_out' ||
+        run.conclusion === 'startup_failure' ||
+        run.conclusion === 'action_required'
+    );
+  } catch (error) {
+    // Fail securely: do not assume CI is passing if we cannot verify it.
+    // Throwing ensures this PR skips sync and the workflow registers an error.
+    const message = error instanceof Error ? error.message : String(error);
+    console.error(`    ✗ Failed to fetch CI checks for ${sha}: ${message}`);
+    throw error;
+  }
+}
+
 /**
  * Determine the correct queue label for a PR based on approval counts.
  *
- * Phase 1 logic (4-stage pipeline):
- *   maintainerApproval >= 1 AND (maintainerApproval + writeApproval) >= 2  → status: ready-to-merge      (CODEOWNERS + min core reviews)
- *   writeApproval >= 1 OR maintainerApproval >= 1 → queue:maintainers   (senior review present, needs more)
- *   anyApproval >= 1                               → queue:committers    (has any approval, needs committer)
- *   else                                           → queue:junior-committer (no approvals yet)
+ * Phase 1 logic (5-stage pipeline):
+ *   ciFailing                                        → queue:junior-committer (CI broken, block promotion)
+ *   maintainerApprovals >= 1 AND coreApprovals >= 2  → status: ready-to-merge (CODEOWNERS + min core reviews)
+ *   maintainerApprovals >= 1 (but coreApprovals < 2) → queue:committers      (maintainer already in, need committer next)
+ *   coreApprovals >= 1 (no maintainer yet)            → queue:maintainers     (committer reviewed, needs maintainer)
+ *   anyApproval >= 1                                 → queue:committers      (has soft approval, needs committer)
+ *   else                                             → queue:junior-committer (no approvals yet)
  *
- * Note: status: ready-to-merge requires BOTH a maintainer approval AND at least 2
- * total core reviews (maintainer or write). This prevents a single maintainer approval
+ * Note: When a maintainer "jumps in early" before triage/committers, the PR should
+ * route to queue:committers (not queue:maintainers) to attract a committer review
+ * rather than a second maintainer. This prevents wasting maintainer bandwidth.
+ *
+ * status: ready-to-merge requires BOTH a maintainer approval AND at least 2
+ * total core reviews (maintainer or committer). This prevents a single maintainer approval
  * + a soft approval from marking a PR as ready when branch protection requires 2+ core reviews.
  *
- * @param {{ maintainerApproval: number, writeApproval: number, softApproval: number, anyApproval: number }} approvals
+ * @param {{ maintainerApprovals: number, coreApprovals: number, softApprovals: number, anyApproval: number }} approvals
+ * @param {boolean} ciFailing - If true, automatically demotes PR to queue:junior-committer
  * @returns {object} The correct QUEUE_LABELS entry
  */
-function determineLabel(approvals) {
-  if (approvals.maintainerApproval >= 1 && (approvals.maintainerApproval + approvals.writeApproval) >= 2) {
+function determineLabel(approvals, ciFailing = false) {
+  if (ciFailing) {
+    return QUEUE_LABELS.JUNIOR;
+  }
+  if (approvals.maintainerApprovals >= 1 && approvals.coreApprovals >= 2) {
     return QUEUE_LABELS.MERGE;
   }
-  if (approvals.writeApproval >= 1 || approvals.maintainerApproval >= 1) {
+  if (approvals.maintainerApprovals >= 1) {
+    return QUEUE_LABELS.COMMITTERS;
+  }
+  if (approvals.coreApprovals >= 1) {
     return QUEUE_LABELS.MAINTAINERS;
   }
   if (approvals.anyApproval >= 1) {
@@ -106,44 +161,60 @@ async function syncLabel(github, owner, repo, pr, dryRun) {
   const prNumber = pr.number;
   const currentLabels = (pr.labels || []).map((l) => l.name);
 
-  // Count approvals and determine the correct label
+  // Count approvals and check CI status
   const approvals = await countApprovals(github, owner, repo, prNumber);
-  const correctLabel = determineLabel(approvals);
+  const ciFailing = await hasCIFailures(github, owner, repo, pr.head.sha);
+  const correctLabel = determineLabel(approvals, ciFailing);
 
   console.log(
-    `  PR #${prNumber}: maintainerApproval=${approvals.maintainerApproval}, ` +
-    `writeApproval=${approvals.writeApproval}, ` +
-    `softApproval=${approvals.softApproval}, anyApproval=${approvals.anyApproval} ` +
-    `→ ${correctLabel.name}`
+    `  PR #${prNumber}: maintainerApprovals=${approvals.maintainerApprovals}, ` +
+    `coreApprovals=${approvals.coreApprovals}, ` +
+    `softApprovals=${approvals.softApprovals}, anyApproval=${approvals.anyApproval}, ` +
+    `ciFailing=${ciFailing} → ${correctLabel.name}`
   );
 
   // Determine which stale queue labels to remove
   const staleLabels = currentLabels.filter(
     (name) => ALL_QUEUE_LABEL_NAMES.includes(name) && name !== correctLabel.name
   );
 
-  // Check if the correct label is already present AND there are no stale labels to remove
-  if (currentLabels.includes(correctLabel.name) && staleLabels.length === 0) {
-    console.log(`    ✓ Already has "${correctLabel.name}". No change needed.`);
+  const isHuman = pr.user && pr.user.type !== 'Bot';
+  const needsCommunityReview = isHuman && !currentLabels.includes(COMMUNITY_REVIEW.name);
+
+  // Check if the correct labels are already present AND there are no stale labels to remove
+  if (currentLabels.includes(correctLabel.name) && staleLabels.length === 0 && !needsCommunityReview) {
+    console.log(`    ✓ Already has "${correctLabel.name}"${isHuman ? ` and "${COMMUNITY_REVIEW.name}"` : ''}. No change needed.`);
     return false;
   }
 
+  const labelsToAdd = [];
+  if (!currentLabels.includes(correctLabel.name)) {
+    labelsToAdd.push(correctLabel.name);
+  }
+  if (needsCommunityReview) {
+    labelsToAdd.push(COMMUNITY_REVIEW.name);
+  }
+
   if (dryRun) {
-    console.log(`    [DRY RUN] Would add "${correctLabel.name}".`);
+    if (labelsToAdd.length > 0) {
+      console.log(`    [DRY RUN] Would add: ${labelsToAdd.join(', ')}.`);
+    }
     if (staleLabels.length > 0) {
       console.log(`    [DRY RUN] Would remove: ${staleLabels.join(', ')}.`);
     }
     return true;
   }
 
-  // Step 1: ADD the correct label FIRST (crash-safe: PR always has at least one label)
-  await github.rest.issues.addLabels({
-    owner,
-    repo,
-    issue_number: prNumber,
-    labels: [correctLabel.name],
-  });
-  console.log(`    + Added "${correctLabel.name}".`);
+  // Step 1: ADD the correct labels FIRST (crash-safe: PR always has at least one label)
+  if (labelsToAdd.length > 0) {
+    await github.rest.issues.addLabels({
+      owner,
+      repo,
+      issue_number: prNumber,
+      labels: labelsToAdd,
+    });
+    console.log(`    + Added: ${labelsToAdd.join(', ')}.`);
+  }
 
   // Step 2: THEN remove stale queue labels one by one
   for (const stale of staleLabels) {
@@ -170,4 +241,4 @@ async function syncLabel(github, owner, repo, pr, dryRun) {
   return true;
 }
 
-module.exports = { ensureLabel, determineLabel, syncLabel };
+module.exports = { ensureLabel, determineLabel, syncLabel, hasCIFailures };

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

@@ -11,18 +11,21 @@
 //
 //   We MUST use role_name to distinguish maintainers from committers.
 //   If we used permission, Sophie (Maintain role per MAINTAINERS.md) would
-//   appear as 'write', and maintainerApproval would always be 0.
+//   appear as 'write', and maintainerApprovals would always be 0.
 //   PRs would be permanently stuck at queue:maintainers.
 //
 //   Phase 3 will replace this with team membership checks
 //   (getMembershipForUserInOrg) for full accuracy.
 
 const { getLatestReviewStates } = require('./reviews');
 
+const permissionCache = new Map();
+
 /**
  * Check the repository role for a given user.
  *
  * Uses role_name (not legacy permission) to correctly detect the maintain role.
+ * Results are cached in memory to avoid redundant API calls during the cron run.
  *
  * @param {object} github   - Octokit instance
  * @param {string} owner    - Repository owner
@@ -31,6 +34,12 @@ const { getLatestReviewStates } = require('./reviews');
  * @returns {string} 'admin' | 'maintain' | 'write' | 'triage' | 'read' | 'none'
  */
 async function getPermissionLevel(github, owner, repo, username) {
+  const cacheKey = `${owner}/${repo}/${username}`;
+
+  if (permissionCache.has(cacheKey)) {
+    return permissionCache.get(cacheKey);
+  }
+
   try {
     const { data } = await github.rest.repos.getCollaboratorPermissionLevel({
       owner,
@@ -40,10 +49,13 @@ async function getPermissionLevel(github, owner, repo, username) {
     // CRITICAL: Use role_name, NOT permission.
     // The legacy 'permission' field maps maintain → write, triage → read.
     // role_name correctly returns: admin | maintain | write | triage | read
-    return data.role_name || data.permission || 'none';
+    const role = data.role_name || data.permission || 'none';
+    permissionCache.set(cacheKey, role);
+    return role;
   } catch (error) {
     if (error.status === 404) {
       // External contributor — not a collaborator
+      permissionCache.set(cacheKey, 'none');
       return 'none';
     }
     // Log unexpected errors but don't crash the run
@@ -57,44 +69,49 @@ async function getPermissionLevel(github, owner, repo, username) {
  * Count approvals on a PR, split by permission level.
  *
  * Returns three counters:
- *   - maintainerApproval: admin or maintain (maps to CODEOWNERS maintainer teams)
- *   - writeApproval: write (committers)
- *   - softApproval: triage, read, none, external contributors
+ *   - maintainerApprovals: admin or maintain (maps to CODEOWNERS maintainer teams)
+ *   - coreApprovals: write (committers) and maintainers
+ *   - softApprovals: triage, read, none, external contributors
  *
  * @param {object} github   - Octokit instance
  * @param {string} owner    - Repository owner
  * @param {string} repo     - Repository name
  * @param {number} prNumber - Pull request number
- * @returns {{ maintainerApproval: number, writeApproval: number, softApproval: number, anyApproval: number }}
+ * @param {{ maintainerApprovals: number, coreApprovals: number, softApprovals: number, anyApproval: number }}
  */
 async function countApprovals(github, owner, repo, prNumber) {
   const latestStates = await getLatestReviewStates(github, owner, repo, prNumber);
 
-  let maintainerApproval = 0;
-  let writeApproval = 0;
-  let softApproval = 0;
+  let maintainerApprovals = 0; // only maintainers
+  let coreApprovals = 0; // anyone with write permission
+  let softApprovals = 0;
 
   for (const [username, state] of latestStates) {
     if (state !== 'APPROVED') continue;
 
     const role = await getPermissionLevel(github, owner, repo, username);
 
     if (role === 'admin' || role === 'maintain') {
-      maintainerApproval++;
+      maintainerApprovals++;
+      coreApprovals++; // Maintainers also count as core
     } else if (role === 'write') {
-      writeApproval++;
+      coreApprovals++; // Committers count as core
     } else {
       // triage, read, none, or any unexpected value → soft approval
-      softApproval++;
+      softApprovals++;
     }
   }
 
   return {
-    maintainerApproval,
-    writeApproval,
-    softApproval,
-    anyApproval: maintainerApproval + writeApproval + softApproval,
+    maintainerApprovals,
+    coreApprovals,
+    softApprovals,
+    anyApproval: coreApprovals + softApprovals,
   };
 }
 
-module.exports = { getPermissionLevel, countApprovals };
+function clearPermissionCache() {
+  permissionCache.clear();
+}
+
+module.exports = { getPermissionLevel, countApprovals, clearPermissionCache };

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

@@ -14,7 +14,7 @@
 // Phase 1 of 4 — label sync only.
 
 const helpers = require('./helpers');
-const { QUEUE_LABELS, RATE_LIMIT_FLOOR } = helpers.constants;
+const { QUEUE_LABELS, RATE_LIMIT_FLOOR, COMMUNITY_REVIEW } = helpers.constants;
 const { ensureLabel, syncLabel } = helpers.labels;
 
 module.exports = async ({ github, context, core }) => {
@@ -55,11 +55,12 @@ module.exports = async ({ github, context, core }) => {
     return;
   }
 
-  // ── 3. Ensure queue labels exist ─────────────────────────────────────────
-  console.log('\n--- Ensuring Queue Labels Exist ---');
+  // ── 3. Ensure labels exist ───────────────────────────────────────────────
+  console.log('\n--- Ensuring Labels Exist ---');
   for (const label of Object.values(QUEUE_LABELS)) {
     await ensureLabel(github, owner, repo, label, dryRun);
   }
+  await ensureLabel(github, owner, repo, COMMUNITY_REVIEW, dryRun);
 
   // ── 4. Sync label on each PR ─────────────────────────────────────────────
   console.log('\n--- Syncing Labels ---');
@@ -90,6 +91,6 @@ module.exports = async ({ github, context, core }) => {
   console.log(`  Errors: ${errors}`);
 
   if (errors > 0) {
-    process.exitCode = 1;
+    core.setFailed(`Review sync completed with ${errors} error(s). Check logs above.`);
   }
 };

.github/scripts/review-sync/package.json

@@ -0,0 +1,10 @@
+{
+  "name": "review-sync",
+  "version": "1.0.0",
+  "description": "Automated PR review queue label sync scripts and tests",
+  "private": true,
+  "main": "index.js",
+  "scripts": {
+    "test": "node tests/test-labels.js && node tests/test-permissions.js && node tests/test-reviews.js"
+  }
+}