mirror of
https://github.com/HDFGroup/hdf5.git
synced 2026-09-25 04:09:44 +03:00
Add change-request and manually-added-CODEOWNER lines to review checklist (#6528)
* 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.
This commit is contained in:
@@ -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 = '<!-- hdf5-review-checklist-manual:';
|
||||
const MANUAL_SUFFIX = '-->';
|
||||
|
||||
// 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<login, Set<filename>> 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<login, Set<filename>>, from
|
||||
// buildChangeRequestFileMap) and `manuallyAdded` (Set<login>, 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<login>, excludedReviewers: Set<login> }.
|
||||
// Returns { confirmedRequested: Set<login>, excludedReviewers: Set<login>,
|
||||
// manuallyAdded: Set<login> } — 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;
|
||||
|
||||
@@ -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('<!-- hdf5-review-checklist-v1 -->\nsome body text');
|
||||
assert.strictEqual(manuallyAdded.size, 0);
|
||||
});
|
||||
|
||||
test('parseManuallyAdded: extracts logins from the hidden marker', () => {
|
||||
const body = '<!-- hdf5-review-checklist-v1 -->\nbody\n<!-- hdf5-review-checklist-manual:alice,bob-->';
|
||||
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()), '<!-- hdf5-review-checklist-manual:-->');
|
||||
});
|
||||
|
||||
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
|
||||
// ----------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user