From a335d04031b4281b35c7214012fb0cca611a4b4462455ab55480c551ce53eab0 Mon Sep 17 00:00:00 2001 From: Andrii Nikitin Date: Tue, 3 Mar 2026 15:51:14 +0100 Subject: [PATCH] pr: enforce all maintainer approvals when ReviewRequired is true - Update MaintainershipMap.IsApproved to require approvals from all listed maintainers (excluding the submitter) when ReviewRequired is enabled. - Split ReviewInterface logic to differentiate between approvals and rejections by adding IsRejectedBy and restricting IsReviewedBy to the APPROVED state. - Fix premature removal of maintainer review requests in FindMissingAndExtraReviewers; ensure requests remain active if ReviewRequired is true and approvals are still missing. - Ensure any maintainer rejection correctly triggers the cleanup of other redundant review requests. Analysis Summary The failures in test_006 and test_007 were caused by two primary logic bugs: 1. Conflated Review States: IsReviewedBy treated both "Approved" and "Request Changes" as the same state. This meant a rejection didn't properly trigger the "cleanup" logic, and an approval was indistinguishable from a rejection in several checks. 2. Greedy Reviewer Removal: The bot assumed that any maintainer approval meant all other maintainer requests could be removed. This contradicted the ReviewRequired flag, which (as seen in test_007) is intended to mandate multiple/all maintainer approvals before the PR is considered ready. --- common/maintainership.go | 30 ++++++++++++++--- common/pr.go | 6 ++-- common/reviews.go | 35 +++++++++++++++++++- integration/tests/workflow_pr_review_test.py | 2 -- 4 files changed, 62 insertions(+), 11 deletions(-) diff --git a/common/maintainership.go b/common/maintainership.go index 1165f31..038f28f 100644 --- a/common/maintainership.go +++ b/common/maintainership.go @@ -168,12 +168,32 @@ func (data *MaintainershipMap) IsApproved(pkg string, reviews []*models.PullRevi return true } - for _, review := range reviews { - if !review.Stale && review.State == ReviewStateApproved && slices.Contains(reviewers, review.User.UserName) { - LogDebug("Reviewed by", review.User.UserName) - return true + if !data.Config.ReviewRequired { + for _, review := range reviews { + if !review.Stale && review.State == ReviewStateApproved && slices.Contains(reviewers, review.User.UserName) { + LogDebug("Reviewed by", review.User.UserName) + return true + } } - + } else { + // all maintainers except submitter must approve + for _, m := range reviewers { + if m == submitter { + continue + } + approved := false + for _, review := range reviews { + if !review.Stale && review.State == ReviewStateApproved && review.User.UserName == m { + approved = true + break + } + } + if !approved { + LogDebug("Missing approval from maintainer:", m) + return false + } + } + return true } return false } diff --git a/common/pr.go b/common/pr.go index 9ccf31c..c805cf3 100644 --- a/common/pr.go +++ b/common/pr.go @@ -328,7 +328,7 @@ func (rs *PRSet) FindMissingAndExtraReviewers(maintainers MaintainershipData, id if len(rs.PRs) > 1 { noReviewPRCreators = append(noReviewPRCreators, rs.BotUser) } - if slices.Contains(noReviewPRCreators, pr.PR.User.UserName) || pr.Reviews.IsReviewedByOneOf(prjMaintainers...) { + if slices.Contains(noReviewPRCreators, pr.PR.User.UserName) || pr.Reviews.IsRejectedByOneOf(prjMaintainers...) || (pr.Reviews.IsReviewedByOneOf(prjMaintainers...) && !rs.Config.ReviewRequired) { LogDebug("Project already reviewed by a project maintainer, remove rest") // do not remove reviewers if they are also maintainers prjMaintainers = slices.DeleteFunc(prjMaintainers, func(m string) bool { return slices.Contains(missing, m) }) @@ -355,8 +355,8 @@ func (rs *PRSet) FindMissingAndExtraReviewers(maintainers MaintainershipData, id LogDebug("packakge maintainers:", Maintainers) missing = slices.Concat(configReviewers.Pkg, configReviewers.PkgOptional) - if slices.Contains(noReviewPkgPRCreators, pr.PR.User.UserName) || pr.Reviews.IsReviewedByOneOf(Maintainers...) { - // submitter is maintainer or already reviewed + if slices.Contains(noReviewPkgPRCreators, pr.PR.User.UserName) || pr.Reviews.IsRejectedByOneOf(Maintainers...) || (pr.Reviews.IsReviewedByOneOf(Maintainers...) && !rs.Config.ReviewRequired) { + // submitter is maintainer or already reviewed/rejected LogDebug("Package reviewed by maintainer (or subitter is maintainer), remove the rest of them") // do not remove reviewers if they are also maintainers Maintainers = slices.DeleteFunc(Maintainers, func(m string) bool { return slices.Contains(missing, m) }) diff --git a/common/reviews.go b/common/reviews.go index da03839..315773b 100644 --- a/common/reviews.go +++ b/common/reviews.go @@ -16,6 +16,8 @@ type ReviewInterface interface { HasPendingReviewBy(reviewer string) bool IsReviewedBy(reviewer string) bool IsReviewedByOneOf(reviewers ...string) bool + IsRejectedBy(reviewer string) bool + IsRejectedByOneOf(reviewers ...string) bool SetRequiredReviewers(reviewers []string) } @@ -230,7 +232,7 @@ func (r *PRReviews) IsReviewedBy(reviewer string) bool { for _, r := range r.Reviews { if r.User.UserName == reviewer && !r.Stale { switch r.State { - case ReviewStateApproved, ReviewStateRequestChanges: + case ReviewStateApproved: return true default: return false @@ -244,6 +246,37 @@ func (r *PRReviews) IsReviewedBy(reviewer string) bool { func (r *PRReviews) IsReviewedByOneOf(reviewers ...string) bool { for _, reviewer := range reviewers { if r.IsReviewedBy(reviewer) { + LogDebug(" -- IsReviewedByOneOf: matched ", reviewer) + return true + } + } + + return false +} + +func (r *PRReviews) IsRejectedBy(reviewer string) bool { + if r == nil { + return false + } + + for _, r := range r.Reviews { + if r.User.UserName == reviewer && !r.Stale { + switch r.State { + case ReviewStateRequestChanges: + return true + default: + return false + } + } + } + + return false +} + +func (r *PRReviews) IsRejectedByOneOf(reviewers ...string) bool { + for _, reviewer := range reviewers { + if r.IsRejectedBy(reviewer) { + LogDebug(" -- IsRejectedByOneOf: matched ", reviewer) return true } } diff --git a/integration/tests/workflow_pr_review_test.py b/integration/tests/workflow_pr_review_test.py index f3985c8..0c0587d 100644 --- a/integration/tests/workflow_pr_review_test.py +++ b/integration/tests/workflow_pr_review_test.py @@ -233,7 +233,6 @@ index 0000000..e69de29 @pytest.mark.t006 -@pytest.mark.xfail(reason="tbd flacky in ci") def test_006_maintainer_rejection_removes_other_requests(maintainer_env, ownerA_client, ownerBB_client): """ Test scenario: @@ -287,7 +286,6 @@ index 0000000..e69de29 @pytest.mark.t007 -@pytest.mark.xfail(reason="TBD troubleshoot") def test_007_review_required_needs_all_approvals(review_required_env, ownerA_client, ownerBB_client): """ Test scenario: -- 2.51.1