From 6e67c499bfc1529e964ac6f0a43963eb15e8b40a Mon Sep 17 00:00:00 2001 From: darshit2308 Date: Wed, 6 May 2026 10:32:52 +0530 Subject: [PATCH 01/10] feat: add Phase 1 review queue label sync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce a cron-based workflow (every 30 min) that classifies every open non-draft PR into one of three review stages based on the number and permission level of approvals: queue:junior-committer → queue:committers → ready-to-merge Phase 1 of 4 — label sync only. No assignments, comments, or routing. Label determination: - writeApproval >= 2 → ready-to-merge - anyApproval >= 1 → queue:committers - else → queue:junior-committer Key design decisions: - Add-first-then-remove label ordering prevents zero-label state - DISMISSED reviews actively delete prior state (prevents ghost approvals) - Reviews explicitly sorted by submitted_at (correct by construction) - Rate-limit guard (floor=200) prevents partial runs - Concurrency block prevents overlapping cron runs from racing - DRY_RUN mode for safe manual testing via workflow_dispatch - 422/404 errors handled silently (race conditions, external users) - Old step:/reviewer: labels coexist intentionally until future cleanup - Bot-authored PRs (Dependabot) receive labels (acknowledged) Files added: .github/workflows/review-sync.yml .github/scripts/review-sync/index.js .github/scripts/review-sync/labels.js Signed-off-by: darshit2308 --- .github/scripts/review-sync/index.js | 129 +++++++++++++ .github/scripts/review-sync/labels.js | 264 ++++++++++++++++++++++++++ .github/workflows/review-sync.yml | 58 ++++++ 3 files changed, 451 insertions(+) create mode 100644 .github/scripts/review-sync/index.js create mode 100644 .github/scripts/review-sync/labels.js create mode 100644 .github/workflows/review-sync.yml diff --git a/.github/scripts/review-sync/index.js b/.github/scripts/review-sync/index.js new file mode 100644 index 000000000..cb22d9463 --- /dev/null +++ b/.github/scripts/review-sync/index.js @@ -0,0 +1,129 @@ +// .github/scripts/review-sync/index.js +// +// Entry point for the Review Queue Label Sync cron job. +// +// Responsibilities: +// 1. Rate-limit guard — abort if remaining calls < 200 +// 2. Fetch all open non-draft PRs (paginated) +// 3. Ensure the three queue labels exist in the repo +// 4. Sync the correct label on every PR via labels.js +// 5. Print a summary of what changed +// +// Phase 1 of 4 — label sync only. + +const { QUEUE_LABELS, syncLabel } = require('./labels.js'); + +const RATE_LIMIT_FLOOR = 200; + +/** + * Ensure a single label exists in the repo. + * Silently handles 422 (label already exists). + * + * Note: checks existence only. If a label already exists with the wrong + * colour or description, it will not be corrected in Phase 1. + */ +async function ensureLabel(github, owner, repo, label, dryRun) { + try { + await github.rest.issues.getLabel({ owner, repo, name: label.name }); + console.log(` Label "${label.name}" already exists. Skipping creation.`); + } catch (error) { + if (error.status === 404) { + if (dryRun) { + console.log(` [DRY RUN] Would create label "${label.name}" (${label.color}).`); + return; + } + try { + await github.rest.issues.createLabel({ + owner, + repo, + name: label.name, + color: label.color, + description: label.description, + }); + console.log(` Created label "${label.name}" (#${label.color}).`); + } catch (createError) { + // 422 = label already exists (race condition or concurrent run) + if (createError.status === 422) { + console.log(` Label "${label.name}" already exists (422). Skipping.`); + } else { + throw createError; + } + } + } else { + throw error; + } + } +} + +module.exports = async ({ github, context, core }) => { + const dryRun = (process.env.DRY_RUN || 'false').toLowerCase() === 'true'; + const { owner, repo } = context.repo; + + if (dryRun) { + console.log('=== DRY RUN MODE — no labels will be created or modified ===\n'); + } + + // ── 1. Rate-limit guard ────────────────────────────────────────────────── + console.log('--- Rate Limit Check ---'); + const { data: rateLimit } = await github.rest.rateLimit.get(); + const remaining = rateLimit.resources.core.remaining; + console.log(` Core API remaining: ${remaining}`); + + if (remaining < RATE_LIMIT_FLOOR) { + console.log(` ⚠ Skipping run: rate limit too low (${remaining} < ${RATE_LIMIT_FLOOR}).`); + return; + } + + // ── 2. Fetch all open non-draft PRs (paginated) ────────────────────────── + console.log('\n--- Fetching Open PRs ---'); + const allPRs = await github.paginate(github.rest.pulls.list, { + owner, + repo, + state: 'open', + per_page: 100, + }); + + const prs = allPRs.filter((pr) => !pr.draft); + console.log(` Total open PRs: ${allPRs.length}`); + console.log(` Non-draft PRs to process: ${prs.length}`); + console.log(` Draft PRs skipped: ${allPRs.length - prs.length}`); + + if (prs.length === 0) { + console.log(' No non-draft PRs found. Exiting.'); + return; + } + + // ── 3. Ensure queue labels exist ───────────────────────────────────────── + console.log('\n--- Ensuring Queue Labels Exist ---'); + for (const label of Object.values(QUEUE_LABELS)) { + await ensureLabel(github, owner, repo, label, dryRun); + } + + // ── 4. Sync label on each PR ───────────────────────────────────────────── + console.log('\n--- Syncing Labels ---'); + let changed = 0; + let skipped = 0; + let errors = 0; + + for (const pr of prs) { + try { + const didChange = await syncLabel(github, owner, repo, pr, dryRun); + if (didChange) { + changed++; + } else { + skipped++; + } + } catch (error) { + errors++; + const message = error instanceof Error ? error.message : String(error); + console.error(` ✗ Error on PR #${pr.number}: ${message}`); + } + } + + // ── 5. Summary ─────────────────────────────────────────────────────────── + console.log('\n=== Summary ==='); + console.log(` PRs processed: ${prs.length}`); + console.log(` Labels changed: ${changed}`); + console.log(` Labels already correct: ${skipped}`); + console.log(` Errors: ${errors}`); +}; diff --git a/.github/scripts/review-sync/labels.js b/.github/scripts/review-sync/labels.js new file mode 100644 index 000000000..7c9be6773 --- /dev/null +++ b/.github/scripts/review-sync/labels.js @@ -0,0 +1,264 @@ +// .github/scripts/review-sync/labels.js +// +// Core label determination and application for the Review Queue Sync. +// +// Logic (Phase 1 — simplified, no difficulty routing): +// 1. Fetch all reviews on a PR (paginated, sorted by submitted_at) +// 2. Build username → latest review state map +// 3. Ignore COMMENTED; DISMISSED actively deletes prior state (prevents ghost approvals) +// 4. For each APPROVED reviewer, check permission level +// 5. Determine correct queue label based on approval counts +// 6. Apply label: add new FIRST, then remove stale (non-negotiable order) +// +// Phase 2 will add difficulty-based routing on top of this logic. + +/** + * The three mutually exclusive queue labels managed by this bot. + * Colours and names match the Phase 1 specification. + */ +const QUEUE_LABELS = { + JUNIOR: { + name: 'queue:junior-committer', + color: 'e4e669', + description: 'PR awaiting initial quality review', + }, + COMMITTERS: { + name: 'queue:committers', + color: '0075ca', + description: 'PR awaiting committer technical review', + }, + MERGE: { + name: 'ready-to-merge', + color: '0e8a16', + description: 'PR has 2+ write approvals and is ready to merge', + }, +}; + +/** All queue label names, used for cleanup operations. */ +const ALL_QUEUE_LABEL_NAMES = Object.values(QUEUE_LABELS).map((l) => l.name); + +/** + * Fetch all reviews on a PR, returning only the latest state per reviewer. + * COMMENTED reviews are ignored. DISMISSED reviews actively delete prior state + * to prevent ghost approvals (e.g. when GitHub auto-dismisses stale reviews + * after new commits are pushed). + * + * @param {object} github - Octokit instance + * @param {string} owner - Repository owner + * @param {string} repo - Repository name + * @param {number} prNumber - Pull request number + * @returns {Map} username → latest review state (APPROVED | CHANGES_REQUESTED) + */ +async function getLatestReviewStates(github, owner, repo, prNumber) { + const reviews = await github.paginate(github.rest.pulls.listReviews, { + owner, + repo, + pull_number: prNumber, + per_page: 100, + }); + + // Sort explicitly by submitted_at to guarantee chronological order. + // The GitHub API returns reviews chronologically in practice, but + // explicit sorting makes this correct by construction. + const sortedReviews = [...reviews].sort( + (a, b) => new Date(a.submitted_at) - new Date(b.submitted_at) + ); + + // Build a map keyed by reviewer login. + // Later entries overwrite earlier ones — giving us the latest state per user. + const latestByUser = new Map(); + + for (const review of sortedReviews) { + const login = review.user?.login; + const state = review.state?.toUpperCase(); + + if (!login || !state) continue; + + if (state === 'APPROVED' || state === 'CHANGES_REQUESTED') { + latestByUser.set(login, state); + } else if (state === 'DISMISSED') { + // CRITICAL: A dismissed review wipes out the user's prior approval. + // Without this, a stale review that GitHub auto-dismissed (e.g. after + // new commits) would persist as a ghost approval in the map. + latestByUser.delete(login); + } + // COMMENTED is the only state intentionally ignored + } + + return latestByUser; +} + +/** + * Check the repository permission level for a given user. + * + * @param {object} github - Octokit instance + * @param {string} owner - Repository owner + * @param {string} repo - Repository name + * @param {string} username - GitHub username + * @returns {string} 'admin' | 'write' | 'read' | 'none' + */ +async function getPermissionLevel(github, owner, repo, username) { + try { + const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ + owner, + repo, + username, + }); + return data.permission || 'none'; + } catch (error) { + if (error.status === 404) { + // External contributor — not a collaborator + return 'none'; + } + // Log unexpected errors but don't crash the run + const message = error instanceof Error ? error.message : String(error); + console.log(` ⚠ Permission check failed for ${username}: ${message}. Treating as "none".`); + return 'none'; + } +} + +/** + * Count approvals on a PR, split by permission level. + * + * @param {object} github - Octokit instance + * @param {string} owner - Repository owner + * @param {string} repo - Repository name + * @param {number} prNumber - Pull request number + * @returns {{ writeApproval: number, softApproval: number, anyApproval: number }} + */ +async function countApprovals(github, owner, repo, prNumber) { + const latestStates = await getLatestReviewStates(github, owner, repo, prNumber); + + let writeApproval = 0; + let softApproval = 0; + + for (const [username, state] of latestStates) { + if (state !== 'APPROVED') continue; + + const permission = await getPermissionLevel(github, owner, repo, username); + + if (permission === 'admin' || permission === 'write') { + writeApproval++; + } else { + // read, none, or any unexpected value → soft approval + softApproval++; + } + } + + return { + writeApproval, + softApproval, + anyApproval: writeApproval + softApproval, + }; +} + +/** + * Determine the correct queue label for a PR based on approval counts. + * + * Phase 1 logic (no difficulty routing): + * writeApproval >= 2 → ready-to-merge + * anyApproval >= 1 → queue:committers + * else → queue:junior-committer + * + * @param {{ writeApproval: number, anyApproval: number }} approvals + * @returns {object} The correct QUEUE_LABELS entry + */ +function determineLabel(approvals) { + if (approvals.writeApproval >= 2) { + return QUEUE_LABELS.MERGE; + } + if (approvals.anyApproval >= 1) { + return QUEUE_LABELS.COMMITTERS; + } + return QUEUE_LABELS.JUNIOR; +} + +/** + * Sync the queue label on a single PR. + * + * Order of operations (non-negotiable): + * 1. Check if the correct label is already present → skip if yes + * 2. ADD the correct label first + * 3. THEN remove any stale queue labels + * + * This ensures a PR never has zero queue labels, even if the process + * crashes mid-run. + * + * @param {object} github - Octokit instance + * @param {string} owner - Repository owner + * @param {string} repo - Repository name + * @param {object} pr - Pull request object from the list API + * @param {boolean} dryRun - If true, log without making changes + * @returns {boolean} true if the label was changed, false if already correct + */ +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 + const approvals = await countApprovals(github, owner, repo, prNumber); + const correctLabel = determineLabel(approvals); + + console.log( + ` PR #${prNumber}: writeApproval=${approvals.writeApproval}, ` + + `softApproval=${approvals.softApproval}, anyApproval=${approvals.anyApproval} ` + + `→ ${correctLabel.name}` + ); + + // Check if the correct label is already present + if (currentLabels.includes(correctLabel.name)) { + console.log(` ✓ Already has "${correctLabel.name}". No change needed.`); + return false; + } + + // Determine which stale queue labels to remove + const staleLabels = currentLabels.filter( + (name) => ALL_QUEUE_LABEL_NAMES.includes(name) && name !== correctLabel.name + ); + + if (dryRun) { + console.log(` [DRY RUN] Would add "${correctLabel.name}".`); + 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 2: THEN remove stale queue labels one by one + for (const stale of staleLabels) { + try { + await github.rest.issues.removeLabel({ + owner, + repo, + issue_number: prNumber, + name: stale, + }); + console.log(` - Removed "${stale}".`); + } catch (error) { + // 404 = label was already removed (race condition or manual action) + if (error.status === 404) { + console.log(` - Label "${stale}" already gone (404). Skipping.`); + } else { + const message = error instanceof Error ? error.message : String(error); + console.error(` ✗ Failed to remove "${stale}": ${message}`); + } + } + } + + return true; +} + +module.exports = { + QUEUE_LABELS, + ALL_QUEUE_LABEL_NAMES, + syncLabel, +}; diff --git a/.github/workflows/review-sync.yml b/.github/workflows/review-sync.yml new file mode 100644 index 000000000..76647acd6 --- /dev/null +++ b/.github/workflows/review-sync.yml @@ -0,0 +1,58 @@ +# Automated PR review queue label sync. +# Runs on a 30-minute schedule to classify every open non-draft PR into one of +# three review stages based on the number and permission level of approvals: +# queue:junior-committer → queue:committers → ready-to-merge +# +# Phase 1 of 4 — label sync only. No assignments, no comments, no routing. +# +# Old step:/reviewer: labels are intentionally left untouched and will coexist +# with the new queue: labels until a future cleanup PR removes them. +# Bot-authored PRs (e.g. Dependabot) are included and will receive labels. + +name: Review Queue Label Sync + +on: + schedule: + - cron: '*/30 * * * *' + workflow_dispatch: + inputs: + dry_run: + description: 'If true, log label changes without applying them. Default true for manual runs.' + required: false + default: 'true' + type: string + +permissions: + pull-requests: write + issues: write + contents: read + +concurrency: + group: ${{ github.workflow }} + cancel-in-progress: true + +jobs: + sync: + runs-on: hl-sdk-py-lin-md + env: + DRY_RUN: ${{ inputs.dry_run || 'false' }} + + steps: + - name: Harden the runner (Audit all outbound calls) + uses: step-security/harden-runner@8d3c67de8e2fe68ef647c8db1e6a09f647780f40 # v2.19.0 + with: + egress-policy: audit + + - name: Checkout scripts + uses: actions/checkout@8e8c483db84b4bee98b60c0593521ed34d9990e8 # v6.0.1 + with: + sparse-checkout: | + .github/scripts + sparse-checkout-cone-mode: false + + - name: Run review sync + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 + with: + script: | + const fn = require('./.github/scripts/review-sync/index.js'); + await fn({ github, context, core }); From 222e06bc24ac32135f73d5481082b53a358ec3c2 Mon Sep 17 00:00:00 2001 From: darshit2308 Date: Wed, 6 May 2026 11:08:33 +0530 Subject: [PATCH 02/10] fix: address Copilot code review feedback - Move early return condition to after stale label determination to ensure stale labels are always removed - Add 'maintain' role to privileged approval permission levels - Downgrade pull-requests permission to 'read' to enforce least privilege Signed-off-by: darshit2308 --- .github/scripts/review-sync/labels.js | 14 +++++++------- .github/workflows/review-sync.yml | 2 +- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.github/scripts/review-sync/labels.js b/.github/scripts/review-sync/labels.js index 7c9be6773..7733fae3d 100644 --- a/.github/scripts/review-sync/labels.js +++ b/.github/scripts/review-sync/labels.js @@ -137,7 +137,7 @@ async function countApprovals(github, owner, repo, prNumber) { const permission = await getPermissionLevel(github, owner, repo, username); - if (permission === 'admin' || permission === 'write') { + if (permission === 'admin' || permission === 'maintain' || permission === 'write') { writeApproval++; } else { // read, none, or any unexpected value → soft approval @@ -205,17 +205,17 @@ async function syncLabel(github, owner, repo, pr, dryRun) { `→ ${correctLabel.name}` ); - // Check if the correct label is already present - if (currentLabels.includes(correctLabel.name)) { - console.log(` ✓ Already has "${correctLabel.name}". No change needed.`); - return false; - } - // 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.`); + return false; + } + if (dryRun) { console.log(` [DRY RUN] Would add "${correctLabel.name}".`); if (staleLabels.length > 0) { diff --git a/.github/workflows/review-sync.yml b/.github/workflows/review-sync.yml index 76647acd6..46f80c21d 100644 --- a/.github/workflows/review-sync.yml +++ b/.github/workflows/review-sync.yml @@ -23,7 +23,7 @@ on: type: string permissions: - pull-requests: write + pull-requests: read issues: write contents: read From 06fdf32d9fb8c58af42c9feacae2b6bdd6703498 Mon Sep 17 00:00:00 2001 From: darshit2308 Date: Wed, 6 May 2026 11:11:34 +0530 Subject: [PATCH 03/10] docs: update syncLabel JSDoc to match revised early-return logic Signed-off-by: darshit2308 --- .github/scripts/review-sync/labels.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/.github/scripts/review-sync/labels.js b/.github/scripts/review-sync/labels.js index 7733fae3d..1b64703c3 100644 --- a/.github/scripts/review-sync/labels.js +++ b/.github/scripts/review-sync/labels.js @@ -177,12 +177,14 @@ function determineLabel(approvals) { * Sync the queue label on a single PR. * * Order of operations (non-negotiable): - * 1. Check if the correct label is already present → skip if yes - * 2. ADD the correct label first - * 3. THEN remove any stale queue labels + * 1. Compute stale queue labels on the PR + * 2. Skip only if correct label is already present AND no stale labels exist + * 3. ADD the correct label first + * 4. THEN remove any stale queue labels * * This ensures a PR never has zero queue labels, even if the process - * crashes mid-run. + * crashes mid-run. Stale labels are always cleaned up, even when the + * correct label was already present. * * @param {object} github - Octokit instance * @param {string} owner - Repository owner From e0be0305aa09cce17388367b47788bbe2e85b8ce Mon Sep 17 00:00:00 2001 From: darshit2308 Date: Wed, 6 May 2026 21:07:36 +0530 Subject: [PATCH 04/10] feat: 4-stage review queue pipeline with maintainer gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses maintainer feedback on PR #2242: - Refactors monolithic labels.js into helpers/ structure (constants, permissions, reviews, labels) matching hiero-sdk-cpp pattern - Adds queue:maintainers label for a 4-stage pipeline: queue:junior-committer → queue:committers → queue:maintainers → ready-to-merge - Gates ready-to-merge on at least 1 maintainer (admin/maintain) approval to satisfy CODEOWNERS requirements - Uses role_name (not legacy permission field) to correctly detect maintain role — fixes critical bug where maintain maps to write - Adds comprehensive unit tests (37 tests) using inline mock pattern from hiero-sdk-cpp test-utils Signed-off-by: darshit2308 --- .../scripts/review-sync/helpers/constants.js | 45 +++ .github/scripts/review-sync/helpers/index.js | 18 ++ .github/scripts/review-sync/helpers/labels.js | 168 +++++++++++ .../review-sync/helpers/permissions.js | 100 +++++++ .../scripts/review-sync/helpers/reviews.js | 63 +++++ .github/scripts/review-sync/index.js | 52 +--- .github/scripts/review-sync/labels.js | 266 ------------------ .../scripts/review-sync/tests/test-labels.js | 151 ++++++++++ .../review-sync/tests/test-permissions.js | 236 ++++++++++++++++ .../scripts/review-sync/tests/test-reviews.js | 144 ++++++++++ .../scripts/review-sync/tests/test-utils.js | 162 +++++++++++ .github/workflows/review-sync.yml | 4 +- 12 files changed, 1096 insertions(+), 313 deletions(-) create mode 100644 .github/scripts/review-sync/helpers/constants.js create mode 100644 .github/scripts/review-sync/helpers/index.js create mode 100644 .github/scripts/review-sync/helpers/labels.js create mode 100644 .github/scripts/review-sync/helpers/permissions.js create mode 100644 .github/scripts/review-sync/helpers/reviews.js delete mode 100644 .github/scripts/review-sync/labels.js create mode 100644 .github/scripts/review-sync/tests/test-labels.js create mode 100644 .github/scripts/review-sync/tests/test-permissions.js create mode 100644 .github/scripts/review-sync/tests/test-reviews.js create mode 100644 .github/scripts/review-sync/tests/test-utils.js diff --git a/.github/scripts/review-sync/helpers/constants.js b/.github/scripts/review-sync/helpers/constants.js new file mode 100644 index 000000000..92c0c12e7 --- /dev/null +++ b/.github/scripts/review-sync/helpers/constants.js @@ -0,0 +1,45 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// helpers/constants.js +// +// Shared constants for the Review Queue Label Sync. + +/** + * Safety floor for the GitHub API rate limit. + * The cron job will skip processing if remaining calls fall below this. + */ +const RATE_LIMIT_FLOOR = 200; + +/** + * The four mutually exclusive queue labels managed by this bot. + * + * Flow: + * queue:junior-committer → queue:committers → queue:maintainers → ready-to-merge + */ +const QUEUE_LABELS = { + JUNIOR: { + name: 'queue:junior-committer', + color: 'e4e669', + description: 'PR awaiting initial quality review', + }, + COMMITTERS: { + name: 'queue:committers', + color: '0075ca', + description: 'PR awaiting committer technical review', + }, + MAINTAINERS: { + name: 'queue:maintainers', + color: 'd876e3', + description: 'PR awaiting maintainer final review', + }, + MERGE: { + name: 'ready-to-merge', + color: '0e8a16', + description: 'PR approved by maintainer and 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 }; diff --git a/.github/scripts/review-sync/helpers/index.js b/.github/scripts/review-sync/helpers/index.js new file mode 100644 index 000000000..22ef9ee28 --- /dev/null +++ b/.github/scripts/review-sync/helpers/index.js @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// helpers/index.js +// +// Namespaced barrel export for review-sync helpers. +// Uses namespacing to prevent silent name collisions from spread exports. +// +// Usage: +// const helpers = require('./helpers'); +// const { QUEUE_LABELS } = helpers.constants; +// const { syncLabel } = helpers.labels; + +module.exports = { + constants: require('./constants'), + permissions: require('./permissions'), + reviews: require('./reviews'), + labels: require('./labels'), +}; diff --git a/.github/scripts/review-sync/helpers/labels.js b/.github/scripts/review-sync/helpers/labels.js new file mode 100644 index 000000000..9cb86fda2 --- /dev/null +++ b/.github/scripts/review-sync/helpers/labels.js @@ -0,0 +1,168 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// helpers/labels.js +// +// Label creation, determination, and synchronization. +// +// Key design decisions: +// - ensureLabel() silently handles 422 (race condition / concurrent run) +// - determineLabel() uses a 4-stage pipeline gated by maintainer approval +// - 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 { countApprovals } = require('./permissions'); + +/** + * Ensure a single label exists in the repo. + * Silently handles 422 (label already exists). + * + * Note: checks existence only. If a label already exists with the wrong + * colour or description, it will not be corrected in Phase 1. + */ +async function ensureLabel(github, owner, repo, label, dryRun) { + try { + await github.rest.issues.getLabel({ owner, repo, name: label.name }); + console.log(` Label "${label.name}" already exists. Skipping creation.`); + } catch (error) { + if (error.status === 404) { + if (dryRun) { + console.log(` [DRY RUN] Would create label "${label.name}" (${label.color}).`); + return; + } + try { + await github.rest.issues.createLabel({ + owner, + repo, + name: label.name, + color: label.color, + description: label.description, + }); + console.log(` Created label "${label.name}" (#${label.color}).`); + } catch (createError) { + // 422 = label already exists (race condition or concurrent run) + if (createError.status === 422) { + console.log(` Label "${label.name}" already exists (422). Skipping.`); + } else { + throw createError; + } + } + } else { + throw error; + } + } +} + +/** + * Determine the correct queue label for a PR based on approval counts. + * + * Phase 1 logic (4-stage pipeline): + * maintainerApproval >= 1 → ready-to-merge (CODEOWNERS satisfied) + * writeApproval >= 1 → queue:maintainers (committer approved, needs maintainer) + * anyApproval >= 1 → queue:committers (has any approval, needs committer) + * else → queue:junior-committer (no approvals yet) + * + * @param {{ maintainerApproval: number, writeApproval: number, softApproval: number, anyApproval: number }} approvals + * @returns {object} The correct QUEUE_LABELS entry + */ +function determineLabel(approvals) { + if (approvals.maintainerApproval >= 1) { + return QUEUE_LABELS.MERGE; + } + if (approvals.writeApproval >= 1) { + return QUEUE_LABELS.MAINTAINERS; + } + if (approvals.anyApproval >= 1) { + return QUEUE_LABELS.COMMITTERS; + } + return QUEUE_LABELS.JUNIOR; +} + +/** + * Sync the queue label on a single PR. + * + * Order of operations (non-negotiable): + * 1. Compute stale queue labels on the PR + * 2. Skip only if correct label is already present AND no stale labels exist + * 3. ADD the correct label first + * 4. THEN remove any stale queue labels + * + * This ensures a PR never has zero queue labels, even if the process + * crashes mid-run. Stale labels are always cleaned up, even when the + * correct label was already present. + * + * @param {object} github - Octokit instance + * @param {string} owner - Repository owner + * @param {string} repo - Repository name + * @param {object} pr - Pull request object from the list API + * @param {boolean} dryRun - If true, log without making changes + * @returns {boolean} true if the label was changed, false if already correct + */ +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 + const approvals = await countApprovals(github, owner, repo, prNumber); + const correctLabel = determineLabel(approvals); + + console.log( + ` PR #${prNumber}: maintainerApproval=${approvals.maintainerApproval}, ` + + `writeApproval=${approvals.writeApproval}, ` + + `softApproval=${approvals.softApproval}, anyApproval=${approvals.anyApproval} ` + + `→ ${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.`); + return false; + } + + if (dryRun) { + console.log(` [DRY RUN] Would add "${correctLabel.name}".`); + 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 2: THEN remove stale queue labels one by one + for (const stale of staleLabels) { + try { + await github.rest.issues.removeLabel({ + owner, + repo, + issue_number: prNumber, + name: stale, + }); + console.log(` - Removed "${stale}".`); + } catch (error) { + // 404 = label was already removed (race condition or manual action) + if (error.status === 404) { + console.log(` - Label "${stale}" already gone (404). Skipping.`); + } else { + const message = error instanceof Error ? error.message : String(error); + console.error(` ✗ Failed to remove "${stale}": ${message}`); + } + } + } + + return true; +} + +module.exports = { ensureLabel, determineLabel, syncLabel }; diff --git a/.github/scripts/review-sync/helpers/permissions.js b/.github/scripts/review-sync/helpers/permissions.js new file mode 100644 index 000000000..fd3dd970b --- /dev/null +++ b/.github/scripts/review-sync/helpers/permissions.js @@ -0,0 +1,100 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// helpers/permissions.js +// +// Permission level checks and approval counting. +// +// CRITICAL NOTE ON role_name vs permission: +// getCollaboratorPermissionLevel returns TWO fields: +// - permission (legacy): maps maintain → write, triage → read +// - role_name (accurate): returns admin | maintain | write | triage | read +// +// 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. +// 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'); + +/** + * Check the repository role for a given user. + * + * Uses role_name (not legacy permission) to correctly detect the maintain role. + * + * @param {object} github - Octokit instance + * @param {string} owner - Repository owner + * @param {string} repo - Repository name + * @param {string} username - GitHub username + * @returns {string} 'admin' | 'maintain' | 'write' | 'triage' | 'read' | 'none' + */ +async function getPermissionLevel(github, owner, repo, username) { + try { + const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ + 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'; + } catch (error) { + if (error.status === 404) { + // External contributor — not a collaborator + return 'none'; + } + // Log unexpected errors but don't crash the run + const message = error instanceof Error ? error.message : String(error); + console.log(` ⚠ Permission check failed for ${username}: ${message}. Treating as "none".`); + return 'none'; + } +} + +/** + * 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 + * + * @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 }} + */ +async function countApprovals(github, owner, repo, prNumber) { + const latestStates = await getLatestReviewStates(github, owner, repo, prNumber); + + let maintainerApproval = 0; + let writeApproval = 0; + let softApproval = 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++; + } else if (role === 'write') { + writeApproval++; + } else { + // triage, read, none, or any unexpected value → soft approval + softApproval++; + } + } + + return { + maintainerApproval, + writeApproval, + softApproval, + anyApproval: maintainerApproval + writeApproval + softApproval, + }; +} + +module.exports = { getPermissionLevel, countApprovals }; diff --git a/.github/scripts/review-sync/helpers/reviews.js b/.github/scripts/review-sync/helpers/reviews.js new file mode 100644 index 000000000..670a8ed91 --- /dev/null +++ b/.github/scripts/review-sync/helpers/reviews.js @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// helpers/reviews.js +// +// Fetches and processes PR review states. +// +// Key design decisions: +// - COMMENTED reviews are intentionally ignored (they carry no approval weight) +// - DISMISSED reviews actively delete prior state to prevent ghost approvals +// - Explicit chronological sort guarantees correctness regardless of API order + +/** + * Fetch all reviews on a PR, returning only the latest state per reviewer. + * COMMENTED reviews are ignored. DISMISSED reviews actively delete prior state + * to prevent ghost approvals (e.g. when GitHub auto-dismisses stale reviews + * after new commits are pushed). + * + * @param {object} github - Octokit instance + * @param {string} owner - Repository owner + * @param {string} repo - Repository name + * @param {number} prNumber - Pull request number + * @returns {Map} username → latest review state (APPROVED | CHANGES_REQUESTED) + */ +async function getLatestReviewStates(github, owner, repo, prNumber) { + const reviews = await github.paginate(github.rest.pulls.listReviews, { + owner, + repo, + pull_number: prNumber, + per_page: 100, + }); + + // Sort explicitly by submitted_at to guarantee chronological order. + // The GitHub API returns reviews chronologically in practice, but + // explicit sorting makes this correct by construction. + const sortedReviews = [...reviews].sort( + (a, b) => new Date(a.submitted_at) - new Date(b.submitted_at) + ); + + // Build a map keyed by reviewer login. + // Later entries overwrite earlier ones — giving us the latest state per user. + const latestByUser = new Map(); + + for (const review of sortedReviews) { + const login = review.user?.login; + const state = review.state?.toUpperCase(); + + if (!login || !state) continue; + + if (state === 'APPROVED' || state === 'CHANGES_REQUESTED') { + latestByUser.set(login, state); + } else if (state === 'DISMISSED') { + // CRITICAL: A dismissed review wipes out the user's prior approval. + // Without this, a stale review that GitHub auto-dismissed (e.g. after + // new commits) would persist as a ghost approval in the map. + latestByUser.delete(login); + } + // COMMENTED is the only state intentionally ignored + } + + return latestByUser; +} + +module.exports = { getLatestReviewStates }; diff --git a/.github/scripts/review-sync/index.js b/.github/scripts/review-sync/index.js index cb22d9463..1546ced9a 100644 --- a/.github/scripts/review-sync/index.js +++ b/.github/scripts/review-sync/index.js @@ -1,3 +1,5 @@ +// SPDX-License-Identifier: Apache-2.0 +// // .github/scripts/review-sync/index.js // // Entry point for the Review Queue Label Sync cron job. @@ -5,55 +7,15 @@ // Responsibilities: // 1. Rate-limit guard — abort if remaining calls < 200 // 2. Fetch all open non-draft PRs (paginated) -// 3. Ensure the three queue labels exist in the repo -// 4. Sync the correct label on every PR via labels.js +// 3. Ensure the four queue labels exist in the repo +// 4. Sync the correct label on every PR via helpers // 5. Print a summary of what changed // // Phase 1 of 4 — label sync only. -const { QUEUE_LABELS, syncLabel } = require('./labels.js'); - -const RATE_LIMIT_FLOOR = 200; - -/** - * Ensure a single label exists in the repo. - * Silently handles 422 (label already exists). - * - * Note: checks existence only. If a label already exists with the wrong - * colour or description, it will not be corrected in Phase 1. - */ -async function ensureLabel(github, owner, repo, label, dryRun) { - try { - await github.rest.issues.getLabel({ owner, repo, name: label.name }); - console.log(` Label "${label.name}" already exists. Skipping creation.`); - } catch (error) { - if (error.status === 404) { - if (dryRun) { - console.log(` [DRY RUN] Would create label "${label.name}" (${label.color}).`); - return; - } - try { - await github.rest.issues.createLabel({ - owner, - repo, - name: label.name, - color: label.color, - description: label.description, - }); - console.log(` Created label "${label.name}" (#${label.color}).`); - } catch (createError) { - // 422 = label already exists (race condition or concurrent run) - if (createError.status === 422) { - console.log(` Label "${label.name}" already exists (422). Skipping.`); - } else { - throw createError; - } - } - } else { - throw error; - } - } -} +const helpers = require('./helpers'); +const { QUEUE_LABELS, RATE_LIMIT_FLOOR } = helpers.constants; +const { ensureLabel, syncLabel } = helpers.labels; module.exports = async ({ github, context, core }) => { const dryRun = (process.env.DRY_RUN || 'false').toLowerCase() === 'true'; diff --git a/.github/scripts/review-sync/labels.js b/.github/scripts/review-sync/labels.js deleted file mode 100644 index 1b64703c3..000000000 --- a/.github/scripts/review-sync/labels.js +++ /dev/null @@ -1,266 +0,0 @@ -// .github/scripts/review-sync/labels.js -// -// Core label determination and application for the Review Queue Sync. -// -// Logic (Phase 1 — simplified, no difficulty routing): -// 1. Fetch all reviews on a PR (paginated, sorted by submitted_at) -// 2. Build username → latest review state map -// 3. Ignore COMMENTED; DISMISSED actively deletes prior state (prevents ghost approvals) -// 4. For each APPROVED reviewer, check permission level -// 5. Determine correct queue label based on approval counts -// 6. Apply label: add new FIRST, then remove stale (non-negotiable order) -// -// Phase 2 will add difficulty-based routing on top of this logic. - -/** - * The three mutually exclusive queue labels managed by this bot. - * Colours and names match the Phase 1 specification. - */ -const QUEUE_LABELS = { - JUNIOR: { - name: 'queue:junior-committer', - color: 'e4e669', - description: 'PR awaiting initial quality review', - }, - COMMITTERS: { - name: 'queue:committers', - color: '0075ca', - description: 'PR awaiting committer technical review', - }, - MERGE: { - name: 'ready-to-merge', - color: '0e8a16', - description: 'PR has 2+ write approvals and is ready to merge', - }, -}; - -/** All queue label names, used for cleanup operations. */ -const ALL_QUEUE_LABEL_NAMES = Object.values(QUEUE_LABELS).map((l) => l.name); - -/** - * Fetch all reviews on a PR, returning only the latest state per reviewer. - * COMMENTED reviews are ignored. DISMISSED reviews actively delete prior state - * to prevent ghost approvals (e.g. when GitHub auto-dismisses stale reviews - * after new commits are pushed). - * - * @param {object} github - Octokit instance - * @param {string} owner - Repository owner - * @param {string} repo - Repository name - * @param {number} prNumber - Pull request number - * @returns {Map} username → latest review state (APPROVED | CHANGES_REQUESTED) - */ -async function getLatestReviewStates(github, owner, repo, prNumber) { - const reviews = await github.paginate(github.rest.pulls.listReviews, { - owner, - repo, - pull_number: prNumber, - per_page: 100, - }); - - // Sort explicitly by submitted_at to guarantee chronological order. - // The GitHub API returns reviews chronologically in practice, but - // explicit sorting makes this correct by construction. - const sortedReviews = [...reviews].sort( - (a, b) => new Date(a.submitted_at) - new Date(b.submitted_at) - ); - - // Build a map keyed by reviewer login. - // Later entries overwrite earlier ones — giving us the latest state per user. - const latestByUser = new Map(); - - for (const review of sortedReviews) { - const login = review.user?.login; - const state = review.state?.toUpperCase(); - - if (!login || !state) continue; - - if (state === 'APPROVED' || state === 'CHANGES_REQUESTED') { - latestByUser.set(login, state); - } else if (state === 'DISMISSED') { - // CRITICAL: A dismissed review wipes out the user's prior approval. - // Without this, a stale review that GitHub auto-dismissed (e.g. after - // new commits) would persist as a ghost approval in the map. - latestByUser.delete(login); - } - // COMMENTED is the only state intentionally ignored - } - - return latestByUser; -} - -/** - * Check the repository permission level for a given user. - * - * @param {object} github - Octokit instance - * @param {string} owner - Repository owner - * @param {string} repo - Repository name - * @param {string} username - GitHub username - * @returns {string} 'admin' | 'write' | 'read' | 'none' - */ -async function getPermissionLevel(github, owner, repo, username) { - try { - const { data } = await github.rest.repos.getCollaboratorPermissionLevel({ - owner, - repo, - username, - }); - return data.permission || 'none'; - } catch (error) { - if (error.status === 404) { - // External contributor — not a collaborator - return 'none'; - } - // Log unexpected errors but don't crash the run - const message = error instanceof Error ? error.message : String(error); - console.log(` ⚠ Permission check failed for ${username}: ${message}. Treating as "none".`); - return 'none'; - } -} - -/** - * Count approvals on a PR, split by permission level. - * - * @param {object} github - Octokit instance - * @param {string} owner - Repository owner - * @param {string} repo - Repository name - * @param {number} prNumber - Pull request number - * @returns {{ writeApproval: number, softApproval: number, anyApproval: number }} - */ -async function countApprovals(github, owner, repo, prNumber) { - const latestStates = await getLatestReviewStates(github, owner, repo, prNumber); - - let writeApproval = 0; - let softApproval = 0; - - for (const [username, state] of latestStates) { - if (state !== 'APPROVED') continue; - - const permission = await getPermissionLevel(github, owner, repo, username); - - if (permission === 'admin' || permission === 'maintain' || permission === 'write') { - writeApproval++; - } else { - // read, none, or any unexpected value → soft approval - softApproval++; - } - } - - return { - writeApproval, - softApproval, - anyApproval: writeApproval + softApproval, - }; -} - -/** - * Determine the correct queue label for a PR based on approval counts. - * - * Phase 1 logic (no difficulty routing): - * writeApproval >= 2 → ready-to-merge - * anyApproval >= 1 → queue:committers - * else → queue:junior-committer - * - * @param {{ writeApproval: number, anyApproval: number }} approvals - * @returns {object} The correct QUEUE_LABELS entry - */ -function determineLabel(approvals) { - if (approvals.writeApproval >= 2) { - return QUEUE_LABELS.MERGE; - } - if (approvals.anyApproval >= 1) { - return QUEUE_LABELS.COMMITTERS; - } - return QUEUE_LABELS.JUNIOR; -} - -/** - * Sync the queue label on a single PR. - * - * Order of operations (non-negotiable): - * 1. Compute stale queue labels on the PR - * 2. Skip only if correct label is already present AND no stale labels exist - * 3. ADD the correct label first - * 4. THEN remove any stale queue labels - * - * This ensures a PR never has zero queue labels, even if the process - * crashes mid-run. Stale labels are always cleaned up, even when the - * correct label was already present. - * - * @param {object} github - Octokit instance - * @param {string} owner - Repository owner - * @param {string} repo - Repository name - * @param {object} pr - Pull request object from the list API - * @param {boolean} dryRun - If true, log without making changes - * @returns {boolean} true if the label was changed, false if already correct - */ -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 - const approvals = await countApprovals(github, owner, repo, prNumber); - const correctLabel = determineLabel(approvals); - - console.log( - ` PR #${prNumber}: writeApproval=${approvals.writeApproval}, ` + - `softApproval=${approvals.softApproval}, anyApproval=${approvals.anyApproval} ` + - `→ ${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.`); - return false; - } - - if (dryRun) { - console.log(` [DRY RUN] Would add "${correctLabel.name}".`); - 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 2: THEN remove stale queue labels one by one - for (const stale of staleLabels) { - try { - await github.rest.issues.removeLabel({ - owner, - repo, - issue_number: prNumber, - name: stale, - }); - console.log(` - Removed "${stale}".`); - } catch (error) { - // 404 = label was already removed (race condition or manual action) - if (error.status === 404) { - console.log(` - Label "${stale}" already gone (404). Skipping.`); - } else { - const message = error instanceof Error ? error.message : String(error); - console.error(` ✗ Failed to remove "${stale}": ${message}`); - } - } - } - - return true; -} - -module.exports = { - QUEUE_LABELS, - ALL_QUEUE_LABEL_NAMES, - syncLabel, -}; diff --git a/.github/scripts/review-sync/tests/test-labels.js b/.github/scripts/review-sync/tests/test-labels.js new file mode 100644 index 000000000..9f5490de9 --- /dev/null +++ b/.github/scripts/review-sync/tests/test-labels.js @@ -0,0 +1,151 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// tests/test-labels.js +// +// Unit tests for helpers/labels.js (determineLabel, ensureLabel, syncLabel). +// Run with: node .github/scripts/review-sync/tests/test-labels.js + +const { runTestSuite, createMockGithub } = require('./test-utils'); +const { determineLabel, ensureLabel, syncLabel } = require('../helpers/labels'); +const { QUEUE_LABELS } = require('../helpers/constants'); + +const unitTests = [ + { + name: 'determineLabel: 0 approvals → queue:junior-committer', + test: () => { + const r = determineLabel({ maintainerApproval: 0, writeApproval: 0, softApproval: 0, anyApproval: 0 }); + return r.name === 'queue:junior-committer'; + }, + }, + { + name: 'determineLabel: 1 soft approval → queue:committers', + test: () => { + const r = determineLabel({ maintainerApproval: 0, writeApproval: 0, softApproval: 1, anyApproval: 1 }); + return r.name === 'queue:committers'; + }, + }, + { + name: 'determineLabel: 1 write approval → queue:maintainers', + test: () => { + const r = determineLabel({ maintainerApproval: 0, writeApproval: 1, softApproval: 0, anyApproval: 1 }); + return r.name === 'queue:maintainers'; + }, + }, + { + name: 'determineLabel: 2 write + 0 maintainer → queue:maintainers (NOT ready-to-merge)', + test: () => { + const r = determineLabel({ maintainerApproval: 0, writeApproval: 2, softApproval: 0, anyApproval: 2 }); + return r.name === 'queue:maintainers'; + }, + }, + { + name: 'determineLabel: 1 maintainer → ready-to-merge', + test: () => { + const r = determineLabel({ maintainerApproval: 1, writeApproval: 0, softApproval: 0, anyApproval: 1 }); + return r.name === 'ready-to-merge'; + }, + }, + { + name: 'determineLabel: 1 maintainer + 1 write → ready-to-merge', + test: () => { + const r = determineLabel({ maintainerApproval: 1, writeApproval: 1, softApproval: 0, anyApproval: 2 }); + return r.name === 'ready-to-merge'; + }, + }, + { + name: 'determineLabel: 3 soft, 0 write → queue:committers', + test: () => { + const r = determineLabel({ maintainerApproval: 0, writeApproval: 0, softApproval: 3, anyApproval: 3 }); + return r.name === 'queue:committers'; + }, + }, + { + name: 'ensureLabel: creates label when missing', + test: async () => { + const mock = createMockGithub({ existingLabels: {} }); + await ensureLabel(mock, 'o', 'r', QUEUE_LABELS.JUNIOR, false); + return mock.calls.labelsCreated.length === 1 && mock.calls.labelsCreated[0].name === 'queue:junior-committer'; + }, + }, + { + name: 'ensureLabel: skips when label exists', + test: async () => { + const mock = createMockGithub({ existingLabels: { 'queue:junior-committer': true } }); + await ensureLabel(mock, 'o', 'r', QUEUE_LABELS.JUNIOR, false); + return mock.calls.labelsCreated.length === 0; + }, + }, + { + name: 'ensureLabel: dry run does not create', + test: async () => { + const mock = createMockGithub({ existingLabels: {} }); + await ensureLabel(mock, 'o', 'r', QUEUE_LABELS.JUNIOR, true); + return mock.calls.labelsCreated.length === 0; + }, + }, + { + name: 'syncLabel: no approvals, no labels → adds junior-committer', + test: async () => { + const mock = createMockGithub({ roles: {}, reviews: [] }); + const changed = await syncLabel(mock, 'o', 'r', { number: 1, labels: [] }, false); + return changed === true && mock.calls.labelsAdded[0] === 'queue:junior-committer'; + }, + }, + { + name: 'syncLabel: already correct, no stale → returns false', + test: async () => { + const mock = createMockGithub({ roles: {}, reviews: [] }); + const changed = await syncLabel(mock, 'o', 'r', { number: 1, labels: [{ name: 'queue:junior-committer' }] }, false); + return changed === false && mock.calls.labelsAdded.length === 0; + }, + }, + { + name: 'syncLabel: stale label → adds correct, removes stale', + test: async () => { + const mock = createMockGithub({ + roles: { sophie: { role_name: 'maintain', permission: 'write' } }, + reviews: [{ user: { login: 'sophie' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }], + }); + const changed = await syncLabel(mock, 'o', 'r', { number: 1, labels: [{ name: 'queue:junior-committer' }] }, false); + return changed === true && mock.calls.labelsAdded.includes('ready-to-merge') && mock.calls.labelsRemoved.includes('queue:junior-committer'); + }, + }, + { + name: 'syncLabel: dry run logs but does not modify', + test: async () => { + const mock = createMockGithub({ roles: {}, reviews: [] }); + const changed = await syncLabel(mock, 'o', 'r', { number: 1, labels: [{ name: 'queue:committers' }] }, true); + return changed === true && mock.calls.labelsAdded.length === 0; + }, + }, + { + name: 'syncLabel: correct + stale present → cleans up stale', + test: async () => { + const mock = createMockGithub({ roles: {}, reviews: [] }); + const pr = { number: 1, labels: [{ name: 'queue:junior-committer' }, { name: 'queue:committers' }] }; + const changed = await syncLabel(mock, 'o', 'r', pr, false); + return changed === true && mock.calls.labelsRemoved.includes('queue:committers'); + }, + }, +]; + +async function runUnitTests() { + console.log('🔬 UNIT TESTS (labels)'); + console.log('='.repeat(70)); + let passed = 0; + let failed = 0; + for (const t of unitTests) { + try { + const result = await Promise.resolve(t.test()); + if (result) { console.log(`✅ ${t.name}`); passed++; } + else { console.log(`❌ ${t.name}`); failed++; } + } catch (error) { console.log(`❌ ${t.name} - Error: ${error.message}`); failed++; } + } + console.log('\n' + '-'.repeat(70)); + console.log(`Unit Tests: ${passed} passed, ${failed} failed`); + return { total: unitTests.length, passed, failed }; +} + +runTestSuite('LABELS TEST SUITE', [], async () => true, [ + { label: 'Unit Tests', run: runUnitTests }, +]); diff --git a/.github/scripts/review-sync/tests/test-permissions.js b/.github/scripts/review-sync/tests/test-permissions.js new file mode 100644 index 000000000..b785a036e --- /dev/null +++ b/.github/scripts/review-sync/tests/test-permissions.js @@ -0,0 +1,236 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// tests/test-permissions.js +// +// Unit tests for helpers/permissions.js (getPermissionLevel, countApprovals). +// Run with: node .github/scripts/review-sync/tests/test-permissions.js + +const { runTestSuite, createMockGithub } = require('./test-utils'); +const { getPermissionLevel, countApprovals } = require('../helpers/permissions'); + +// ============================================================================= +// UNIT TESTS +// ============================================================================= + +const unitTests = [ + // --------------------------------------------------------------------------- + // getPermissionLevel + // --------------------------------------------------------------------------- + { + name: 'getPermissionLevel: uses role_name over permission (maintain case)', + test: async () => { + const mock = createMockGithub({ + roles: { sophie: { role_name: 'maintain', permission: 'write' } }, + }); + const result = await getPermissionLevel(mock, 'owner', 'repo', 'sophie'); + return result === 'maintain'; // NOT 'write' + }, + }, + { + name: 'getPermissionLevel: uses role_name over permission (admin case)', + test: async () => { + const mock = createMockGithub({ + roles: { admin: { role_name: 'admin', permission: 'admin' } }, + }); + const result = await getPermissionLevel(mock, 'owner', 'repo', 'admin'); + return result === 'admin'; + }, + }, + { + name: 'getPermissionLevel: falls back to permission if role_name missing', + test: async () => { + const mock = createMockGithub({ + roles: { bob: { permission: 'write' } }, + }); + const result = await getPermissionLevel(mock, 'owner', 'repo', 'bob'); + return result === 'write'; + }, + }, + { + name: 'getPermissionLevel: external contributor (404) returns none', + test: async () => { + const mock = createMockGithub({ roles: {} }); + const result = await getPermissionLevel(mock, 'owner', 'repo', 'unknown-user'); + return result === 'none'; + }, + }, + { + name: 'getPermissionLevel: triage role returned correctly', + test: async () => { + const mock = createMockGithub({ + roles: { triager: { role_name: 'triage', permission: 'read' } }, + }); + const result = await getPermissionLevel(mock, 'owner', 'repo', 'triager'); + return result === 'triage'; + }, + }, + { + name: 'getPermissionLevel: read role returned correctly', + test: async () => { + const mock = createMockGithub({ + roles: { reader: { role_name: 'read', permission: 'read' } }, + }); + const result = await getPermissionLevel(mock, 'owner', 'repo', 'reader'); + return result === 'read'; + }, + }, + + // --------------------------------------------------------------------------- + // countApprovals + // --------------------------------------------------------------------------- + { + name: 'countApprovals: maintain counted as maintainerApproval, write as writeApproval', + test: async () => { + const mock = createMockGithub({ + roles: { + sophie: { role_name: 'maintain', permission: 'write' }, + bob: { role_name: 'write', permission: 'write' }, + }, + reviews: [ + { user: { login: 'sophie' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + { user: { login: 'bob' }, state: 'APPROVED', submitted_at: '2026-01-02T00:00:00Z' }, + ], + }); + const result = await countApprovals(mock, 'owner', 'repo', 1); + return ( + result.maintainerApproval === 1 && + result.writeApproval === 1 && + result.softApproval === 0 && + result.anyApproval === 2 + ); + }, + }, + { + name: 'countApprovals: admin counted as maintainerApproval', + test: async () => { + const mock = createMockGithub({ + roles: { + admin: { role_name: 'admin', permission: 'admin' }, + }, + reviews: [ + { user: { login: 'admin' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + ], + }); + const result = await countApprovals(mock, 'owner', 'repo', 1); + return result.maintainerApproval === 1 && result.anyApproval === 1; + }, + }, + { + name: 'countApprovals: external contributor counted as softApproval', + test: async () => { + const mock = createMockGithub({ + roles: {}, + reviews: [ + { user: { login: 'external' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + ], + }); + const result = await countApprovals(mock, 'owner', 'repo', 1); + return ( + result.maintainerApproval === 0 && + result.writeApproval === 0 && + result.softApproval === 1 && + result.anyApproval === 1 + ); + }, + }, + { + name: 'countApprovals: CHANGES_REQUESTED not counted in any counter', + test: async () => { + const mock = createMockGithub({ + roles: { + bob: { role_name: 'write', permission: 'write' }, + }, + reviews: [ + { user: { login: 'bob' }, state: 'CHANGES_REQUESTED', submitted_at: '2026-01-01T00:00:00Z' }, + ], + }); + const result = await countApprovals(mock, 'owner', 'repo', 1); + return result.anyApproval === 0; + }, + }, + { + name: 'countApprovals: no reviews returns all zeros', + test: async () => { + const mock = createMockGithub({ roles: {}, reviews: [] }); + const result = await countApprovals(mock, 'owner', 'repo', 1); + return ( + result.maintainerApproval === 0 && + result.writeApproval === 0 && + result.softApproval === 0 && + result.anyApproval === 0 + ); + }, + }, + { + name: 'countApprovals: mixed roles — 1 admin + 1 write + 1 external', + test: async () => { + const mock = createMockGithub({ + roles: { + admin: { role_name: 'admin', permission: 'admin' }, + committer: { role_name: 'write', permission: 'write' }, + }, + reviews: [ + { user: { login: 'admin' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + { user: { login: 'committer' }, state: 'APPROVED', submitted_at: '2026-01-02T00:00:00Z' }, + { user: { login: 'random' }, state: 'APPROVED', submitted_at: '2026-01-03T00:00:00Z' }, + ], + }); + const result = await countApprovals(mock, 'owner', 'repo', 1); + return ( + result.maintainerApproval === 1 && + result.writeApproval === 1 && + result.softApproval === 1 && + result.anyApproval === 3 + ); + }, + }, + { + name: 'countApprovals: DISMISSED approval is not counted', + test: async () => { + const mock = createMockGithub({ + roles: { + sophie: { role_name: 'maintain', permission: 'write' }, + }, + reviews: [ + { user: { login: 'sophie' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + { user: { login: 'sophie' }, state: 'DISMISSED', submitted_at: '2026-01-02T00:00:00Z' }, + ], + }); + const result = await countApprovals(mock, 'owner', 'repo', 1); + return result.maintainerApproval === 0 && result.anyApproval === 0; + }, + }, +]; + +// ============================================================================= +// TEST RUNNER +// ============================================================================= + +async function runUnitTests() { + console.log('🔬 UNIT TESTS (permissions)'); + console.log('='.repeat(70)); + let passed = 0; + let failed = 0; + for (const test of unitTests) { + try { + const result = await Promise.resolve(test.test()); + if (result) { + console.log(`✅ ${test.name}`); + passed++; + } else { + console.log(`❌ ${test.name}`); + failed++; + } + } catch (error) { + console.log(`❌ ${test.name} - Error: ${error.message}`); + failed++; + } + } + console.log('\n' + '-'.repeat(70)); + console.log(`Unit Tests: ${passed} passed, ${failed} failed`); + return { total: unitTests.length, passed, failed }; +} + +runTestSuite('PERMISSIONS TEST SUITE', [], async () => true, [ + { label: 'Unit Tests', run: runUnitTests }, +]); diff --git a/.github/scripts/review-sync/tests/test-reviews.js b/.github/scripts/review-sync/tests/test-reviews.js new file mode 100644 index 000000000..2dab3c725 --- /dev/null +++ b/.github/scripts/review-sync/tests/test-reviews.js @@ -0,0 +1,144 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// tests/test-reviews.js +// +// Unit tests for helpers/reviews.js (getLatestReviewStates). +// Run with: node .github/scripts/review-sync/tests/test-reviews.js + +const { runTestSuite, createMockGithub } = require('./test-utils'); +const { getLatestReviewStates } = require('../helpers/reviews'); + +const unitTests = [ + { + name: 'getLatestReviewStates: single APPROVED → returns approved', + test: async () => { + const mock = createMockGithub({ + reviews: [ + { user: { login: 'alice' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + ], + }); + const states = await getLatestReviewStates(mock, 'o', 'r', 1); + return states.size === 1 && states.get('alice') === 'APPROVED'; + }, + }, + { + name: 'getLatestReviewStates: DISMISSED deletes prior APPROVED', + test: async () => { + const mock = createMockGithub({ + reviews: [ + { user: { login: 'alice' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + { user: { login: 'alice' }, state: 'DISMISSED', submitted_at: '2026-01-02T00:00:00Z' }, + ], + }); + const states = await getLatestReviewStates(mock, 'o', 'r', 1); + return states.size === 0; + }, + }, + { + name: 'getLatestReviewStates: COMMENTED is ignored', + test: async () => { + const mock = createMockGithub({ + reviews: [ + { user: { login: 'bob' }, state: 'COMMENTED', submitted_at: '2026-01-01T00:00:00Z' }, + ], + }); + const states = await getLatestReviewStates(mock, 'o', 'r', 1); + return states.size === 0; + }, + }, + { + name: 'getLatestReviewStates: later APPROVED overwrites CHANGES_REQUESTED', + test: async () => { + const mock = createMockGithub({ + reviews: [ + { user: { login: 'alice' }, state: 'CHANGES_REQUESTED', submitted_at: '2026-01-01T00:00:00Z' }, + { user: { login: 'alice' }, state: 'APPROVED', submitted_at: '2026-01-02T00:00:00Z' }, + ], + }); + const states = await getLatestReviewStates(mock, 'o', 'r', 1); + return states.get('alice') === 'APPROVED'; + }, + }, + { + name: 'getLatestReviewStates: later CHANGES_REQUESTED overwrites APPROVED', + test: async () => { + const mock = createMockGithub({ + reviews: [ + { user: { login: 'alice' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + { user: { login: 'alice' }, state: 'CHANGES_REQUESTED', submitted_at: '2026-01-02T00:00:00Z' }, + ], + }); + const states = await getLatestReviewStates(mock, 'o', 'r', 1); + return states.get('alice') === 'CHANGES_REQUESTED'; + }, + }, + { + name: 'getLatestReviewStates: multiple reviewers tracked independently', + test: async () => { + const mock = createMockGithub({ + reviews: [ + { user: { login: 'alice' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + { user: { login: 'bob' }, state: 'CHANGES_REQUESTED', submitted_at: '2026-01-02T00:00:00Z' }, + ], + }); + const states = await getLatestReviewStates(mock, 'o', 'r', 1); + return states.size === 2 && states.get('alice') === 'APPROVED' && states.get('bob') === 'CHANGES_REQUESTED'; + }, + }, + { + name: 'getLatestReviewStates: no reviews → empty map', + test: async () => { + const mock = createMockGithub({ reviews: [] }); + const states = await getLatestReviewStates(mock, 'o', 'r', 1); + return states.size === 0; + }, + }, + { + name: 'getLatestReviewStates: null login or state gracefully skipped', + test: async () => { + const mock = createMockGithub({ + reviews: [ + { user: null, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + { user: { login: 'bob' }, state: null, submitted_at: '2026-01-02T00:00:00Z' }, + ], + }); + const states = await getLatestReviewStates(mock, 'o', 'r', 1); + return states.size === 0; + }, + }, + { + name: 'getLatestReviewStates: out-of-order timestamps sorted correctly', + test: async () => { + const mock = createMockGithub({ + reviews: [ + { user: { login: 'alice' }, state: 'APPROVED', submitted_at: '2026-01-05T00:00:00Z' }, + { user: { login: 'alice' }, state: 'CHANGES_REQUESTED', submitted_at: '2026-01-01T00:00:00Z' }, + ], + }); + const states = await getLatestReviewStates(mock, 'o', 'r', 1); + // After sorting, CHANGES_REQUESTED is first, APPROVED is second → final state is APPROVED + return states.get('alice') === 'APPROVED'; + }, + }, +]; + +async function runUnitTests() { + console.log('🔬 UNIT TESTS (reviews)'); + console.log('='.repeat(70)); + let passed = 0; + let failed = 0; + for (const t of unitTests) { + try { + const result = await Promise.resolve(t.test()); + if (result) { console.log(`✅ ${t.name}`); passed++; } + else { console.log(`❌ ${t.name}`); failed++; } + } catch (error) { console.log(`❌ ${t.name} - Error: ${error.message}`); failed++; } + } + console.log('\n' + '-'.repeat(70)); + console.log(`Unit Tests: ${passed} passed, ${failed} failed`); + return { total: unitTests.length, passed, failed }; +} + +runTestSuite('REVIEWS TEST SUITE', [], async () => true, [ + { label: 'Unit Tests', run: runUnitTests }, +]); diff --git a/.github/scripts/review-sync/tests/test-utils.js b/.github/scripts/review-sync/tests/test-utils.js new file mode 100644 index 000000000..7f4aa7cc9 --- /dev/null +++ b/.github/scripts/review-sync/tests/test-utils.js @@ -0,0 +1,162 @@ +// SPDX-License-Identifier: Apache-2.0 +// +// tests/test-utils.js +// +// Shared test utilities for review-sync test suites. +// Adapted from hiero-sdk-cpp/.github/scripts/tests/test-utils.js +// +// Provides: +// - runTestSuite() — CLI-aware test runner with summary +// - printSummaryAndExit() — formatted results + exit code +// - createMockGithub() — mock GitHub API factory with call tracking + +// ============================================================================= +// SUMMARY & RUNNER +// ============================================================================= + +/** + * Prints a summary table and exits with the appropriate code. + * + * @param {{ label: string, total: number, passed: number, failed: number }[]} sections + */ +function printSummaryAndExit(sections) { + console.log('\n' + '='.repeat(70)); + console.log('📈 SUMMARY'); + console.log('='.repeat(70)); + + let anyFailed = false; + for (const { label, total, passed, failed } of sections) { + if (failed > 0) anyFailed = true; + console.log( + ` ${label}: ${total} total, ${passed} passed` + + `${failed > 0 ? `, ${failed} failed ❌` : ' ✅'}` + ); + } + + console.log('='.repeat(70)); + process.exit(anyFailed ? 1 : 0); +} + +/** + * Parses the optional test-index CLI argument and either runs a single + * test or all tests, then prints a summary and exits. + * + * @param {string} suiteName - Display name (e.g. "PERMISSIONS TEST SUITE") + * @param {object[]} scenarios - Array of scenario objects (unused for unit-only suites, pass []) + * @param {function} runScenario - Async function that runs one scenario (pass async () => true for unit-only) + * @param {{ label: string, run: () => Promise<{ total: number, passed: number, failed: number }> }[]} extraSections + */ +async function runTestSuite(suiteName, scenarios, runScenario, extraSections = []) { + console.log(`🧪 ${suiteName}`); + console.log('='.repeat(suiteName.length + 3) + '\n'); + + const summaries = []; + + for (const section of extraSections) { + const result = await section.run(); + summaries.push({ label: section.label, ...result }); + } + + // Only run integration scenarios if there are any + if (scenarios.length > 0) { + console.log('\n\n🔗 INTEGRATION TESTS'); + console.log('='.repeat(70)); + + let passed = 0; + let failed = 0; + for (let i = 0; i < scenarios.length; i++) { + const ok = await runScenario(scenarios[i], i); + if (ok) passed++; + else failed++; + } + summaries.push({ label: 'Integration Tests', total: scenarios.length, passed, failed }); + } + + printSummaryAndExit(summaries); +} + +// ============================================================================= +// MOCK GITHUB FACTORY +// ============================================================================= + +/** + * Creates a mock GitHub API object for review-sync tests. + * Tracks labels, permission checks, and review fetches via the returned calls object. + * + * @param {object} options + * @param {Record} options.roles + * Map of username → permission data returned by getCollaboratorPermissionLevel + * @param {Array} options.reviews + * Array of review objects returned by pulls.listReviews + * @param {Record} options.existingLabels + * Map of label name → true (label exists in repo) + * @returns {{ calls: object, rest: object, paginate: function }} + */ +function createMockGithub(options = {}) { + const { + roles = {}, + reviews = [], + existingLabels = {}, + } = options; + + const calls = { + labelsAdded: [], + labelsRemoved: [], + labelsCreated: [], + labelsChecked: [], + permissionsChecked: [], + }; + + const mock = { + calls, + rest: { + repos: { + getCollaboratorPermissionLevel: async ({ username }) => { + calls.permissionsChecked.push(username); + const role = roles[username]; + if (!role) { + throw Object.assign(new Error('Not found'), { status: 404 }); + } + return { data: role }; + }, + }, + pulls: { + listReviews: async () => ({ data: reviews }), + }, + issues: { + getLabel: async ({ name }) => { + calls.labelsChecked.push(name); + if (!existingLabels[name]) { + throw Object.assign(new Error('Not found'), { status: 404 }); + } + return { data: { name } }; + }, + createLabel: async ({ name, color, description }) => { + calls.labelsCreated.push({ name, color, description }); + return {}; + }, + addLabels: async ({ labels }) => { + calls.labelsAdded.push(...labels); + return {}; + }, + removeLabel: async ({ name }) => { + calls.labelsRemoved.push(name); + return {}; + }, + }, + rateLimit: { + get: async () => ({ + data: { resources: { core: { remaining: 5000 } } }, + }), + }, + }, + paginate: async (fn, opts) => { + const result = await fn(opts); + return result.data || result || []; + }, + }; + + return mock; +} + +module.exports = { runTestSuite, printSummaryAndExit, createMockGithub }; diff --git a/.github/workflows/review-sync.yml b/.github/workflows/review-sync.yml index 46f80c21d..9799fdbf4 100644 --- a/.github/workflows/review-sync.yml +++ b/.github/workflows/review-sync.yml @@ -1,7 +1,7 @@ # Automated PR review queue label sync. # Runs on a 30-minute schedule to classify every open non-draft PR into one of -# three review stages based on the number and permission level of approvals: -# queue:junior-committer → queue:committers → ready-to-merge +# four review stages based on the number and permission level of approvals: +# queue:junior-committer → queue:committers → queue:maintainers → ready-to-merge # # Phase 1 of 4 — label sync only. No assignments, no comments, no routing. # From 84a35d41a3806baeafc34c34c9042b5f963d43f7 Mon Sep 17 00:00:00 2001 From: darshit2308 Date: Thu, 7 May 2026 01:14:43 +0530 Subject: [PATCH 05/10] 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 --- .github/scripts/review-sync/helpers/labels.js | 16 ++++++++----- .../scripts/review-sync/tests/test-labels.js | 24 +++++++++++++++---- 2 files changed, 29 insertions(+), 11 deletions(-) diff --git a/.github/scripts/review-sync/helpers/labels.js b/.github/scripts/review-sync/helpers/labels.js index 9cb86fda2..c9ca5b275 100644 --- a/.github/scripts/review-sync/helpers/labels.js +++ b/.github/scripts/review-sync/helpers/labels.js @@ -57,19 +57,23 @@ async function ensureLabel(github, owner, repo, label, dryRun) { * Determine the correct queue label for a PR based on approval counts. * * Phase 1 logic (4-stage pipeline): - * maintainerApproval >= 1 → ready-to-merge (CODEOWNERS satisfied) - * writeApproval >= 1 → queue:maintainers (committer approved, needs maintainer) - * anyApproval >= 1 → queue:committers (has any approval, needs committer) - * else → queue:junior-committer (no approvals yet) + * maintainerApproval >= 1 AND anyApproval >= 2 → ready-to-merge (CODEOWNERS + min 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) + * + * Note: ready-to-merge requires BOTH a maintainer approval AND at least 2 + * total reviews. This prevents a single maintainer approval from marking + * a PR as ready-to-merge when branch protection requires 2+ reviews. * * @param {{ maintainerApproval: number, writeApproval: number, softApproval: number, anyApproval: number }} approvals * @returns {object} The correct QUEUE_LABELS entry */ function determineLabel(approvals) { - if (approvals.maintainerApproval >= 1) { + if (approvals.maintainerApproval >= 1 && approvals.anyApproval >= 2) { return QUEUE_LABELS.MERGE; } - if (approvals.writeApproval >= 1) { + if (approvals.writeApproval >= 1 || approvals.maintainerApproval >= 1) { return QUEUE_LABELS.MAINTAINERS; } if (approvals.anyApproval >= 1) { diff --git a/.github/scripts/review-sync/tests/test-labels.js b/.github/scripts/review-sync/tests/test-labels.js index 9f5490de9..0d876462b 100644 --- a/.github/scripts/review-sync/tests/test-labels.js +++ b/.github/scripts/review-sync/tests/test-labels.js @@ -39,19 +39,27 @@ const unitTests = [ }, }, { - name: 'determineLabel: 1 maintainer → ready-to-merge', + name: 'determineLabel: 1 maintainer alone → queue:maintainers (NOT ready-to-merge, needs 2 reviews)', test: () => { + // Sophie's edge case: maintainer approves first, only 1 total review const r = determineLabel({ maintainerApproval: 1, writeApproval: 0, softApproval: 0, anyApproval: 1 }); - return r.name === 'ready-to-merge'; + return r.name === 'queue:maintainers'; }, }, { - name: 'determineLabel: 1 maintainer + 1 write → ready-to-merge', + name: 'determineLabel: 1 maintainer + 1 write → ready-to-merge (2 reviews satisfied)', test: () => { const r = determineLabel({ maintainerApproval: 1, writeApproval: 1, softApproval: 0, anyApproval: 2 }); return r.name === 'ready-to-merge'; }, }, + { + name: 'determineLabel: 1 maintainer + 1 soft → ready-to-merge (2 reviews satisfied)', + test: () => { + const r = determineLabel({ maintainerApproval: 1, writeApproval: 0, softApproval: 1, anyApproval: 2 }); + return r.name === 'ready-to-merge'; + }, + }, { name: 'determineLabel: 3 soft, 0 write → queue:committers', test: () => { @@ -103,8 +111,14 @@ const unitTests = [ name: 'syncLabel: stale label → adds correct, removes stale', test: async () => { const mock = createMockGithub({ - roles: { sophie: { role_name: 'maintain', permission: 'write' } }, - reviews: [{ user: { login: 'sophie' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }], + roles: { + sophie: { role_name: 'maintain', permission: 'write' }, + bob: { role_name: 'write', permission: 'write' }, + }, + reviews: [ + { user: { login: 'sophie' }, state: 'APPROVED', submitted_at: '2026-01-01T00:00:00Z' }, + { user: { login: 'bob' }, state: 'APPROVED', submitted_at: '2026-01-02T00:00:00Z' }, + ], }); const changed = await syncLabel(mock, 'o', 'r', { number: 1, labels: [{ name: 'queue:junior-committer' }] }, false); return changed === true && mock.calls.labelsAdded.includes('ready-to-merge') && mock.calls.labelsRemoved.includes('queue:junior-committer'); From 058267934b2643513968701a9f70f9901a683a73 Mon Sep 17 00:00:00 2001 From: darshit2308 Date: Thu, 7 May 2026 12:48:31 +0530 Subject: [PATCH 06/10] fix: rename ready-to-merge to status: ready-to-merge Addresses maintainer feedback to match the original MVP proposal naming convention. Also updates the label description to accurately reflect the new logic requirement of '1+ maintainer and 2+ total approvals' instead of the old description. Signed-off-by: darshit2308 --- .github/scripts/review-sync/helpers/constants.js | 4 ++-- .github/scripts/review-sync/helpers/labels.js | 6 +++--- .github/scripts/review-sync/tests/test-labels.js | 14 +++++++------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/.github/scripts/review-sync/helpers/constants.js b/.github/scripts/review-sync/helpers/constants.js index 92c0c12e7..df25c398c 100644 --- a/.github/scripts/review-sync/helpers/constants.js +++ b/.github/scripts/review-sync/helpers/constants.js @@ -33,9 +33,9 @@ const QUEUE_LABELS = { description: 'PR awaiting maintainer final review', }, MERGE: { - name: 'ready-to-merge', + name: 'status: ready-to-merge', color: '0e8a16', - description: 'PR approved by maintainer and ready to merge', + description: 'PR has 1+ maintainer and 2+ total approvals, ready to merge', }, }; diff --git a/.github/scripts/review-sync/helpers/labels.js b/.github/scripts/review-sync/helpers/labels.js index c9ca5b275..b7299e281 100644 --- a/.github/scripts/review-sync/helpers/labels.js +++ b/.github/scripts/review-sync/helpers/labels.js @@ -57,14 +57,14 @@ async function ensureLabel(github, owner, repo, label, dryRun) { * Determine the correct queue label for a PR based on approval counts. * * Phase 1 logic (4-stage pipeline): - * maintainerApproval >= 1 AND anyApproval >= 2 → ready-to-merge (CODEOWNERS + min reviews) + * maintainerApproval >= 1 AND anyApproval >= 2 → status: ready-to-merge (CODEOWNERS + min 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) * - * Note: ready-to-merge requires BOTH a maintainer approval AND at least 2 + * Note: status: ready-to-merge requires BOTH a maintainer approval AND at least 2 * total reviews. This prevents a single maintainer approval from marking - * a PR as ready-to-merge when branch protection requires 2+ reviews. + * a PR as status: ready-to-merge when branch protection requires 2+ reviews. * * @param {{ maintainerApproval: number, writeApproval: number, softApproval: number, anyApproval: number }} approvals * @returns {object} The correct QUEUE_LABELS entry diff --git a/.github/scripts/review-sync/tests/test-labels.js b/.github/scripts/review-sync/tests/test-labels.js index 0d876462b..41436f06d 100644 --- a/.github/scripts/review-sync/tests/test-labels.js +++ b/.github/scripts/review-sync/tests/test-labels.js @@ -32,14 +32,14 @@ const unitTests = [ }, }, { - name: 'determineLabel: 2 write + 0 maintainer → queue:maintainers (NOT ready-to-merge)', + name: 'determineLabel: 2 write + 0 maintainer → queue:maintainers (NOT status: ready-to-merge)', test: () => { const r = determineLabel({ maintainerApproval: 0, writeApproval: 2, softApproval: 0, anyApproval: 2 }); return r.name === 'queue:maintainers'; }, }, { - name: 'determineLabel: 1 maintainer alone → queue:maintainers (NOT ready-to-merge, needs 2 reviews)', + name: 'determineLabel: 1 maintainer alone → queue:maintainers (NOT status: ready-to-merge, needs 2 reviews)', test: () => { // Sophie's edge case: maintainer approves first, only 1 total review const r = determineLabel({ maintainerApproval: 1, writeApproval: 0, softApproval: 0, anyApproval: 1 }); @@ -47,17 +47,17 @@ const unitTests = [ }, }, { - name: 'determineLabel: 1 maintainer + 1 write → ready-to-merge (2 reviews satisfied)', + name: 'determineLabel: 1 maintainer + 1 write → status: ready-to-merge (2 reviews satisfied)', test: () => { const r = determineLabel({ maintainerApproval: 1, writeApproval: 1, softApproval: 0, anyApproval: 2 }); - return r.name === 'ready-to-merge'; + return r.name === 'status: ready-to-merge'; }, }, { - name: 'determineLabel: 1 maintainer + 1 soft → ready-to-merge (2 reviews satisfied)', + name: 'determineLabel: 1 maintainer + 1 soft → status: ready-to-merge (2 reviews satisfied)', test: () => { const r = determineLabel({ maintainerApproval: 1, writeApproval: 0, softApproval: 1, anyApproval: 2 }); - return r.name === 'ready-to-merge'; + return r.name === 'status: ready-to-merge'; }, }, { @@ -121,7 +121,7 @@ const unitTests = [ ], }); const changed = await syncLabel(mock, 'o', 'r', { number: 1, labels: [{ name: 'queue:junior-committer' }] }, false); - return changed === true && mock.calls.labelsAdded.includes('ready-to-merge') && mock.calls.labelsRemoved.includes('queue:junior-committer'); + return changed === true && mock.calls.labelsAdded.includes('status: ready-to-merge') && mock.calls.labelsRemoved.includes('queue:junior-committer'); }, }, { From 96acaa5d458c80220fb24e90ecc707456753cc1d Mon Sep 17 00:00:00 2001 From: darshit2308 Date: Thu, 7 May 2026 13:03:24 +0530 Subject: [PATCH 07/10] chore: fix lingering comment referring to ready-to-merge Updates a JSDoc comment in constants.js that still referenced the old label name without the status: prefix. Signed-off-by: darshit2308 --- .github/scripts/review-sync/helpers/constants.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/scripts/review-sync/helpers/constants.js b/.github/scripts/review-sync/helpers/constants.js index df25c398c..f23dcf3f4 100644 --- a/.github/scripts/review-sync/helpers/constants.js +++ b/.github/scripts/review-sync/helpers/constants.js @@ -14,7 +14,7 @@ const RATE_LIMIT_FLOOR = 200; * The four mutually exclusive queue labels managed by this bot. * * Flow: - * queue:junior-committer → queue:committers → queue:maintainers → ready-to-merge + * queue:junior-committer → queue:committers → queue:maintainers → status: ready-to-merge */ const QUEUE_LABELS = { JUNIOR: { From 5b87b8926e0bdf96ca36dd185bfdfdce23ba2f87 Mon Sep 17 00:00:00 2001 From: darshit2308 Date: Thu, 7 May 2026 13:27:37 +0530 Subject: [PATCH 08/10] fix: throw on non-404 stale label removal errors Addresses CodeRabbit review feedback. Previously, if the API failed to remove a stale label (e.g. due to rate limits or network issues), the error was logged but execution continued. Since the new label was already added earlier in the function, this left the PR in an invalid state with multiple mutually-exclusive queue labels. Now, non-404 errors are explicitly re-thrown to fail the workflow and alert maintainers, preventing broken label states. Signed-off-by: darshit2308 --- .github/scripts/review-sync/helpers/labels.js | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/scripts/review-sync/helpers/labels.js b/.github/scripts/review-sync/helpers/labels.js index b7299e281..3210eb118 100644 --- a/.github/scripts/review-sync/helpers/labels.js +++ b/.github/scripts/review-sync/helpers/labels.js @@ -162,6 +162,7 @@ async function syncLabel(github, owner, repo, pr, dryRun) { } else { const message = error instanceof Error ? error.message : String(error); console.error(` ✗ Failed to remove "${stale}": ${message}`); + throw error; // Re-throw to prevent silently leaving PR in a broken multi-label state } } } From 8d48308349fdacce0942249fc7b75c23173db9f5 Mon Sep 17 00:00:00 2001 From: darshit2308 Date: Thu, 7 May 2026 14:49:58 +0530 Subject: [PATCH 09/10] 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 --- .github/scripts/review-sync/helpers/labels.js | 8 ++++---- .github/scripts/review-sync/index.js | 4 ++++ .github/scripts/review-sync/tests/test-labels.js | 4 ++-- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/.github/scripts/review-sync/helpers/labels.js b/.github/scripts/review-sync/helpers/labels.js index 3210eb118..a867465fe 100644 --- a/.github/scripts/review-sync/helpers/labels.js +++ b/.github/scripts/review-sync/helpers/labels.js @@ -57,20 +57,20 @@ async function ensureLabel(github, owner, repo, label, dryRun) { * Determine the correct queue label for a PR based on approval counts. * * Phase 1 logic (4-stage pipeline): - * maintainerApproval >= 1 AND anyApproval >= 2 → status: ready-to-merge (CODEOWNERS + min reviews) + * 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) * * Note: status: ready-to-merge requires BOTH a maintainer approval AND at least 2 - * total reviews. This prevents a single maintainer approval from marking - * a PR as status: ready-to-merge when branch protection requires 2+ reviews. + * total core reviews (maintainer or write). 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 * @returns {object} The correct QUEUE_LABELS entry */ function determineLabel(approvals) { - if (approvals.maintainerApproval >= 1 && approvals.anyApproval >= 2) { + if (approvals.maintainerApproval >= 1 && (approvals.maintainerApproval + approvals.writeApproval) >= 2) { return QUEUE_LABELS.MERGE; } if (approvals.writeApproval >= 1 || approvals.maintainerApproval >= 1) { diff --git a/.github/scripts/review-sync/index.js b/.github/scripts/review-sync/index.js index 1546ced9a..5fc13a34c 100644 --- a/.github/scripts/review-sync/index.js +++ b/.github/scripts/review-sync/index.js @@ -88,4 +88,8 @@ module.exports = async ({ github, context, core }) => { console.log(` Labels changed: ${changed}`); console.log(` Labels already correct: ${skipped}`); console.log(` Errors: ${errors}`); + + if (errors > 0) { + process.exitCode = 1; + } }; diff --git a/.github/scripts/review-sync/tests/test-labels.js b/.github/scripts/review-sync/tests/test-labels.js index 41436f06d..e04181dc2 100644 --- a/.github/scripts/review-sync/tests/test-labels.js +++ b/.github/scripts/review-sync/tests/test-labels.js @@ -54,10 +54,10 @@ const unitTests = [ }, }, { - name: 'determineLabel: 1 maintainer + 1 soft → status: ready-to-merge (2 reviews satisfied)', + name: 'determineLabel: 1 maintainer + 1 soft → queue:maintainers (soft approvals do not count as core review)', test: () => { const r = determineLabel({ maintainerApproval: 1, writeApproval: 0, softApproval: 1, anyApproval: 2 }); - return r.name === 'status: ready-to-merge'; + return r.name === 'queue:maintainers'; }, }, { From 4a028e6d9c628b175e7e67c4f61f2831f18cd9ca Mon Sep 17 00:00:00 2001 From: darshit2308 Date: Thu, 7 May 2026 16:53:20 +0530 Subject: [PATCH 10/10] style: fix trailing whitespace caught by pre-commit hook Signed-off-by: darshit2308 --- .github/scripts/review-sync/helpers/labels.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/scripts/review-sync/helpers/labels.js b/.github/scripts/review-sync/helpers/labels.js index a867465fe..596cebbeb 100644 --- a/.github/scripts/review-sync/helpers/labels.js +++ b/.github/scripts/review-sync/helpers/labels.js @@ -63,7 +63,7 @@ async function ensureLabel(github, owner, repo, label, dryRun) { * 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 + * total core reviews (maintainer or write). 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