diff --git a/.github/workflows/maintainer-approval.js b/.github/workflows/maintainer-approval.js index 93ce4de78c..0e0b798c54 100644 --- a/.github/workflows/maintainer-approval.js +++ b/.github/workflows/maintainer-approval.js @@ -36,6 +36,25 @@ async function isTeamMember(github, org, teamSlug, login, core) { } } +/** + * Reduce a PR's review history to each reviewer's current standing. + * + * The API returns every review ever submitted, in chronological order, so a + * reviewer who approves and later requests changes appears twice. Only their + * last review counts. COMMENTED reviews carry no approval state and leave the + * previous standing intact, matching how GitHub itself resolves the reviewers + * list. + */ +function latestReviews(reviews) { + const byReviewer = new Map(); + for (const review of reviews) { + const login = review.user?.login; + if (!login || review.state === "COMMENTED") continue; + byReviewer.set(login.toLowerCase(), review); + } + return Array.from(byReviewer.values()); +} + /** * Find which approver (if any) satisfies a group's ownership requirement. * Returns the login of the first matching approver, or null. @@ -461,11 +480,12 @@ module.exports = async ({ github, context, core }) => { name: STATUS_CONTEXT, }; - const reviews = await github.paginate(github.rest.pulls.listReviews, { + const reviewHistory = await github.paginate(github.rest.pulls.listReviews, { owner: context.repo.owner, repo: context.repo.repo, pull_number: context.issue.number, }); + const reviews = latestReviews(reviewHistory); // Maintainer approval -> success with simple comment const maintainerApproval = reviews.find( diff --git a/.github/workflows/maintainer-approval.test.js b/.github/workflows/maintainer-approval.test.js index 2866dc9d3d..378b0fcbc7 100644 --- a/.github/workflows/maintainer-approval.test.js +++ b/.github/workflows/maintainer-approval.test.js @@ -334,6 +334,88 @@ describe("maintainer-approval", () => { assert.equal(github._checkRuns.length, 0); }); + it("owner who approved then requested changes no longer approves", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "jefferycheng1" } }, + { state: "CHANGES_REQUESTED", user: { login: "jefferycheng1" } }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._checkRuns.length, 0); + }); + + it("maintainer who approved then requested changes no longer approves", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "maintainer1" } }, + { state: "CHANGES_REQUESTED", user: { login: "maintainer1" } }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._checkRuns.length, 0); + }); + + it("owner who requested changes then approved counts as approval", async () => { + const github = makeGithub({ + reviews: [ + { state: "CHANGES_REQUESTED", user: { login: "jefferycheng1" } }, + { state: "APPROVED", user: { login: "jefferycheng1" } }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._checkRuns.length, 1); + assert.equal(github._checkRuns[0].conclusion, "success"); + }); + + it("COMMENTED review after an approval leaves the approval standing", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "jefferycheng1" } }, + { state: "COMMENTED", user: { login: "jefferycheng1" } }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._checkRuns.length, 1); + assert.equal(github._checkRuns[0].conclusion, "success"); + }); + + it("dismissed approval no longer counts", async () => { + const github = makeGithub({ + reviews: [ + { state: "APPROVED", user: { login: "jefferycheng1" } }, + { state: "DISMISSED", user: { login: "jefferycheng1" } }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core = makeCore(); + const context = makeContext(); + + await runModule({ github, context, core }); + + assert.equal(github._checkRuns.length, 0); + }); + it("self-approval by PR author is excluded", async () => { const github = makeGithub({ reviews: [