From 1fc781961d035a3cd0e7dab4ba08281eaaf99df9 Mon Sep 17 00:00:00 2001 From: Scot Breitenfeld Date: Thu, 16 Jul 2026 11:23:26 -0500 Subject: [PATCH] Add change-request and manually-added-CODEOWNER lines to review checklist (#6528) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add change-request and manually-added-CODEOWNER lines to review checklist Reviewers who request changes now get their own line under each area their inline comments touch, whether or not they're a CODEOWNER or a requested reviewer. CODEOWNERS who are review-requested directly by a human (rather than auto-picked) get a separate required-approval line, and their approval is now required for that area's sign-off in addition to the usual reviewer's. * Prune reviewers whose area dropped out of scope A CODEOWNER requested for an area the PR used to touch, before a later push narrowed the diff, was never removed: touchedAreaOwners only reflects areas touched right now, so none of the existing avalanche pruning ever considered them. Reproduced live on PR #6528, where the branch's diff shrank to just .github/ but gheber/fortnern/mattjala (owners of unrelated areas) stayed as requested reviewers. Manually-added CODEOWNERS are exempt, since a human's deliberate request shouldn't be silently undone by a later push. * review-checklist: fix avalanche exemption regressing genuine avalanches #6524 exempted a review_requested login's whole area from avalanche detection to stop a manual re-request from being undone. But a genuine CODEOWNERS avalanche's own review_requested sub-events look identical to that case, and one of them can survive the ready_for_review vs. review_requested concurrency race (#6530: draft PR marked ready for review left all 3 CODEOWNERS owners requested instead of pruning to one, because the surviving run's action was one of the avalanche's own review_requested events, exempting the whole area). Replace the exemption with a forced pick: the directly-requested login still always survives, but now by replacing the rest of that area's avalanche instead of exempting the area from pruning — so a genuine avalanche still collapses to one person. Also gate justRequestedLogin on !isBotSender, matching updatedManuallyAdded's existing reasoning just above it: the bot's own requestReviewers calls fire this same event and aren't a human decision. --- .github/scripts/review-checklist.js | 296 +++++++++++--- .github/scripts/review-checklist.test.js | 482 ++++++++++++++++++++++- 2 files changed, 718 insertions(+), 60 deletions(-) diff --git a/.github/scripts/review-checklist.js b/.github/scripts/review-checklist.js index 10143a2b434..d8a5ad71900 100644 --- a/.github/scripts/review-checklist.js +++ b/.github/scripts/review-checklist.js @@ -44,6 +44,37 @@ function withExcluded(commentBody, excluded) { return commentBody.slice(0, start) + marker + commentBody.slice(end + EXCLUDED_SUFFIX.length); } +// Persisted record of CODEOWNERS who were review-requested directly by a +// human (github.rest.pulls.requestReviewers called by the bot itself, or +// GitHub's own CODEOWNERS auto-assignment, both fire the identical +// review_requested webhook — see the isBotSender guard in +// coordinateReviewers for how a human's own action is told apart from +// those). A manually-added CODEOWNER is presumed deliberately chosen for +// their judgment, not just load-balanced into the slot — so buildBody +// requires their own approval for that area's sign-off, on top of (not +// instead of) whichever owner was auto-picked. Same durability rules as +// EXCLUDED_PREFIX: it's the only persistent storage available, so it rides +// along as a third hidden marker in the checklist comment body. +const MANUAL_PREFIX = ''; + +// Extracts the persisted manually-added-CODEOWNER list. Returns an empty Set +// if there's no comment yet or no marker in it. +function parseManuallyAdded(commentBody) { + if (!commentBody) return new Set(); + const start = commentBody.indexOf(MANUAL_PREFIX); + if (start === -1) return new Set(); + const end = commentBody.indexOf(MANUAL_SUFFIX, start); + if (end === -1) return new Set(); + const list = commentBody.slice(start + MANUAL_PREFIX.length, end); + return new Set(list.split(',').map(s => s.trim()).filter(Boolean)); +} + +// Serializes the manually-added-CODEOWNER list back into its hidden-marker form. +function serializeManuallyAdded(manuallyAdded) { + return `${MANUAL_PREFIX}${[...manuallyAdded].join(',')}${MANUAL_SUFFIX}`; +} + // ── Pure helpers ────────────────────────────────────────────────────────────── function labelFromPattern(pattern) { @@ -107,10 +138,10 @@ function attributeFiles(changedFileData, areas) { return filesByArea; } -// Returns Set of logins whose most-recent substantive review state is APPROVED. -// COMMENTED reviews are ignored — they don't change the approval state. -// A CHANGES_REQUESTED or DISMISSED review after an APPROVED one cancels the approval. -function computeApprovals(reviews) { +// Returns { login: state } of each reviewer's most-recent substantive review +// state (APPROVED, CHANGES_REQUESTED, or DISMISSED). COMMENTED reviews are +// ignored — they don't change the approval/change-request state. +function latestReviewStates(reviews) { const latest = {}; for (const review of reviews) { if (!review.user) continue; // ghost / deleted account @@ -119,13 +150,44 @@ function computeApprovals(reviews) { latest[review.user.login] = state; } } + return latest; +} + +// Returns Set of logins whose most-recent substantive review state is APPROVED. +// A CHANGES_REQUESTED or DISMISSED review after an APPROVED one cancels the approval. +function computeApprovals(reviews) { return new Set( - Object.entries(latest) + Object.entries(latestReviewStates(reviews)) .filter(([, s]) => s === 'APPROVED') .map(([login]) => login) ); } +// Returns Set of logins whose most-recent substantive review state is +// CHANGES_REQUESTED — i.e. reviewers with an outstanding, unresolved +// change request right now (a later APPROVED or DISMISSED review clears it). +function computeChangesRequested(reviews) { + return new Set( + Object.entries(latestReviewStates(reviews)) + .filter(([, s]) => s === 'CHANGES_REQUESTED') + .map(([login]) => login) + ); +} + +// Returns Map> of the files each reviewer left inline +// review comments on, restricted to reviewers in `changesRequestedUsers` — +// this is how buildBody knows which areas a change-requester's feedback +// actually falls under, rather than listing them against every area. +function buildChangeRequestFileMap(reviewComments, changesRequestedUsers) { + const map = new Map(); + for (const comment of reviewComments) { + if (!comment.user || !changesRequestedUsers.has(comment.user.login)) continue; + if (!map.has(comment.user.login)) map.set(comment.user.login, new Set()); + map.get(comment.user.login).add(comment.path); + } + return map; +} + // Pure reviewer selection. Returns { selected, updatedRequested, log }. // // `touchedAreas` entries must carry `.files` (array of file objects with @@ -197,7 +259,12 @@ function chooseReviewers(touchedAreas, { } // Builds the markdown checklist comment body (pure, no I/O). -function buildBody(touchedAreas, approvedUsers, confirmedRequested) { +// +// `changeRequestFilesByUser` (Map>, from +// buildChangeRequestFileMap) and `manuallyAdded` (Set, from +// parseManuallyAdded) are both optional — callers that don't track them can +// omit either and get the pre-existing behavior. +function buildBody(touchedAreas, approvedUsers, confirmedRequested, changeRequestFilesByUser = new Map(), manuallyAdded = new Set()) { // Reviewers manually assigned who are not CODEOWNERS for any touched area. // Used as a fallback for areas that have no CODEOWNER assigned — their // approval also counts as sign-off for that area. @@ -211,24 +278,51 @@ function buildBody(touchedAreas, approvedUsers, confirmedRequested) { const rowData = touchedAreas.map(area => { const ownerReviewers = area.owners.filter(o => confirmedRequested.has(o)); + // CODEOWNERS of this area who were review-requested directly by a human + // (see MANUAL_PREFIX) rather than auto-picked — their own approval is + // required in addition to the area's usual sign-off, shown on its own + // line below rather than folded into the main mention list. + const manualOwnersHere = ownerReviewers.filter(o => manuallyAdded.has(o)); + const autoOwnersHere = ownerReviewers.filter(o => !manuallyAdded.has(o)); // If no CODEOWNER is assigned for this area, fall back to non-CODEOWNER // reviewers so manually-assigned people are shown and their approval counts. const effectiveReviewers = ownerReviewers.length > 0 ? ownerReviewers : nonOwnerReviewers; if (ownerReviewers.length === 0) nonOwnerReviewers.forEach(o => usedAsFallback.add(o)); // Any owner's approval counts for sign-off, not only the assigned reviewer's. // Fall back to effectiveReviewers for areas with no CODEOWNER (non-owner assignee). - const approver = area.owners.find(o => approvedUsers.has(o)) + const approver = area.owners.find(o => approvedUsers.has(o)) || effectiveReviewers.find(o => approvedUsers.has(o)); - const signedOff = !!approver; + const allManualApproved = manualOwnersHere.every(o => approvedUsers.has(o)); + const signedOff = !!approver && allManualApproved; const box = signedOff ? 'x' : ' '; const tick = signedOff ? ' ✅' : ''; - // Signed off: show who approved. Pending: show all confirmed reviewers for - // this area (normally just the load-balanced pick, plus any CODEOWNER who - // was manually added on top of it). + // Signed off: show who approved. Pending: show the auto/fallback + // reviewers for this area — manually-added owners get their own line + // below instead, so they're left out of this list to avoid double-listing. + const pendingMentionPool = ownerReviewers.length > 0 ? autoOwnersHere : effectiveReviewers; const mention = approver ? ` — @${approver}` - : effectiveReviewers.length > 0 ? ` — ${effectiveReviewers.map(o => `@${o}`).join(', ')}` : ''; - return { text: `- [${box}] **${area.label}**${tick}${mention}`, signedOff }; + : pendingMentionPool.length > 0 ? ` — ${pendingMentionPool.map(o => `@${o}`).join(', ')}` : ''; + + const manualLines = manualOwnersHere.map(o => { + const approved = approvedUsers.has(o); + return ` - [${approved ? 'x' : ' '}] @${o} (manually added)${approved ? ' ✅' : ' — approval required'}`; + }); + + // Anyone with inline comments on a file attributed to this area, whose + // most-recent review is still CHANGES_REQUESTED — one line per person, + // regardless of whether they're a CODEOWNER or a drive-by reviewer. + const areaFilenames = new Set(area.files.map(f => f.filename)); + const changeRequesters = [...changeRequestFilesByUser.entries()] + .filter(([, files]) => [...files].some(f => areaFilenames.has(f))) + .map(([login]) => login) + .sort(); + const changeRequestLines = changeRequesters.map(login => ` - ⚠️ Changes requested by @${login}`); + + return { + text: [`- [${box}] **${area.label}**${tick}${mention}`, ...manualLines, ...changeRequestLines].join('\n'), + signedOff, + }; }); const allDone = rowData.every(r => r.signedOff); @@ -335,7 +429,10 @@ function planSynchronizeSwaps(eligibleAreas, allReviews, { // ── Reviewer coordination ───────────────────────────────────────────────────── // // Determines who should be in confirmedRequested (the checklist display set). -// Returns { confirmedRequested: Set, excludedReviewers: Set }. +// Returns { confirmedRequested: Set, excludedReviewers: Set, +// manuallyAdded: Set } — the last tracks CODEOWNERS requested +// directly by a human (see MANUAL_PREFIX), updated alongside excludedReviewers +// wherever a review_requested/review_request_removed event is inspected below. // The bot strips reviewers only in three deliberate cases; everywhere else it // is purely additive (fills in a load-balanced pick for uncovered areas only): // @@ -409,7 +506,7 @@ function planSynchronizeSwaps(eligibleAreas, allReviews, { // async function coordinateReviewers(github, context, core, { owner, repo, pr_number, prData, allCodeOwners, catchAllOwners, touchedAreas, reviewerLoad, - excludedReviewers, allReviews, hasExistingComment, LINE_THRESHOLD, AREA_THRESHOLDS, PUBLIC_HEADER, + excludedReviewers, manuallyAdded, allReviews, hasExistingComment, LINE_THRESHOLD, AREA_THRESHOLDS, PUBLIC_HEADER, }) { const pr = { owner, repo, pr_number }; const action = context.payload.action; @@ -440,23 +537,37 @@ async function coordinateReviewers(github, context, core, { // this PR again, even after a draft becomes ready for review. const isBotSender = context.payload.sender?.type === 'Bot'; const updatedExcluded = new Set(excludedReviewers); + // CODEOWNERS review-requested directly by a human — see MANUAL_PREFIX. + // Gated on !isBotSender same as the review_requested branch below: the + // bot's own requestReviewers calls (the normal load-balanced auto-pick) + // fire this identical webhook event with a bot sender, and must not be + // mistaken for a deliberate human choice. + const updatedManuallyAdded = new Set(manuallyAdded); if (action === 'review_request_removed' && context.payload.requested_reviewer && !isBotSender) { - updatedExcluded.add(context.payload.requested_reviewer.login); - core.info(`${context.payload.requested_reviewer.login} explicitly removed — excluding from future auto-reassignment`); - } else if (action === 'review_requested' && context.payload.requested_reviewer) { + const login = context.payload.requested_reviewer.login; + updatedExcluded.add(login); + updatedManuallyAdded.delete(login); + core.info(`${login} explicitly removed — excluding from future auto-reassignment`); + } else if (action === 'review_requested' && context.payload.requested_reviewer && !isBotSender) { const login = context.payload.requested_reviewer.login; if (updatedExcluded.delete(login)) { core.info(`${login} explicitly re-requested — clearing prior exclusion`); } + if (allCodeOwners.has(login)) { + updatedManuallyAdded.add(login); + core.info(`${login} manually requested by a human — their own approval will be required on areas they own`); + } } - // 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) + // The specific login a direct human 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. Gated on !isBotSender for the same + // reason as updatedManuallyAdded above — the bot's own requestReviewers + // calls fire this identical event and aren't a human decision. + const justRequestedLogin = (action === 'review_requested' && context.payload.requested_reviewer && !isBotSender) ? context.payload.requested_reviewer.login : null; @@ -467,7 +578,7 @@ async function coordinateReviewers(github, context, core, { // ── read-only events ───────────────────────────────────────────────────── if (context.eventName === 'pull_request_review' || context.eventName === 'workflow_run') { core.info('Read-only event — reflecting current reviewer assignments'); - return { confirmedRequested: new Set(existingRequested), excludedReviewers: updatedExcluded }; + return { confirmedRequested: new Set(existingRequested), excludedReviewers: updatedExcluded, manuallyAdded: updatedManuallyAdded }; } // Enforce the exclusion list against whatever's actually still on the PR — @@ -515,12 +626,35 @@ async function coordinateReviewers(github, context, core, { // judgment, not their path ownership) is never touched. await removeUnselected(github, core, pr, touchedAreaOwners, existingRequested, new Set()); core.info('Draft PR opened — clearing auto-assigned reviewers, deferring until ready for review'); - return { confirmedRequested: new Set(), excludedReviewers: updatedExcluded }; + return { confirmedRequested: new Set(), excludedReviewers: updatedExcluded, manuallyAdded: updatedManuallyAdded }; } // Any other event while draft (synchronize, review_requested, ...): // leave whoever's there alone, request no one new. core.info('Draft PR — leaving existing reviewer assignments untouched, no new requests while draft'); - return { confirmedRequested: new Set(existingRequested), excludedReviewers: updatedExcluded }; + return { confirmedRequested: new Set(existingRequested), excludedReviewers: updatedExcluded, manuallyAdded: updatedManuallyAdded }; + } + + // Scope-shrink pruning: a reviewer requested for an area this PR *used to* + // touch, before a later push narrowed the diff, is never in + // touchedAreaOwners (that set only reflects areas touched right now) — so + // none of the avalanche/first-pass pruning above ever considers removing + // them. Catch that here: any still-pending CODEOWNER (GitHub drops someone + // from requested_reviewers the moment they actually submit a review, so + // this can never strip a completed approval/change-request) who doesn't + // own any currently-touched area is stale. Manually-added CODEOWNERS are + // exempt — a human deliberately requesting them is not an artifact of a + // stale diff and must not be silently undone by a later push. + const outOfScopeOwners = [...existingRequested].filter( + o => allCodeOwners.has(o) && !touchedAreaOwners.has(o) && !updatedManuallyAdded.has(o) + ); + for (const login of outOfScopeOwners) { + try { + await github.rest.pulls.removeRequestedReviewers({ owner, repo, pull_number: pr_number, reviewers: [login] }); + existingRequested.delete(login); + core.info(`Removed ${login} — no longer owns any area this PR touches (scope shrank)`); + } catch (e) { + core.warning(`Could not remove out-of-scope reviewer ${login}: ${e.message}`); + } } // Excluded owners are dropped from each area's candidate pool first — an @@ -560,7 +694,7 @@ async function coordinateReviewers(github, context, core, { if (toRequest.size > 0) await requestReviewers(github, core, pr, toRequest); core.info(`Non-draft PR ${action} — pruned to load-balanced selection: ${[...selected].join(', ') || '(none)'}`); - return { confirmedRequested: selected, excludedReviewers: updatedExcluded }; + return { confirmedRequested: selected, excludedReviewers: updatedExcluded, manuallyAdded: updatedManuallyAdded }; } // Per-area avalanche detection: GitHub's CODEOWNERS engine re-fires whenever a @@ -571,15 +705,27 @@ 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. + // + // A genuine multi-owner CODEOWNERS avalanche and "someone just manually + // review_requested a specific login on top of an already-settled pick" + // produce an identical shape (an area with >1 owner currently requested) — + // this run's own action can't tell them apart (PR #6530: the ready_for_review + // avalanche's own review_requested sub-events raced the ready_for_review + // action itself via cancel-in-progress, and one of them survived). So + // justRequestedLogin's area is never exempted from pruning; instead it's + // forced as that area's kept pick — still collapses a real avalanche to + // one person, while guaranteeing a direct review_requested is never the + // one removed. 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, { + const forcedAreas = justRequestedLogin + ? avalancheAreas.filter(a => a.owners.includes(justRequestedLogin)) + : []; + const algorithmAreas = avalancheAreas.filter(a => !forcedAreas.includes(a)); + + const { selected: algoPicked, log: pruneLog } = chooseReviewers(algorithmAreas, { prAuthor, existingRequested: new Set(), // pick fresh: treat each area as uncovered reviewerLoad, @@ -587,6 +733,15 @@ async function coordinateReviewers(github, context, core, { }); for (const msg of pruneLog) core.info(msg); + const avalanchePruned = new Set(algoPicked); + if (forcedAreas.length > 0) { + avalanchePruned.add(justRequestedLogin); + core.info( + `Area(s) ${forcedAreas.map(a => a.label).join(', ')}: keeping explicitly ` + + `review_requested ${justRequestedLogin} instead of the load-balanced pick` + ); + } + const avalancheOwners = new Set(avalancheAreas.flatMap(a => a.owners)); // Keep the pruned single pick per area; leave non-avalanche owners untouched. const keepSet = new Set([ @@ -652,11 +807,11 @@ async function coordinateReviewers(github, context, core, { if (selected.size === 0) { core.info('Every touched area already has a reviewer — nothing to add'); - return { confirmedRequested: new Set(existingRequested), excludedReviewers: updatedExcluded }; + return { confirmedRequested: new Set(existingRequested), excludedReviewers: updatedExcluded, manuallyAdded: updatedManuallyAdded }; } const confirmed = await requestReviewers(github, core, pr, selected); - return { confirmedRequested: new Set([...existingRequested, ...confirmed]), excludedReviewers: updatedExcluded }; + return { confirmedRequested: new Set([...existingRequested, ...confirmed]), excludedReviewers: updatedExcluded, manuallyAdded: updatedManuallyAdded }; } // ── Entry point ─────────────────────────────────────────────────────────────── @@ -786,13 +941,15 @@ module.exports = async function run({ github, context, core }) { }); const stale = allComments.find(c => c.body.includes(MARKER)); if (stale) { - // Preserve the exclusion list even though there's nothing to check off - // right now — it should still apply if this PR touches tracked areas again. - const preservedExcluded = serializeExcluded(parseExcluded(stale.body)); + // Preserve the exclusion and manually-added lists even though there's + // nothing to check off right now — they should still apply if this + // PR touches tracked areas again. + const preservedExcluded = serializeExcluded(parseExcluded(stale.body)); + const preservedManuallyAdded = serializeManuallyAdded(parseManuallyAdded(stale.body)); await github.rest.issues.updateComment({ owner, repo, comment_id: stale.id, body: MARKER + '\n_No CODEOWNERS-tracked areas are touched by this PR — no review checklist required._' - + '\n' + preservedExcluded, + + '\n' + preservedExcluded + '\n' + preservedManuallyAdded, }); core.info(`Cleared stale checklist comment #${stale.id}`); } @@ -813,7 +970,23 @@ module.exports = async function run({ github, context, core }) { } catch (error) { core.warning(`Failed to fetch reviews; approval state may be stale: ${error.message}`); } - const approvedUsers = computeApprovals(allReviews); + const approvedUsers = computeApprovals(allReviews); + const changesRequestedBy = computeChangesRequested(allReviews); + + // Inline comments, so a change-requester's row-line can be scoped to the + // area(s) their feedback actually falls under. Only fetched when someone's + // outstanding review state is CHANGES_REQUESTED — nothing to attribute otherwise. + let changeRequestFilesByUser = new Map(); + if (changesRequestedBy.size > 0) { + try { + const reviewComments = await github.paginate(github.rest.pulls.listReviewComments, { + owner, repo, pull_number: pr_number, per_page: 100, + }); + changeRequestFilesByUser = buildChangeRequestFileMap(reviewComments, changesRequestedBy); + } catch (error) { + core.warning(`Failed to fetch review comments; change-request lines may be incomplete: ${error.message}`); + } + } let prData; try { @@ -858,22 +1031,27 @@ module.exports = async function run({ github, context, core }) { commentFetchFailed = true; } const excludedReviewers = parseExcluded(existingComment && existingComment.body); + const manuallyAdded = parseManuallyAdded(existingComment && existingComment.body); // On a fetch failure we genuinely don't know whether a comment exists — // default to true (assume it does) so coordinateReviewers falls back to its // non-destructive additive-fill path rather than treating an API hiccup as // "first coordination pass" and pruning an established PR's reviewers. const hasExistingComment = commentFetchFailed ? true : !!existingComment; - const { confirmedRequested, excludedReviewers: updatedExcluded } = await coordinateReviewers(github, context, core, { + const { + confirmedRequested, + excludedReviewers: updatedExcluded, + manuallyAdded: updatedManuallyAdded, + } = await coordinateReviewers(github, context, core, { owner, repo, pr_number, prData, allCodeOwners, catchAllOwners, touchedAreas, reviewerLoad, - excludedReviewers, allReviews, hasExistingComment, LINE_THRESHOLD, AREA_THRESHOLDS, PUBLIC_HEADER, + excludedReviewers, manuallyAdded, allReviews, hasExistingComment, LINE_THRESHOLD, AREA_THRESHOLDS, PUBLIC_HEADER, }); // ---------------------------------------------------------------- // 8. Build and post (or update) the checklist comment. // ---------------------------------------------------------------- - const body = buildBody(touchedAreas, approvedUsers, confirmedRequested) + - '\n' + serializeExcluded(updatedExcluded); + const body = buildBody(touchedAreas, approvedUsers, confirmedRequested, changeRequestFilesByUser, updatedManuallyAdded) + + '\n' + serializeExcluded(updatedExcluded) + '\n' + serializeManuallyAdded(updatedManuallyAdded); try { if (existingComment) { @@ -888,15 +1066,19 @@ module.exports = async function run({ github, context, core }) { } }; -module.exports.MARKER = MARKER; -module.exports.matchesPattern = matchesPattern; -module.exports.labelFromPattern = labelFromPattern; -module.exports.attributeFiles = attributeFiles; -module.exports.computeApprovals = computeApprovals; -module.exports.chooseReviewers = chooseReviewers; -module.exports.buildBody = buildBody; -module.exports.parseExcluded = parseExcluded; -module.exports.serializeExcluded = serializeExcluded; -module.exports.withExcluded = withExcluded; -module.exports.coordinateReviewers = coordinateReviewers; -module.exports.planSynchronizeSwaps = planSynchronizeSwaps; +module.exports.MARKER = MARKER; +module.exports.matchesPattern = matchesPattern; +module.exports.labelFromPattern = labelFromPattern; +module.exports.attributeFiles = attributeFiles; +module.exports.computeApprovals = computeApprovals; +module.exports.computeChangesRequested = computeChangesRequested; +module.exports.buildChangeRequestFileMap = buildChangeRequestFileMap; +module.exports.chooseReviewers = chooseReviewers; +module.exports.buildBody = buildBody; +module.exports.parseExcluded = parseExcluded; +module.exports.serializeExcluded = serializeExcluded; +module.exports.withExcluded = withExcluded; +module.exports.parseManuallyAdded = parseManuallyAdded; +module.exports.serializeManuallyAdded = serializeManuallyAdded; +module.exports.coordinateReviewers = coordinateReviewers; +module.exports.planSynchronizeSwaps = planSynchronizeSwaps; diff --git a/.github/scripts/review-checklist.test.js b/.github/scripts/review-checklist.test.js index 2e9951d1b9f..2b218abc265 100644 --- a/.github/scripts/review-checklist.test.js +++ b/.github/scripts/review-checklist.test.js @@ -8,11 +8,15 @@ const { labelFromPattern, attributeFiles, computeApprovals, + computeChangesRequested, + buildChangeRequestFileMap, chooseReviewers, buildBody, parseExcluded, serializeExcluded, withExcluded, + parseManuallyAdded, + serializeManuallyAdded, planSynchronizeSwaps, coordinateReviewers, } = require('./review-checklist.js'); @@ -248,6 +252,86 @@ test('computeApprovals: independent approvals from two users', () => { assert.ok(approved.has('bob')); }); +// ---------------------------------------------------------------- +// computeChangesRequested +// ---------------------------------------------------------------- + +test('computeChangesRequested: basic change request', () => { + const changesRequested = computeChangesRequested([{ user: { login: 'alice' }, state: 'CHANGES_REQUESTED' }]); + assert.ok(changesRequested.has('alice')); +}); + +test('computeChangesRequested: APPROVED after CHANGES_REQUESTED clears it', () => { + const changesRequested = computeChangesRequested([ + { user: { login: 'alice' }, state: 'CHANGES_REQUESTED' }, + { user: { login: 'alice' }, state: 'APPROVED' }, + ]); + assert.strictEqual(changesRequested.has('alice'), false); +}); + +test('computeChangesRequested: DISMISSED after CHANGES_REQUESTED clears it', () => { + const changesRequested = computeChangesRequested([ + { user: { login: 'alice' }, state: 'CHANGES_REQUESTED' }, + { user: { login: 'alice' }, state: 'DISMISSED' }, + ]); + assert.strictEqual(changesRequested.has('alice'), false); +}); + +test('computeChangesRequested: drive-by reviewer (never a requested reviewer) still counts', () => { + const changesRequested = computeChangesRequested([ + { user: { login: 'driveby' }, state: 'CHANGES_REQUESTED' }, + ]); + assert.ok(changesRequested.has('driveby')); +}); + +test('computeChangesRequested: null user is skipped (ghost / deleted account)', () => { + const changesRequested = computeChangesRequested([ + { user: null, state: 'CHANGES_REQUESTED' }, + { user: { login: 'bob' }, state: 'CHANGES_REQUESTED' }, + ]); + assert.strictEqual(changesRequested.size, 1); + assert.ok(changesRequested.has('bob')); +}); + +// ---------------------------------------------------------------- +// buildChangeRequestFileMap +// ---------------------------------------------------------------- + +test('buildChangeRequestFileMap: maps a change-requester to their commented files', () => { + const map = buildChangeRequestFileMap( + [{ user: { login: 'alice' }, path: 'src/H5F.c' }], + new Set(['alice']) + ); + assert.ok(map.get('alice').has('src/H5F.c')); +}); + +test('buildChangeRequestFileMap: excludes commenters not in changesRequestedUsers', () => { + const map = buildChangeRequestFileMap( + [{ user: { login: 'bob' }, path: 'src/H5F.c' }], + new Set(['alice']) + ); + assert.strictEqual(map.has('bob'), false); +}); + +test('buildChangeRequestFileMap: skips comments with a null user (ghost / deleted account)', () => { + const map = buildChangeRequestFileMap( + [{ user: null, path: 'src/H5F.c' }], + new Set(['alice']) + ); + assert.strictEqual(map.size, 0); +}); + +test('buildChangeRequestFileMap: aggregates multiple files from the same reviewer', () => { + const map = buildChangeRequestFileMap( + [ + { user: { login: 'alice' }, path: 'src/H5F.c' }, + { user: { login: 'alice' }, path: 'src/H5D.c' }, + ], + new Set(['alice']) + ); + assert.strictEqual(map.get('alice').size, 2); +}); + // ---------------------------------------------------------------- // chooseReviewers helpers // ---------------------------------------------------------------- @@ -495,6 +579,123 @@ test('buildBody: reviewer used as no-CODEOWNER fallback is not double-listed as assert.ok(body.includes('— @charlie')); }); +// Change-requester sub-lines: someone with an outstanding CHANGES_REQUESTED +// review is listed on a separate line under each area their inline comments +// touch — whether or not they're a CODEOWNER or a requested reviewer at all. + +test('buildBody: change-requester on an area file gets a sub-line under that area', () => { + const areas = [makeArea('src', ['alice'], 10, [{ filename: 'src/H5F.c', changes: 10 }])]; + const changeRequestFiles = new Map([['dan', new Set(['src/H5F.c'])]]); + const body = buildBody(areas, new Set(), new Set(['alice']), changeRequestFiles); + assert.ok(body.includes(' - ⚠️ Changes requested by @dan')); +}); + +test('buildBody: drive-by change-requester (not a CODEOWNER or requested reviewer) still gets a sub-line', () => { + const areas = [makeArea('src', ['alice'], 10, [{ filename: 'src/H5F.c', changes: 10 }])]; + const changeRequestFiles = new Map([['driveby', new Set(['src/H5F.c'])]]); + const body = buildBody(areas, new Set(), new Set(['alice']), changeRequestFiles); + assert.ok(body.includes(' - ⚠️ Changes requested by @driveby')); +}); + +test('buildBody: change-requester is scoped to the area their comments touch, not every area', () => { + const areas = [ + makeArea('src', ['alice'], 10, [{ filename: 'src/H5F.c', changes: 10 }]), + makeArea('fortran', ['bob'], 10, [{ filename: 'fortran/H5f.F90', changes: 10 }]), + ]; + const changeRequestFiles = new Map([['dan', new Set(['src/H5F.c'])]]); + const body = buildBody(areas, new Set(), new Set(['alice', 'bob']), changeRequestFiles); + const lines = body.split('\n'); + const srcIdx = lines.findIndex(l => l.includes('**src**')); + const fortranIdx = lines.findIndex(l => l.includes('**fortran**')); + assert.ok(lines[srcIdx + 1].includes('⚠️ Changes requested by @dan')); + assert.ok(!lines[fortranIdx + 1] || !lines[fortranIdx + 1].includes('@dan')); +}); + +test('buildBody: multiple change-requesters on the same area each get their own line', () => { + const areas = [makeArea('src', ['alice'], 10, [{ filename: 'src/H5F.c', changes: 10 }])]; + const changeRequestFiles = new Map([ + ['dan', new Set(['src/H5F.c'])], + ['erin', new Set(['src/H5F.c'])], + ]); + const body = buildBody(areas, new Set(), new Set(['alice']), changeRequestFiles); + assert.ok(body.includes(' - ⚠️ Changes requested by @dan')); + assert.ok(body.includes(' - ⚠️ Changes requested by @erin')); +}); + +test('buildBody: no change-requesters produces no sub-lines and omitting the param is safe', () => { + const areas = [makeArea('src', ['alice'], 10, [{ filename: 'src/H5F.c', changes: 10 }])]; + const body = buildBody(areas, new Set(), new Set(['alice'])); + assert.ok(!body.includes('⚠️')); +}); + +test('buildBody: change-requester with no commented files in any touched area gets no sub-line', () => { + const areas = [makeArea('src', ['alice'], 10, [{ filename: 'src/H5F.c', changes: 10 }])]; + const changeRequestFiles = new Map([['dan', new Set(['unrelated/file.c'])]]); + const body = buildBody(areas, new Set(), new Set(['alice']), changeRequestFiles); + assert.ok(!body.includes('⚠️')); +}); + +// Manually-added-CODEOWNER sub-lines: a CODEOWNER who was review-requested +// directly by a human (rather than auto-picked) gets their own required- +// approval line, and the area doesn't sign off until they've approved too. + +test('buildBody: manually-added CODEOWNER gets a separate required-approval line', () => { + const areas = [makeArea('src', ['alice', 'bob'], 10)]; + const body = buildBody(areas, new Set(), new Set(['alice', 'bob']), new Map(), new Set(['bob'])); + assert.ok(body.includes(' - [ ] @bob (manually added) — approval required')); +}); + +test('buildBody: manually-added CODEOWNER is left out of the main mention line', () => { + const areas = [makeArea('src', ['alice', 'bob'], 10)]; + const body = buildBody(areas, new Set(), new Set(['alice', 'bob']), new Map(), new Set(['bob'])); + const mainRow = body.split('\n').find(l => l.startsWith('- [')); + assert.ok(mainRow.includes('— @alice')); + assert.ok(!mainRow.includes('@bob')); +}); + +test('buildBody: area does not sign off when auto-pick approved but manually-added CODEOWNER has not', () => { + const areas = [makeArea('src', ['alice', 'bob'], 10)]; + const body = buildBody(areas, new Set(['alice']), new Set(['alice', 'bob']), new Map(), new Set(['bob'])); + const mainRow = body.split('\n').find(l => l.startsWith('- [')); + assert.ok(mainRow.startsWith('- [ ] **src**')); + assert.ok(!mainRow.includes('✅')); + assert.ok(body.includes(' - [ ] @bob (manually added) — approval required')); +}); + +test('buildBody: area signs off once both the auto-pick and the manually-added CODEOWNER approve', () => { + const areas = [makeArea('src', ['alice', 'bob'], 10)]; + const body = buildBody(areas, new Set(['alice', 'bob']), new Set(['alice', 'bob']), new Map(), new Set(['bob'])); + const mainRow = body.split('\n').find(l => l.startsWith('- [')); + assert.ok(mainRow.startsWith('- [x] **src** ✅')); + assert.ok(body.includes(' - [x] @bob (manually added) ✅')); +}); + +test('buildBody: no manually-added CODEOWNERS produces no sub-lines and omitting the param is safe', () => { + const areas = [makeArea('src', ['alice'], 10)]; + const body = buildBody(areas, new Set(), new Set(['alice'])); + assert.ok(!body.includes('manually added')); +}); + +test('buildBody: manually-added sole owner of an area only needs their own approval', () => { + // alice is the only owner of this area and was manually requested — no + // separate auto-pick exists, so her own approval alone should sign it off. + const areas = [makeArea('src', ['alice'], 10)]; + const body = buildBody(areas, new Set(['alice']), new Set(['alice']), new Map(), new Set(['alice'])); + const mainRow = body.split('\n').find(l => l.startsWith('- [')); + assert.ok(mainRow.startsWith('- [x] **src** ✅')); + assert.ok(body.includes(' - [x] @alice (manually added) ✅')); +}); + +test('buildBody: a manually-added CODEOWNER who is no longer in confirmedRequested (removed) gets no line', () => { + // bob was manually added at some point (still in the persisted marker) but + // has since been removed from the PR — confirmedRequested no longer has + // him, so he must not show up as still owing an approval. + const areas = [makeArea('src', ['alice', 'bob'], 10)]; + const body = buildBody(areas, new Set(), new Set(['alice']), new Map(), new Set(['bob'])); + assert.ok(!body.includes('bob')); + assert.ok(!body.includes('manually added')); +}); + // ---------------------------------------------------------------- // parseExcluded / serializeExcluded — persisted "explicitly removed" list // ---------------------------------------------------------------- @@ -561,6 +762,39 @@ test('withExcluded: round-trips an empty set to the empty marker', () => { assert.strictEqual(parseExcluded(updated).size, 0); }); +// ---------------------------------------------------------------- +// parseManuallyAdded / serializeManuallyAdded — persisted "manually +// requested CODEOWNER" list (mirrors parseExcluded / serializeExcluded). +// ---------------------------------------------------------------- + +test('parseManuallyAdded: no comment body yet returns an empty set', () => { + const manuallyAdded = parseManuallyAdded(undefined); + assert.strictEqual(manuallyAdded.size, 0); +}); + +test('parseManuallyAdded: comment with no manual marker returns an empty set', () => { + const manuallyAdded = parseManuallyAdded('\nsome body text'); + assert.strictEqual(manuallyAdded.size, 0); +}); + +test('parseManuallyAdded: extracts logins from the hidden marker', () => { + const body = '\nbody\n'; + const manuallyAdded = parseManuallyAdded(body); + assert.ok(manuallyAdded.has('alice')); + assert.ok(manuallyAdded.has('bob')); + assert.strictEqual(manuallyAdded.size, 2); +}); + +test('serializeManuallyAdded: empty set produces the empty marker', () => { + assert.strictEqual(serializeManuallyAdded(new Set()), ''); +}); + +test('serializeManuallyAdded: round-trips through parseManuallyAdded', () => { + const original = new Set(['alice', 'bob']); + const roundTripped = parseManuallyAdded(serializeManuallyAdded(original)); + assert.deepStrictEqual([...roundTripped].sort(), ['alice', 'bob']); +}); + // ---------------------------------------------------------------- // planSynchronizeSwaps // ---------------------------------------------------------------- @@ -898,6 +1132,85 @@ asyncTest('coordinateReviewers: synchronize with one owner per area does not pru assert.strictEqual(github.calls.removeRequestedReviewers.length, 0); }); +// ---------------------------------------------------------------- +// coordinateReviewers — scope-shrink pruning (PR #6528 scenario): a +// CODEOWNER requested for an area the PR used to touch, before a later push +// narrowed the diff down, is never in touchedAreaOwners (that set only +// reflects areas touched right now) — so none of the avalanche/first-pass +// pruning considers removing them without this dedicated check. +// ---------------------------------------------------------------- + +asyncTest('coordinateReviewers: reviewer whose area dropped out of scope is removed', async () => { + const github = makeGithubMock(); + const context = { + eventName: 'pull_request_target', + payload: { action: 'synchronize', sender: { type: 'User' } }, + }; + const args = makeCoordinateBaseArgs({ + // gheber owns /docs/, not the .github area this fixture's touchedAreas + // covers — modeling a push that dropped doc changes from the diff. + allCodeOwners: new Set(['hyoklee', 'lrknox', 'jhendersonHDF', 'glennsong09', 'gheber']), + prData: { + user: { login: 'lrknox' }, + draft: false, + requested_reviewers: [{ login: 'hyoklee' }, { login: 'gheber' }], + }, + }); + + const { confirmedRequested } = await coordinateReviewers(github, context, makeCore(), args); + + assert.ok(github.calls.removeRequestedReviewers.includes('gheber')); + assert.ok(!confirmedRequested.has('gheber')); + assert.ok(confirmedRequested.has('hyoklee')); +}); + +asyncTest('coordinateReviewers: manually-added CODEOWNER survives scope-shrink pruning', async () => { + // gheber's area is out of scope same as above, but a human deliberately + // requested him directly — that must not be undone by a later push. + const github = makeGithubMock(); + const context = { + eventName: 'pull_request_target', + payload: { action: 'synchronize', sender: { type: 'User' } }, + }; + const args = makeCoordinateBaseArgs({ + allCodeOwners: new Set(['hyoklee', 'lrknox', 'jhendersonHDF', 'glennsong09', 'gheber']), + manuallyAdded: new Set(['gheber']), + prData: { + user: { login: 'lrknox' }, + draft: false, + requested_reviewers: [{ login: 'hyoklee' }, { login: 'gheber' }], + }, + }); + + const { confirmedRequested } = await coordinateReviewers(github, context, makeCore(), args); + + assert.ok(!github.calls.removeRequestedReviewers.includes('gheber')); + assert.ok(confirmedRequested.has('gheber')); +}); + +asyncTest('coordinateReviewers: non-CODEOWNER reviewer is untouched by scope-shrink pruning', async () => { + // driveby isn't a repo CODEOWNER at all (e.g. manually added for judgment, + // not path ownership) — scope-shrink pruning only concerns itself with + // CODEOWNERS-based auto-assignment, same as every other pruning path here. + const github = makeGithubMock(); + const context = { + eventName: 'pull_request_target', + payload: { action: 'synchronize', sender: { type: 'User' } }, + }; + const args = makeCoordinateBaseArgs({ + prData: { + user: { login: 'lrknox' }, + draft: false, + requested_reviewers: [{ login: 'hyoklee' }, { login: 'driveby' }], + }, + }); + + const { confirmedRequested } = await coordinateReviewers(github, context, makeCore(), args); + + assert.ok(!github.calls.removeRequestedReviewers.includes('driveby')); + assert.ok(confirmedRequested.has('driveby')); +}); + // ---------------------------------------------------------------- // coordinateReviewers — a direct review_requested must survive avalanche // detection on the very same run (reported bug: manually re-requesting a @@ -905,10 +1218,12 @@ asyncTest('coordinateReviewers: synchronize with one owner per area does not pru // 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). +// the just-requested login is carved out). The area is still pruned to one +// pick, same as any other avalanche — it's just forced to be the login that +// was directly requested, rather than the load-balancer's own choice. // ---------------------------------------------------------------- -asyncTest('coordinateReviewers: review_requested for a specific login is not undone by avalanche pruning', async () => { +asyncTest('coordinateReviewers: review_requested for a specific login becomes that area\'s forced pick', async () => { const github = makeGithubMock(); const context = { eventName: 'pull_request_target', @@ -934,8 +1249,76 @@ asyncTest('coordinateReviewers: review_requested for a specific login is not und const { confirmedRequested } = await coordinateReviewers(github, context, makeCore(), args); - assert.strictEqual(github.calls.removeRequestedReviewers.length, 0, 'Nothing should be removed'); + assert.deepStrictEqual(github.calls.removeRequestedReviewers, ['hyoklee'], 'The old pick is swapped out'); assert.ok(confirmedRequested.has('jhendersonHDF'), 'The just-requested reviewer must stay'); + assert.ok(!confirmedRequested.has('hyoklee'), 'The area keeps exactly one reviewer'); +}); + +// ---------------------------------------------------------------- +// coordinateReviewers — a genuine multi-owner avalanche must still prune to +// one even when the surviving run's own action is one of the avalanche's own +// review_requested sub-events (PR #6530: ready_for_review fired the +// ready_for_review action plus one review_requested per CODEOWNERS owner, +// all near-simultaneously; concurrency: cancel-in-progress let one of the +// review_requested runs survive instead of ready_for_review itself. Since a +// checklist comment already existed from the draft-open phase, that survivor +// fell through past the ready_for_review-only pruning branch entirely). +// ---------------------------------------------------------------- + +asyncTest('coordinateReviewers: review_requested surviving a ready_for_review avalanche still prunes to one', 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, + // GitHub's own CODEOWNERS avalanche assigned all 3 non-author owners — + // never pruned, since the surviving run isn't the ready_for_review action. + requested_reviewers: [{ login: 'hyoklee' }, { login: 'jhendersonHDF' }, { login: 'glennsong09' }], + }, + }); + + const { confirmedRequested } = await coordinateReviewers(github, context, makeCore(), args); + + assert.strictEqual(confirmedRequested.size, 1, 'Should prune to exactly one reviewer'); + assert.ok(confirmedRequested.has('jhendersonHDF'), 'The directly-requested login wins the forced pick'); + assert.strictEqual(github.calls.removeRequestedReviewers.length, 2, 'The other two avalanche owners are removed'); +}); + +asyncTest('coordinateReviewers: bot-sourced review_requested is not treated as a forced pick', async () => { + // The bot's own requestReviewers calls (e.g. re-requesting a dismissed + // reviewer, or its normal load-balanced auto-pick) fire this identical + // review_requested event with a Bot sender — must not be mistaken for a + // deliberate human override the way a User-sent one is. + const github = makeGithubMock(); + const context = { + eventName: 'pull_request_target', + payload: { + action: 'review_requested', + requested_reviewer: { login: 'jhendersonHDF' }, + sender: { type: 'Bot' }, + }, + }; + const args = makeCoordinateBaseArgs({ + prData: { + user: { login: 'lrknox' }, + draft: false, + requested_reviewers: [{ login: 'hyoklee' }, { login: 'jhendersonHDF' }, { login: 'glennsong09' }], + }, + reviewerLoad: { hyoklee: 0, jhendersonHDF: 99, glennsong09: 0 }, + }); + + const { confirmedRequested } = await coordinateReviewers(github, context, makeCore(), args); + + assert.strictEqual(confirmedRequested.size, 1, 'Should still prune to exactly one reviewer'); + assert.ok(confirmedRequested.has('hyoklee'), 'The normal load-balanced pick wins, not the bot-sourced login'); }); // ---------------------------------------------------------------- @@ -986,6 +1369,99 @@ asyncTest('coordinateReviewers: human-sender review_request_removed does persist assert.ok(excludedReviewers.has('jhendersonHDF')); }); +// ---------------------------------------------------------------- +// coordinateReviewers — manually-added-CODEOWNER tracking. A human directly +// review-requesting a CODEOWNER (as opposed to the bot's own load-balanced +// requestReviewers call, or GitHub's CODEOWNERS auto-assignment surviving +// the cancel-in-progress race) marks them as needing their own approval — +// see MANUAL_PREFIX and the buildBody tests above. +// ---------------------------------------------------------------- + +function makeManualAddContext(senderType, login) { + return { + eventName: 'pull_request_target', + payload: { + action: 'review_requested', + requested_reviewer: { login }, + sender: { type: senderType }, + }, + }; +} + +asyncTest('coordinateReviewers: human review_requested for a CODEOWNER marks them manually-added', async () => { + const github = makeGithubMock(); + const args = makeCoordinateBaseArgs({ + hasExistingComment: true, + prData: { + user: { login: 'lrknox' }, + draft: false, + requested_reviewers: [{ login: 'jhendersonHDF' }], // one .github owner — no avalanche + }, + }); + + const { manuallyAdded } = await coordinateReviewers( + github, makeManualAddContext('User', 'jhendersonHDF'), makeCore(), args + ); + + assert.ok(manuallyAdded.has('jhendersonHDF')); +}); + +asyncTest('coordinateReviewers: bot-sender review_requested does NOT mark the reviewer manually-added', async () => { + // The bot's own load-balanced requestReviewers call fires this identical + // webhook event with a bot sender — must not be mistaken for a human's + // deliberate choice, or every auto-picked owner would end up requiring + // a redundant "manually added" approval. + const github = makeGithubMock(); + const args = makeCoordinateBaseArgs({ + hasExistingComment: true, + prData: { + user: { login: 'lrknox' }, + draft: false, + requested_reviewers: [{ login: 'jhendersonHDF' }], + }, + }); + + const { manuallyAdded } = await coordinateReviewers( + github, makeManualAddContext('Bot', 'jhendersonHDF'), makeCore(), args + ); + + assert.ok(!manuallyAdded.has('jhendersonHDF')); +}); + +asyncTest('coordinateReviewers: review_requested for a non-CODEOWNER does not mark them manually-added', async () => { + const github = makeGithubMock(); + const args = makeCoordinateBaseArgs({ + hasExistingComment: true, + prData: { + user: { login: 'lrknox' }, + draft: false, + requested_reviewers: [{ login: 'jhendersonHDF' }, { login: 'driveby' }], + }, + }); + + const { manuallyAdded } = await coordinateReviewers( + github, makeManualAddContext('User', 'driveby'), makeCore(), args + ); + + assert.ok(!manuallyAdded.has('driveby')); +}); + +asyncTest('coordinateReviewers: human-sender review_request_removed clears a prior manually-added flag', async () => { + const github = makeGithubMock(); + const args = makeCoordinateBaseArgs({ + manuallyAdded: new Set(['jhendersonHDF']), + prData: { + user: { login: 'lrknox' }, + draft: false, + requested_reviewers: [{ login: 'hyoklee' }, { login: 'glennsong09' }], + }, + }); + + const { manuallyAdded } = await coordinateReviewers(github, makeRemovalContext('User'), makeCore(), args); + + assert.ok(!manuallyAdded.has('jhendersonHDF')); +}); + // ---------------------------------------------------------------- // Summary // ----------------------------------------------------------------