From 5c8fd0a48c8fe5e1cac46005f612e118e9bf2047 Mon Sep 17 00:00:00 2001 From: chichuan Date: Fri, 7 Aug 2026 15:20:41 +0800 Subject: [PATCH] fix: preserve reviewer routing fallback --- .github/reviewer-routing.js | 60 +++++++++++++- .github/reviewer-routing.test.js | 79 ++++++++++++++++++- .github/workflows/reviewer-router.yml | 63 +++++++++------ test/scripts/reviewer_router_workflow_test.go | 9 ++- 4 files changed, 178 insertions(+), 33 deletions(-) diff --git a/.github/reviewer-routing.js b/.github/reviewer-routing.js index e1495ad5..0dd93ed8 100644 --- a/.github/reviewer-routing.js +++ b/.github/reviewer-routing.js @@ -185,6 +185,58 @@ function addReviewer(reviewers, reviewer) { } } +function reviewerCandidates({preferredReviewers, fallbackReviewers, eligibleReviewers}) { + const eligible = new Set(eligibleReviewers.map((reviewer) => reviewer.toLowerCase())); + const candidates = []; + for (const reviewer of [...preferredReviewers, ...fallbackReviewers]) { + if ( + eligible.has(reviewer.toLowerCase()) && + !candidates.some((candidate) => candidate.toLowerCase() === reviewer.toLowerCase()) + ) { + candidates.push(reviewer); + } + } + return candidates; +} + +async function requestReviewersWithFallback({ + candidates, + requiredReviewers, + satisfiedReviewers = [], + requestReviewer, + onFailure = () => {}, +}) { + const alreadySatisfied = new Set( + satisfiedReviewers.map((reviewer) => reviewer.toLowerCase()), + ); + const satisfied = new Set(); + const requested = []; + + for (const reviewer of candidates) { + if (satisfied.size >= requiredReviewers) { + break; + } + const normalizedReviewer = reviewer.toLowerCase(); + if (alreadySatisfied.has(normalizedReviewer)) { + satisfied.add(normalizedReviewer); + continue; + } + + try { + const shouldContinue = await requestReviewer(reviewer); + if (shouldContinue === false) { + return {requested, satisfiedReviewers: [...satisfied], aborted: true}; + } + requested.push(reviewer); + satisfied.add(normalizedReviewer); + } catch (error) { + onFailure(reviewer, error); + } + } + + return {requested, satisfiedReviewers: [...satisfied], aborted: false}; +} + function resolveReviewRouting({files, author, latestPusher, fallbackReviewers = REVIEWER_POOL}) { const modules = classifyFiles(files); const unavailable = new Set([author, latestPusher].filter(Boolean).map((login) => login.toLowerCase())); @@ -224,4 +276,10 @@ function resolveReviewRouting({files, author, latestPusher, fallbackReviewers = }; } -module.exports = {REVIEWER_POOL, classifyFiles, resolveReviewRouting}; +module.exports = { + REVIEWER_POOL, + classifyFiles, + requestReviewersWithFallback, + resolveReviewRouting, + reviewerCandidates, +}; diff --git a/.github/reviewer-routing.test.js b/.github/reviewer-routing.test.js index 49a6fa5d..3ad317fd 100644 --- a/.github/reviewer-routing.test.js +++ b/.github/reviewer-routing.test.js @@ -1,7 +1,11 @@ 'use strict'; const assert = require('node:assert/strict'); -const {resolveReviewRouting} = require('./reviewer-routing'); +const { + requestReviewersWithFallback, + resolveReviewRouting, + reviewerCandidates, +} = require('./reviewer-routing'); function route(files, author = 'author', latestPusher = author) { return resolveReviewRouting({files: files.map((filename) => ({filename})), author, latestPusher}); @@ -70,4 +74,75 @@ function route(files, author = 'author', latestPusher = author) { assert.equal(result.reason, 'unknown_paths'); } -console.log('reviewer routing policy tests passed'); +async function testSingleReviewerFallback() { + const candidates = reviewerCandidates({ + preferredReviewers: ['wxianfeng'], + fallbackReviewers: ['wxianfeng', 'typefield', 'haofeng0705'], + eligibleReviewers: ['wxianfeng', 'typefield', 'haofeng0705'], + }); + const attempts = []; + const result = await requestReviewersWithFallback({ + candidates, + requiredReviewers: 1, + requestReviewer: async (reviewer) => { + attempts.push(reviewer); + if (reviewer === 'wxianfeng') { + throw Object.assign(new Error('cannot request primary'), {status: 422}); + } + return true; + }, + }); + assert.deepEqual(attempts, ['wxianfeng', 'typefield']); + assert.deepEqual(result.requested, ['typefield']); + assert.equal(result.satisfiedReviewers.length, 1); +} + +async function testTwoReviewerFallback() { + const candidates = reviewerCandidates({ + preferredReviewers: ['haofeng0705', 'wxianfeng'], + fallbackReviewers: ['haofeng0705', 'wxianfeng', 'typefield', 'hlzjsong'], + eligibleReviewers: ['haofeng0705', 'wxianfeng', 'typefield', 'hlzjsong'], + }); + const attempts = []; + const result = await requestReviewersWithFallback({ + candidates, + requiredReviewers: 2, + requestReviewer: async (reviewer) => { + attempts.push(reviewer); + if (reviewer === 'wxianfeng') { + throw Object.assign(new Error('temporary failure'), {status: 503}); + } + return true; + }, + }); + assert.deepEqual(attempts, ['haofeng0705', 'wxianfeng', 'typefield']); + assert.deepEqual(result.requested, ['haofeng0705', 'typefield']); + assert.equal(result.satisfiedReviewers.length, 2); +} + +async function testLowerPriorityExistingRequestDoesNotReplaceOwner() { + const attempts = []; + const result = await requestReviewersWithFallback({ + candidates: ['wxianfeng', 'typefield'], + requiredReviewers: 1, + satisfiedReviewers: ['typefield'], + requestReviewer: async (reviewer) => { + attempts.push(reviewer); + return true; + }, + }); + assert.deepEqual(attempts, ['wxianfeng']); + assert.deepEqual(result.requested, ['wxianfeng']); + assert.deepEqual(result.satisfiedReviewers, ['wxianfeng']); +} + +Promise.all([ + testSingleReviewerFallback(), + testTwoReviewerFallback(), + testLowerPriorityExistingRequestDoesNotReplaceOwner(), +]) + .then(() => console.log('reviewer routing policy tests passed')) + .catch((error) => { + console.error(error); + process.exitCode = 1; + }); diff --git a/.github/workflows/reviewer-router.yml b/.github/workflows/reviewer-router.yml index b3f16459..105ac713 100644 --- a/.github/workflows/reviewer-router.yml +++ b/.github/workflows/reviewer-router.yml @@ -22,9 +22,10 @@ jobs: timeout-minutes: 5 steps: - name: Check out trusted routing policy - uses: actions/checkout@v4 + uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 with: ref: ${{ github.event.pull_request.base.sha }} + persist-credentials: false - name: Route review and enable auto-merge uses: actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b # v7.1.0 with: @@ -33,7 +34,12 @@ jobs: const repo = context.repo.repo; const pullNumber = context.payload.pull_request.number; const eventHeadSha = context.payload.pull_request.head.sha; - const {REVIEWER_POOL, resolveReviewRouting} = require('./.github/reviewer-routing.js'); + const { + REVIEWER_POOL, + requestReviewersWithFallback, + resolveReviewRouting, + reviewerCandidates, + } = require('./.github/reviewer-routing.js'); const reviewerPool = REVIEWER_POOL; async function getReadyEventPull(phase) { @@ -191,21 +197,12 @@ jobs: latestPusher, fallbackReviewers: ranked, }); - const desiredReviewers = []; - for (const reviewer of [...routing.reviewers, ...ranked]) { - if ( - desiredReviewers.length < routing.requiredReviewers && - eligible.some((candidate) => candidate.toLowerCase() === reviewer.toLowerCase()) && - !desiredReviewers.some((candidate) => candidate.toLowerCase() === reviewer.toLowerCase()) - ) { - desiredReviewers.push(reviewer); - } - } - const reviewersToRequest = desiredReviewers.filter( - reviewer => - !currentHeadReviewers.has(reviewer.toLowerCase()) && - !existingRequestedReviewers.has(reviewer.toLowerCase()), - ); + const candidates = reviewerCandidates({ + preferredReviewers: routing.reviewers, + fallbackReviewers: ranked, + eligibleReviewers: eligible, + }); + const desiredReviewers = candidates.slice(0, routing.requiredReviewers); if (routing.reason === 'unknown_paths' && currentHeadReviewers.size > 0) { core.info( `PR #${pullNumber} has a current-head review for unknown paths; leaving manual ownership unchanged.`, @@ -216,12 +213,20 @@ jobs: `PR #${pullNumber} routing: ${routing.reason}; modules=${routing.modules.map(module => module.id).join(',') || 'unknown'}; reviewers=${desiredReviewers.join(',') || 'load-balanced fallback'}.`, ); - const requestedByRouter = new Set(); - for (const reviewer of reviewersToRequest) { - try { + const satisfiedReviewers = new Set([ + ...currentHeadReviewers, + ...[...existingRequestedReviewers].filter((reviewer) => + candidates.some((candidate) => candidate.toLowerCase() === reviewer), + ), + ]); + const requestResult = await requestReviewersWithFallback({ + candidates, + requiredReviewers: routing.requiredReviewers, + satisfiedReviewers: [...satisfiedReviewers], + requestReviewer: async (reviewer) => { const currentPull = await getReadyEventPull('review request'); if (!currentPull) { - return; + return false; } await github.rest.pulls.requestReviewers({ owner, @@ -229,19 +234,25 @@ jobs: pull_number: pullNumber, reviewers: [reviewer], }); - requestedByRouter.add(reviewer.toLowerCase()); core.info( `Requested @${reviewer} for PR #${pullNumber} (open request load: ${loads.get(reviewer)}).`, ); - } catch (error) { + return true; + }, + onFailure: (reviewer, error) => { core.warning( `Could not request @${reviewer} for PR #${pullNumber}; trying the next candidate (${error.status || 'unknown status'}).`, ); - } + }, + }); + if (requestResult.aborted) { + return; } - if (requestedByRouter.size === 0) { - core.warning(`No reviewer request could be created for PR #${pullNumber}.`); + if (requestResult.satisfiedReviewers.length < routing.requiredReviewers) { + core.warning( + `Only ${requestResult.satisfiedReviewers.length} of ${routing.requiredReviewers} required reviewers could be satisfied for PR #${pullNumber}.`, + ); } } diff --git a/test/scripts/reviewer_router_workflow_test.go b/test/scripts/reviewer_router_workflow_test.go index cdc7572d..61fe102a 100644 --- a/test/scripts/reviewer_router_workflow_test.go +++ b/test/scripts/reviewer_router_workflow_test.go @@ -101,8 +101,9 @@ func TestReviewerRouterWorkflowContract(t *testing.T) { if job.If != "github.event.pull_request.draft == false" { t.Fatalf("route.if = %q, want non-draft guard", job.If) } + const checkoutSHA = "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683" const githubScriptSHA = "actions/github-script@f28e40c7f34bde8b3046d885e986cb6290c5673b" - if len(job.Steps) != 2 || job.Steps[0].Uses != "actions/checkout@v4" || job.Steps[0].With["ref"] != "${{ github.event.pull_request.base.sha }}" || job.Steps[1].Uses != githubScriptSHA { + if len(job.Steps) != 2 || job.Steps[0].Uses != checkoutSHA || job.Steps[0].With["ref"] != "${{ github.event.pull_request.base.sha }}" || job.Steps[0].With["persist-credentials"] != "false" || job.Steps[1].Uses != githubScriptSHA { t.Fatalf("route steps = %#v, want trusted base checkout followed by pinned github-script", job.Steps) } @@ -135,9 +136,9 @@ func TestReviewerRouterWorkflowContract(t *testing.T) { "loads.set(candidate, loads.get(candidate) + 1)", "loads.get(left) - loads.get(right)", "routing.requiredReviewers", - "reviewersToRequest", - "for (const reviewer of reviewersToRequest)", - "requestedByRouter", + "reviewerCandidates", + "requestReviewersWithFallback", + "satisfiedReviewers", "github.rest.pulls.requestReviewers", "trying the next candidate", "Reviewer routing hit an unexpected error",