Skip to content

Use latest review state in maintainer-approval #5323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fallintoplace wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Use latest review state in maintainer-approval #5323

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ff58a7
Use latest review state in maintainer approval
fallintoplace May 24, 2026
f9a7550
Ignore commented reviews for approval state
fallintoplace Jun 9, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 43 additions & 4 deletions .github/workflows/maintainer-approval.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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)
);
Expand All @@ -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
);
Expand All @@ -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
Expand Down
149 changes: 149 additions & 0 deletions .github/workflows/maintainer-approval.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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: [
Expand Down