diff --git a/.github/workflows/maintainer-approval.js b/.github/workflows/maintainer-approval.js index 38371f8cdcb..9b7fe91a03a 100644 --- a/.github/workflows/maintainer-approval.js +++ b/.github/workflows/maintainer-approval.js @@ -57,6 +57,44 @@ async function findGroupApprover(owners, approverLogins, github, org, core) { return null; } +const OPINIONATED_REVIEW_STATES = new Set(["APPROVED", "CHANGES_REQUESTED"]); + +// GitHub returns all reviews, but only opinionated reviews affect approval. +function latestReviewsByUser(reviews) { + const latest = new Map(); + + for (const [index, review] of reviews.entries()) { + if (!OPINIONATED_REVIEW_STATES.has(review.state)) continue; + + const login = review.user?.login?.toLowerCase(); + if (!login) continue; + + const previous = latest.get(login); + if (!previous) { + latest.set(login, { index, review }); + continue; + } + + const submittedAt = Date.parse(review.submitted_at || ""); + const previousSubmittedAt = Date.parse(previous.review.submitted_at || ""); + const useSubmittedAt = + !Number.isNaN(submittedAt) && + !Number.isNaN(previousSubmittedAt) && + submittedAt !== previousSubmittedAt; + + if ( + (useSubmittedAt && submittedAt > previousSubmittedAt) || + (!useSubmittedAt && index > previous.index) + ) { + latest.set(login, { index, review }); + } + } + + return Array.from(latest.values()) + .sort((a, b) => a.index - b.index) + .map(({ review }) => review); +} + /** * Per-path approval check. Each ownership group needs at least one * approval from its owners. Files matching only "*" require a maintainer. @@ -465,9 +503,10 @@ module.exports = async ({ github, context, core }) => { repo: context.repo.repo, pull_number: context.issue.number, }); + const latestReviews = latestReviewsByUser(reviews); // Maintainer approval -> success with simple comment - const maintainerApproval = reviews.find( + const maintainerApproval = latestReviews.find( ({ state, user }) => state === "APPROVED" && user && maintainers.includes(user.login) ); @@ -486,7 +525,7 @@ module.exports = async ({ github, context, core }) => { // Maintainer-authored PR with any approval -> success if (authorLogin && maintainers.includes(authorLogin)) { - const hasAnyApproval = reviews.some( + const hasAnyApproval = latestReviews.some( ({ state, user }) => state === "APPROVED" && user && user.login !== authorLogin ); @@ -503,8 +542,8 @@ module.exports = async ({ github, context, core }) => { } } - // Gather approved logins (excluding the PR author). - const approverLogins = reviews + // Gather currently approved logins (excluding the PR author). + const approverLogins = latestReviews .filter( ({ state, user }) => state === "APPROVED" && user && user.login !== authorLogin diff --git a/.github/workflows/maintainer-approval.test.js b/.github/workflows/maintainer-approval.test.js index 2866dc9d3d7..3c18e7e9618 100644 --- a/.github/workflows/maintainer-approval.test.js +++ b/.github/workflows/maintainer-approval.test.js @@ -334,6 +334,155 @@ describe("maintainer-approval", () => { assert.equal(github._checkRuns.length, 0); }); + it("keeps maintainer approval after the same maintainer leaves a comment", async () => { + const github = makeGithub({ + reviews: [ + { + state: "APPROVED", + submitted_at: "2026-01-01T00:00:00Z", + user: { login: "maintainer1" }, + }, + { + state: "COMMENTED", + submitted_at: "2026-01-02T00:00:00Z", + 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, 1); + assert.equal(github._checkRuns[0].conclusion, "success"); + assert.ok(github._checkRuns[0].output.summary.includes("maintainer1")); + }); + + it("keeps approval on a maintainer-authored PR after the same reviewer leaves a comment", async () => { + const github = makeGithub({ + reviews: [ + { + state: "APPROVED", + submitted_at: "2026-01-01T00:00:00Z", + user: { login: "randomreviewer" }, + }, + { + state: "COMMENTED", + submitted_at: "2026-01-02T00:00:00Z", + user: { login: "randomreviewer" }, + }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core = makeCore(); + const context = makeContext({ author: "maintainer1" }); + + await runModule({ github, context, core }); + + assert.equal(github._checkRuns.length, 1); + assert.equal(github._checkRuns[0].conclusion, "success"); + assert.ok(github._checkRuns[0].output.summary.includes("maintainer-authored")); + }); + + it("keeps path-owner approval after the same owner leaves a comment", async () => { + const github = makeGithub({ + reviews: [ + { + state: "APPROVED", + submitted_at: "2026-01-01T00:00:00Z", + user: { login: "jefferycheng1" }, + }, + { + state: "COMMENTED", + submitted_at: "2026-01-02T00:00:00Z", + 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("ignores an older maintainer approval after the same maintainer requests changes", async () => { + const github = makeGithub({ + reviews: [ + { + state: "APPROVED", + submitted_at: "2026-01-01T00:00:00Z", + user: { login: "maintainer1" }, + }, + { + state: "CHANGES_REQUESTED", + submitted_at: "2026-01-02T00:00:00Z", + 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("ignores an older approval on a maintainer-authored PR after the same reviewer requests changes", async () => { + const github = makeGithub({ + reviews: [ + { + state: "APPROVED", + submitted_at: "2026-01-01T00:00:00Z", + user: { login: "randomreviewer" }, + }, + { + state: "CHANGES_REQUESTED", + submitted_at: "2026-01-02T00:00:00Z", + user: { login: "randomreviewer" }, + }, + ], + files: [{ filename: "cmd/pipelines/foo.go" }], + }); + const core = makeCore(); + const context = makeContext({ author: "maintainer1" }); + + await runModule({ github, context, core }); + + assert.equal(github._checkRuns.length, 0); + }); + + it("ignores an older path-owner approval after the same owner requests changes", async () => { + const github = makeGithub({ + reviews: [ + { + state: "APPROVED", + submitted_at: "2026-01-01T00:00:00Z", + user: { login: "jefferycheng1" }, + }, + { + state: "CHANGES_REQUESTED", + submitted_at: "2026-01-02T00:00:00Z", + 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: [