From b31e756e8e845d584e2683568053c54adbdd5691 Mon Sep 17 00:00:00 2001 From: navareamol <28789438+navareamol@users.noreply.github.com> Date: Mon, 9 Nov 2026 20:42:07 -0800 Subject: [PATCH] ci(board): add Changes requested status to the board sweep A ready PR whose reviewer asked for changes used to go back to In Progress, next to drafts. The board could not show which PRs were waiting on their author after a review. The sweep now moves such a PR to a new Changes requested status. It sits between In Progress and Needs review. When the author asks every blocking reviewer to look again, the PR moves to Needs review as before. Drafts stay In Progress. Issues follow their furthest linked PR, so an issue whose only PR has changes requested shows Changes requested. Tests: 59 pass, 8 of them new or changed. 8 fail against the old code. The privacy test checks the new status name never reaches the public log. --- .github/scripts/project-status-sweep.js | 13 ++-- .github/scripts/project-status-sweep.test.js | 69 +++++++++++++++++--- 2 files changed, 70 insertions(+), 12 deletions(-) diff --git a/.github/scripts/project-status-sweep.js b/.github/scripts/project-status-sweep.js index 4789172c5..8cff03641 100644 --- a/.github/scripts/project-status-sweep.js +++ b/.github/scripts/project-status-sweep.js @@ -8,7 +8,10 @@ * * Todo issue with no assignee and no trusted open linked PR * In Progress issue assigned or with a trusted open linked PR; any PR that - * is a draft or has an outstanding changes-requested verdict + * is a draft + * Changes requested + * ready PR with a changes-requested verdict from a reviewer + * who has not been asked to review again: waits on the author * Needs review PR that is ready (not a draft) and waits on a reviewer: no * review from another person yet, or its changes-requested * reviewers have all been asked to review again @@ -24,7 +27,7 @@ * It never touches: * - closed or merged items (the board's built-in workflows set Done) * - draft (board-only), inaccessible and archived items - * - a card whose Status is not one of the four above (e.g. Done, Blocked) + * - a card whose Status is not one of the five above (e.g. Done, Blocked) * - a card whose "Auto status" field is "Off" (a permanent hold) * - a card a person moved by hand, until the issue, the PR, or the issue's * trusted linked PR changes again. "By hand" is read from the item's @@ -54,7 +57,7 @@ const HOLD_FIELD = 'Auto status'; const HOLD_OFF = 'Off'; const HEARTBEAT_PREFIX = 'Board sweep:'; // Statuses the sweep manages, lowest to highest. -const MANAGED = ['Todo', 'In Progress', 'Needs review', 'Review']; +const MANAGED = ['Todo', 'In Progress', 'Changes requested', 'Needs review', 'Review']; const TRUSTED = new Set(['OWNER', 'MEMBER', 'COLLABORATOR']); const REVIEW_STATES = new Set(['APPROVED', 'CHANGES_REQUESTED', 'COMMENTED', 'DISMISSED']); // Built-in board workflows that also set Status and would fight the sweep. @@ -206,7 +209,9 @@ function prStatus(pr) { const blocking = verdicts.filter( (r) => r.state === 'CHANGES_REQUESTED' && !requested.has(r.author.login), ); - if (blocking.length > 0) return 'In Progress'; + // A reviewer asked for changes and the author has not asked them back yet: + // the ball is with the author. + if (blocking.length > 0) return 'Changes requested'; // Every changes-requested reviewer was asked to look again: the ball is // back with a reviewer. if (verdicts.some((r) => r.state === 'CHANGES_REQUESTED')) return 'Needs review'; diff --git a/.github/scripts/project-status-sweep.test.js b/.github/scripts/project-status-sweep.test.js index 8504b4806..cbaf978c0 100644 --- a/.github/scripts/project-status-sweep.test.js +++ b/.github/scripts/project-status-sweep.test.js @@ -87,7 +87,7 @@ function fakeGithub({ pages, workflows = [], failUpdates = 0, linkedById = {}, f }; } -const OPTIONS = ['Todo', 'In Progress', 'Needs review', 'Review', 'Done'].map((name) => ({ id: `OPT_${name}`, name })); +const OPTIONS = ['Todo', 'In Progress', 'Changes requested', 'Needs review', 'Review', 'Done'].map((name) => ({ id: `OPT_${name}`, name })); const boardPage = (nodes, { hasNextPage = false, remaining = 4000, field = { id: 'F', options: OPTIONS } } = {}) => ({ rateLimit: { cost: 9, remaining }, @@ -151,19 +151,38 @@ test('an approval moves a PR to Review', () => { assert.equal(prStatus(pr({ verdicts: { nodes: [verdict(user('bob'), 'APPROVED')] } })), 'Review'); }); -test('changes requested sends a PR back to In Progress', () => { +test('changes requested moves a PR to Changes requested', () => { const p = pr({ verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] }, reviews: { nodes: [review(user('bob'), 'CHANGES_REQUESTED'), review(user('alice'), 'COMMENTED')] }, }); - assert.equal(prStatus(p), 'In Progress'); + assert.equal(prStatus(p), 'Changes requested'); }); test('one approval does not outweigh another reviewer requesting changes', () => { const p = pr({ verdicts: { nodes: [verdict(user('bob'), 'APPROVED'), verdict(user('cat'), 'CHANGES_REQUESTED')] } }); + assert.equal(prStatus(p), 'Changes requested'); +}); + +test('a draft PR with changes requested stays In Progress', () => { + const p = pr({ isDraft: true, verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] } }); assert.equal(prStatus(p), 'In Progress'); }); +test('a bot requesting changes does not move a PR to Changes requested', () => { + assert.equal(prStatus(pr({ verdicts: { nodes: [verdict(bot('some-bot'), 'CHANGES_REQUESTED')] } })), 'Needs review'); +}); + +test("the author's own changes-requested verdict does not count", () => { + assert.equal(prStatus(pr({ verdicts: { nodes: [verdict(user('alice'), 'CHANGES_REQUESTED')] } })), 'Needs review'); +}); + +test('a dismissed changes-requested review (no longer a verdict) is Review, not Changes requested', () => { + // GitHub drops a dismissed review from latestOpinionatedReviews; it stays in reviews. + const p = pr({ reviews: { nodes: [review(user('bob'), 'DISMISSED')] } }); + assert.equal(prStatus(p), 'Review'); +}); + test('re-requesting review from the reviewer who asked for changes moves it to Needs review', () => { const p = pr({ verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] }, @@ -184,12 +203,12 @@ test('a draft PR with no reviews stays In Progress', () => { assert.equal(prStatus(pr({ isDraft: true })), 'In Progress'); }); -test('re-requesting only some blocking reviewers keeps In Progress', () => { +test('re-requesting only some blocking reviewers keeps Changes requested', () => { const p = pr({ verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED'), verdict(user('cat'), 'CHANGES_REQUESTED')] }, reviewRequests: { nodes: [request('bob')] }, }); - assert.equal(prStatus(p), 'In Progress'); + assert.equal(prStatus(p), 'Changes requested'); }); test('a stale changes-requested verdict is not lost behind many later comments', () => { @@ -197,7 +216,7 @@ test('a stale changes-requested verdict is not lost behind many later comments', verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] }, reviews: { nodes: Array.from({ length: 30 }, () => review(user('bob'), 'COMMENTED')) }, }); - assert.equal(prStatus(p), 'In Progress'); + assert.equal(prStatus(p), 'Changes requested'); }); test('a PR whose only review was dismissed counts as reviewed', () => { @@ -244,6 +263,17 @@ test('an issue takes the furthest of its linked PRs', () => { assert.equal(issueStatus(i), 'Review'); }); +test('issue follows its trusted linked PR into Changes requested', () => { + const blocked = pr({ verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] } }); + assert.equal(issueStatus(issue({ assignees: { totalCount: 1 }, linked: linked(blocked) })), 'Changes requested'); +}); + +test('an issue with one PR waiting on a reviewer and one on its author shows Needs review', () => { + const blocked = pr({ id: 'PR_2', verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] } }); + const i = issue({ linked: { totalCount: 2, nodes: [blocked, pr()] } }); + assert.equal(issueStatus(i), 'Needs review'); +}); + test('a merged or closed linked PR is ignored', () => { assert.equal(issueStatus(issue({ linked: linked(pr({ state: 'MERGED' })) })), 'Todo'); }); @@ -359,6 +389,7 @@ test('the public log names no issue, PR, status or draft; the private card has t item(pr({ number: 501, verdicts: { nodes: [verdict(user('bob'), 'APPROVED')] } }), 'Todo'), item(issue({ number: 602, assignees: { totalCount: 1 } }), 'Todo'), item(pr({ number: 703 }), 'Todo'), + item(pr({ number: 804, verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] } }), 'Todo'), item({ __typename: 'DraftIssue', id: 'D1', title: 'Secret launch plan' }, 'Todo'), item(null, 'Todo'), ]; @@ -369,11 +400,11 @@ test('the public log names no issue, PR, status or draft; the private card has t }); const { core } = await run(gh); const publicLog = [...core.info, ...core.warning, ...core.failed].join('\n'); - for (const leak of ['#501', '#602', '#703', 'Todo', 'In Progress', 'Needs review', 'Review', 'review', 'draft', 'Secret', 'linked to issue', 'inaccessible', 'boom']) { + for (const leak of ['#501', '#602', '#703', '#804', 'Todo', 'In Progress', 'Changes requested', 'Needs review', 'Review', 'review', 'draft', 'Secret', 'linked to issue', 'inaccessible', 'boom']) { assert.ok(!publicLog.includes(leak), `public log leaked "${leak}": ${publicLog}`); } const card = gh.writes.find((w) => w.kind === 'heartbeat-add'); - for (const detail of ['PR #501', 'issue #602', 'PR #703: Todo -> Needs review', 'Pull request linked to issue', 'draft item 1', 'boom']) { + for (const detail of ['PR #501', 'issue #602', 'PR #703: Todo -> Needs review', 'PR #804: Todo -> Changes requested', 'Pull request linked to issue', 'draft item 1', 'boom']) { assert.ok(card.body.includes(detail), `card is missing "${detail}"`); } }); @@ -418,6 +449,28 @@ test('a renamed status option is reported, not fatal', async () => { assert.equal(core.failed.length, 0); }); +test('before the Changes requested column exists, those cards stay put and the run is degraded', async () => { + const field = { id: 'F', options: OPTIONS.filter((o) => o.name !== 'Changes requested') }; + const blocked = pr({ number: 5, verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] } }); + const gh = fakeGithub({ pages: [boardPage([item(blocked, 'In Progress'), item(pr({ number: 6 }), 'Todo')], { field })] }); + const { result, core } = await run(gh); + assert.equal(result.outcome, 'degraded'); + assert.ok(result.problems.some((m) => m.includes('PR #5') && m.includes('no "Changes requested" status option'))); + assert.deepEqual(gh.writes.filter((w) => w.kind === 'status').map((w) => w.item), ['ITEM_6']); + assert.equal(core.failed.length, 0); +}); + +test('a card already in Changes requested is managed, and moves on once review is re-requested', async () => { + const back = pr({ + number: 5, + verdicts: { nodes: [verdict(user('bob'), 'CHANGES_REQUESTED')] }, + reviewRequests: { nodes: [request('bob')] }, + }); + const gh = fakeGithub({ pages: [boardPage([item(back, 'Changes requested')])] }); + const { result } = await run(gh); + assert.deepEqual(result.changes.map((c) => [c.label, c.from, c.to]), [['PR #5', 'Changes requested', 'Needs review']]); +}); + test('a missing Status field changes nothing and is reported', async () => { const gh = fakeGithub({ pages: [boardPage([item(pr({ number: 5 }), 'Todo')], { field: null })] }); const { result } = await run(gh);