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

@@ -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
 }

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) })

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
 		}
 	}

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: