From 50465b5f6a382b0ec100b27b46150fb4880c6ac2 Mon Sep 17 00:00:00 2001 From: Scot Breitenfeld Date: Mon, 13 Jul 2026 17:21:15 -0500 Subject: [PATCH] review-checklist: exempt a just-requested reviewer from avalanche pruning (#6524) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A direct review_requested (e.g. manually re-requesting a reviewer via the GitHub UI) adds a second currently-requested owner to that reviewer's area — indistinguishable from an unpruned CODEOWNERS avalanche, so the avalanche-detection pass immediately removed them again on the same run. Carve out the login this run's review_requested action names before running avalanche detection. --- .github/scripts/review-checklist.js | 14 +++++++++ .github/scripts/review-checklist.test.js | 40 ++++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/.github/scripts/review-checklist.js b/.github/scripts/review-checklist.js index e20cbf53a0a..10143a2b434 100644 --- a/.github/scripts/review-checklist.js +++ b/.github/scripts/review-checklist.js @@ -450,6 +450,16 @@ async function coordinateReviewers(github, context, core, { } } + // The specific login a direct review_requested action just added, if any — + // see its use below in avalanche detection. A deliberate re-request must + // survive this same run: it lands existingRequested at two owners for that + // login's area (them plus whoever an earlier pruning pass already picked), + // which is indistinguishable from an unpruned CODEOWNERS avalanche unless + // this login is carved out. + const justRequestedLogin = (action === 'review_requested' && context.payload.requested_reviewer) + ? context.payload.requested_reviewer.login + : null; + const existingRequested = new Set( prData.requested_reviewers.map(r => r.login).filter(Boolean).filter(l => !updatedExcluded.has(l)) ); @@ -561,8 +571,12 @@ async function coordinateReviewers(github, context, core, { // synchronize-swap or additive-fill logic runs, so those paths see an already- // correct one-per-area baseline. (PR #6484: user pushed a commit that first // touched .github; GitHub assigned all 4 .github CODEOWNERS simultaneously.) + // justRequestedLogin's area is exempted — a human just deliberately asked + // for exactly that person, which looks identical to an avalanche (two + // owners now requested) but isn't one. const avalancheAreas = eligibleAreas.filter( area => area.owners.filter(o => existingRequested.has(o)).length > 1 + && !(justRequestedLogin && area.owners.includes(justRequestedLogin)) ); if (avalancheAreas.length > 0) { const { selected: avalanchePruned, log: pruneLog } = chooseReviewers(avalancheAreas, { diff --git a/.github/scripts/review-checklist.test.js b/.github/scripts/review-checklist.test.js index 82ecb2fbf6f..2e9951d1b9f 100644 --- a/.github/scripts/review-checklist.test.js +++ b/.github/scripts/review-checklist.test.js @@ -898,6 +898,46 @@ asyncTest('coordinateReviewers: synchronize with one owner per area does not pru assert.strictEqual(github.calls.removeRequestedReviewers.length, 0); }); +// ---------------------------------------------------------------- +// coordinateReviewers — a direct review_requested must survive avalanche +// detection on the very same run (reported bug: manually re-requesting a +// reviewer via the GitHub UI got them immediately removed again, because +// their area now had two currently-requested owners — themselves plus +// whichever pick an earlier ready_for_review pruning pass had already made +// — which is indistinguishable from an unpruned CODEOWNERS avalanche unless +// the just-requested login is carved out). +// ---------------------------------------------------------------- + +asyncTest('coordinateReviewers: review_requested for a specific login is not undone by avalanche pruning', async () => { + const github = makeGithubMock(); + const context = { + eventName: 'pull_request_target', + payload: { + action: 'review_requested', + requested_reviewer: { login: 'jhendersonHDF' }, + sender: { type: 'User' }, + }, + }; + const args = makeCoordinateBaseArgs({ + prData: { + user: { login: 'lrknox' }, + draft: false, + // hyoklee is the load-balanced pick an earlier pass already made; + // jhendersonHDF was just manually re-requested on top of it. + requested_reviewers: [{ login: 'hyoklee' }, { login: 'jhendersonHDF' }], + }, + // Rig the load-balancer so a fresh pick would land on hyoklee, not + // jhendersonHDF — proving jhendersonHDF survives because they were + // just requested, not because they'd have won the pick anyway. + reviewerLoad: { hyoklee: 0, jhendersonHDF: 99, glennsong09: 0 }, + }); + + const { confirmedRequested } = await coordinateReviewers(github, context, makeCore(), args); + + assert.strictEqual(github.calls.removeRequestedReviewers.length, 0, 'Nothing should be removed'); + assert.ok(confirmedRequested.has('jhendersonHDF'), 'The just-requested reviewer must stay'); +}); + // ---------------------------------------------------------------- // coordinateReviewers — bot-self-triggered review_request_removed must not // create a sticky exclusion (the bot's own removeUnselected/removeRequestedReviewers