mirror of
https://github.com/HDFGroup/hdf5.git
synced 2026-09-25 04:09:44 +03:00
ci(review-checklist): fix three CODEOWNERS reviewer-avalanche bugs (#6485)
Three distinct but related bugs caused the review-checklist bot to @ mention more reviewers than intended. **Bug 1 — bot-sender guard (sticky exclusion)** The bot's own removeRequestedReviewers API calls fire review_request_removed events that self-trigger the workflow. Without a guard, that self-triggered run treated its own bookkeeping removal as a deliberate human decision and added the login to the persisted exclusion set permanently. Fix: check sender.type === 'Bot' and skip the exclusion update for bot-originated removals. **Bug 2 — cancel-in-progress race on opened/ready_for_review** GitHub fires one review_requested event per CODEOWNERS auto-assigned owner; each triggers a workflow run. With cancel-in-progress, the surviving run may be a review_requested rather than the opened event, bypassing the avalanche-prune branch entirely and leaving all CODEOWNERS requested. Fix: track hasExistingComment as a proxy for "first coordination pass" — if no checklist comment exists yet, prune regardless of which action survives the race. **Bug 3 — per-area avalanche on synchronize (PR #6484)** GitHub's CODEOWNERS engine re-fires when a commit first touches a new CODEOWNERS-covered area, not only on PR open but also on synchronize. The surviving run fell through to additive fill, saw the area as "already has owners, skip", and listed all auto-assigned CODEOWNERS. Fix: before the synchronize-swap and additive-fill paths, detect any area whose owner-list ∩ existingRequested > 1 (per-area avalanche) and prune that area to the single load-balanced pick. 84 tests (was 82).
This commit is contained in:
@@ -553,6 +553,45 @@ async function coordinateReviewers(github, context, core, {
|
||||
return { confirmedRequested: selected, excludedReviewers: updatedExcluded };
|
||||
}
|
||||
|
||||
// Per-area avalanche detection: GitHub's CODEOWNERS engine re-fires whenever a
|
||||
// commit first touches a new CODEOWNERS-covered area — not only on PR open,
|
||||
// but also on synchronize. If multiple owners of the same area are currently
|
||||
// requested, that's an auto-assignment avalanche that was never pruned. Reduce
|
||||
// each such area to the single load-balanced pick now, before the
|
||||
// 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.)
|
||||
const avalancheAreas = eligibleAreas.filter(
|
||||
area => area.owners.filter(o => existingRequested.has(o)).length > 1
|
||||
);
|
||||
if (avalancheAreas.length > 0) {
|
||||
const { selected: avalanchePruned, log: pruneLog } = chooseReviewers(avalancheAreas, {
|
||||
prAuthor,
|
||||
existingRequested: new Set(), // pick fresh: treat each area as uncovered
|
||||
reviewerLoad,
|
||||
LINE_THRESHOLD, AREA_THRESHOLDS, PUBLIC_HEADER,
|
||||
});
|
||||
for (const msg of pruneLog) core.info(msg);
|
||||
|
||||
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([
|
||||
...[...existingRequested].filter(r => !avalancheOwners.has(r)),
|
||||
...avalanchePruned,
|
||||
]);
|
||||
await removeUnselected(github, core, pr, avalancheOwners, existingRequested, keepSet);
|
||||
|
||||
// Update existingRequested so the swap and additive-fill steps see the
|
||||
// post-prune state — not the stale avalanche.
|
||||
for (const login of avalancheOwners) existingRequested.delete(login);
|
||||
for (const login of avalanchePruned) existingRequested.add(login);
|
||||
|
||||
core.info(
|
||||
`Pruned per-area CODEOWNERS avalanche — area(s): ${avalancheAreas.map(a => a.label).join(', ')}; ` +
|
||||
`kept: ${[...avalanchePruned].join(', ') || '(none)'}`
|
||||
);
|
||||
}
|
||||
|
||||
// Synchronize: a new commit dismissed a prior reviewer's approval.
|
||||
// Re-request that reviewer instead of keeping a fresh CODEOWNERS pick —
|
||||
// they already have context and only need to see what changed.
|
||||
|
||||
@@ -772,20 +772,26 @@ asyncTest('coordinateReviewers: ready_for_review on a draft-opened PR (still dra
|
||||
assert.deepStrictEqual([...confirmedRequested].sort(), ['glennsong09', 'hyoklee', 'jhendersonHDF']);
|
||||
});
|
||||
|
||||
asyncTest('coordinateReviewers: plain synchronize (no dismissed reviews) is left to additive fill, not pruned', async () => {
|
||||
// Contrast case: a synchronize with no dismissed reviews must NOT trigger
|
||||
// avalanche-style pruning — only opened/reopened/ready_for_review do. All
|
||||
// three avalanche-style owners stay; nothing is removed, nothing new is
|
||||
// requested since the area is already covered.
|
||||
asyncTest('coordinateReviewers: plain synchronize (no dismissed reviews, no avalanche) is left to additive fill', async () => {
|
||||
// When only one CODEOWNER is requested per area (normal steady state after
|
||||
// prior pruning), a plain synchronize with no dismissed reviews must stay on
|
||||
// the additive-fill path: the area is already covered, nothing is removed,
|
||||
// nothing new is requested.
|
||||
const github = makeGithubMock();
|
||||
const context = { eventName: 'pull_request_target', payload: { action: 'synchronize', sender: { type: 'User' } } };
|
||||
const args = makeCoordinateBaseArgs();
|
||||
const args = makeCoordinateBaseArgs({
|
||||
prData: {
|
||||
user: { login: 'lrknox' },
|
||||
draft: false,
|
||||
requested_reviewers: [{ login: 'hyoklee' }], // one .github owner — normal steady state
|
||||
},
|
||||
});
|
||||
|
||||
const { confirmedRequested } = await coordinateReviewers(github, context, makeCore(), args);
|
||||
|
||||
assert.strictEqual(github.calls.removeRequestedReviewers.length, 0);
|
||||
assert.strictEqual(github.calls.requestReviewers.length, 0);
|
||||
assert.deepStrictEqual([...confirmedRequested].sort(), ['glennsong09', 'hyoklee', 'jhendersonHDF']);
|
||||
assert.ok(confirmedRequested.has('hyoklee'));
|
||||
});
|
||||
|
||||
asyncTest('coordinateReviewers: review_requested survives the opened race and still prunes (PR #6479 scenario)', async () => {
|
||||
@@ -814,17 +820,82 @@ asyncTest('coordinateReviewers: review_requested on an already-established PR is
|
||||
// later in the PR's life (e.g. a human manually adding a reviewer) must stay
|
||||
// on the additive-fill path — it must not be reinterpreted as "first
|
||||
// coordination pass" and prune reviewers an established PR already has.
|
||||
// Use one requested reviewer per area so the per-area avalanche detector
|
||||
// does not also fire — this isolates the isFirstCoordinationPass behavior.
|
||||
const github = makeGithubMock();
|
||||
const context = {
|
||||
eventName: 'pull_request_target',
|
||||
payload: { action: 'review_requested', requested_reviewer: { login: 'jhendersonHDF' }, sender: { type: 'User' } },
|
||||
payload: { action: 'review_requested', requested_reviewer: { login: 'hyoklee' }, sender: { type: 'User' } },
|
||||
};
|
||||
const args = makeCoordinateBaseArgs({ hasExistingComment: true });
|
||||
const args = makeCoordinateBaseArgs({
|
||||
hasExistingComment: true,
|
||||
prData: {
|
||||
user: { login: 'lrknox' },
|
||||
draft: false,
|
||||
requested_reviewers: [{ login: 'hyoklee' }], // one .github owner — no avalanche to detect
|
||||
},
|
||||
});
|
||||
|
||||
const { confirmedRequested } = await coordinateReviewers(github, context, makeCore(), args);
|
||||
|
||||
assert.strictEqual(github.calls.removeRequestedReviewers.length, 0);
|
||||
assert.deepStrictEqual([...confirmedRequested].sort(), ['glennsong09', 'hyoklee', 'jhendersonHDF']);
|
||||
assert.ok(confirmedRequested.has('hyoklee'));
|
||||
});
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
// coordinateReviewers — per-area CODEOWNERS avalanche detection (PR #6484)
|
||||
//
|
||||
// When a synchronize push first touches a new CODEOWNERS area, GitHub
|
||||
// auto-assigns ALL that area's owners simultaneously. The surviving
|
||||
// review_requested run (after cancel-in-progress) may then fall into
|
||||
// the additive-fill path, see the area as "already has owners → skip",
|
||||
// and leave all of them listed. The per-area avalanche detector must
|
||||
// prune that area to one load-balanced pick even on synchronize.
|
||||
// ----------------------------------------------------------------
|
||||
|
||||
asyncTest('coordinateReviewers: synchronize with per-area avalanche prunes the area to one pick', async () => {
|
||||
// Model PR #6484: PR has an existing checklist (hasExistingComment: true),
|
||||
// a synchronize push touched a new area (.github), GitHub assigned 3 of its
|
||||
// owners, the surviving run must prune to one.
|
||||
const github = makeGithubMock();
|
||||
const context = {
|
||||
eventName: 'pull_request_target',
|
||||
payload: { action: 'synchronize', sender: { type: 'User' } },
|
||||
};
|
||||
// Default args: 3 .github owners in requested_reviewers, hasExistingComment: true.
|
||||
// That satisfies "existing PR + multiple CODEOWNERS for same area" = avalanche.
|
||||
const args = makeCoordinateBaseArgs();
|
||||
|
||||
const { confirmedRequested } = await coordinateReviewers(github, context, makeCore(), args);
|
||||
|
||||
assert.strictEqual(confirmedRequested.size, 1, 'Should prune to exactly one reviewer');
|
||||
// Exactly 2 removed (the 2 non-picked avalanche owners).
|
||||
assert.strictEqual(github.calls.removeRequestedReviewers.length, 2);
|
||||
// The kept reviewer is never re-requested (already on the PR).
|
||||
assert.strictEqual(github.calls.requestReviewers.length, 0);
|
||||
});
|
||||
|
||||
asyncTest('coordinateReviewers: synchronize with one owner per area does not prune', async () => {
|
||||
// Contrast: when each area already has exactly one CODEOWNER requested
|
||||
// (normal steady state), synchronize must NOT trigger avalanche pruning.
|
||||
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' }], // only one .github owner — no avalanche
|
||||
},
|
||||
});
|
||||
|
||||
const { confirmedRequested } = await coordinateReviewers(github, context, makeCore(), args);
|
||||
|
||||
// hyoklee stays, nothing pruned.
|
||||
assert.ok(confirmedRequested.has('hyoklee'));
|
||||
assert.strictEqual(github.calls.removeRequestedReviewers.length, 0);
|
||||
});
|
||||
|
||||
// ----------------------------------------------------------------
|
||||
|
||||
Reference in New Issue
Block a user