diff --git a/org-tools/governance/scripts/pr_validator.py b/org-tools/governance/scripts/pr_validator.py index 84d7498..620adf9 100755 --- a/org-tools/governance/scripts/pr_validator.py +++ b/org-tools/governance/scripts/pr_validator.py @@ -174,12 +174,12 @@ def validate(self, pr: PullRequest) -> ValidationResult: # 7. Approvals & Assignments Evaluation (Global) requirement_statuses = self._evaluate_requirements( - merged_requirements, approver_usernames, assigned_usernames + merged_requirements, approver_usernames, assigned_usernames, author=pr.author ) # 8. File-by-File Evaluation file_statuses = self._evaluate_file_statuses( - requirements_by_file, approver_usernames, assigned_usernames + requirements_by_file, approver_usernames, assigned_usernames, author=pr.author ) # 9. Changes Requested Check @@ -223,6 +223,7 @@ def _evaluate_requirement( req: RuleRequirement, approver_usernames: set[str], requested_users_set: set[str], + author: str | None = None, ) -> RequirementStatus: """Calculate and return status for a requirement under the Venn Diagram model.""" approver_users = [User.create(u, self.memberships) for u in approver_usernames] @@ -231,11 +232,44 @@ def _evaluate_requirement( approvers = [u.username for u in approver_users if req.is_satisfied_by(u)] assigned_count = sum(1 for u in assigned_users if req.is_satisfied_by(u)) approved_count = len(approvers) + + # Dynamic Governance Council requirement evaluation based on PR author + is_gc_req = (req.team and req.team.name == "governance-council") or ( + req.min_team and req.min_team.name == "governance-council" + ) + + effective_req = req + required_approver = None + + if is_gc_req and author: + author_user = User.create(author, self.memberships) + is_author_gc = "governance-council" in author_user.teams + + if is_author_gc: + if author == "amithanda": + min_approvals = 1 + else: + min_approvals = 2 + required_approver = "amithanda" + else: + min_approvals = 2 + + if min_approvals != req.min_approvals: + effective_req = RuleRequirement( + min_approvals=min_approvals, + team=req.team, + min_team=req.min_team, + ) + + is_satisfied = approved_count >= effective_req.min_approvals + if required_approver and required_approver not in approvers: + is_satisfied = False + return RequirementStatus( - requirement=req, + requirement=effective_req, approved_count=approved_count, assigned_count=assigned_count, - is_satisfied=approved_count >= req.min_approvals, + is_satisfied=is_satisfied, approvers=sorted(approvers), ) @@ -244,12 +278,13 @@ def _evaluate_requirements( requirements: list[RuleRequirement], approver_usernames: set[str], assigned_usernames: set[str], + author: str | None = None, ) -> list[RequirementStatus]: """Evaluate each requirement's approvals and assignments count under the Venn Diagram model.""" requirement_statuses = [] for req in requirements: status = self._evaluate_requirement( - req, approver_usernames, assigned_usernames + req, approver_usernames, assigned_usernames, author=author ) requirement_statuses.append(status) @@ -291,7 +326,13 @@ def _get_all_approvers_and_assigned_usernames( else: assigned.add(user) + # Count reviewers who have submitted reviews (including comments) as actively reviewing + for review in pr.reviews: + if review.user != pr.author: + assigned.add(review.user.lower()) + assigned.difference_update(approvers) + assigned.discard(pr.author) return approvers, assigned def _evaluate_file_statuses( @@ -299,12 +340,13 @@ def _evaluate_file_statuses( requirements_by_file: dict[str, list[RuleRequirement]], approver_usernames: set[str], assigned_usernames: set[str], + author: str | None = None, ) -> list[FileValidationStatus]: """Evaluates and generates validation statuses for each changed file in the PR.""" file_statuses = [] for file, file_requirements in requirements_by_file.items(): file_req_statuses = self._evaluate_requirements( - file_requirements, approver_usernames, assigned_usernames + file_requirements, approver_usernames, assigned_usernames, author=author ) file_satisfied = all(status.is_satisfied for status in file_req_statuses) diff --git a/org-tools/governance/tests/test_pr_validator.py b/org-tools/governance/tests/test_pr_validator.py index cd5ef1e..d4fd561 100644 --- a/org-tools/governance/tests/test_pr_validator.py +++ b/org-tools/governance/tests/test_pr_validator.py @@ -120,6 +120,8 @@ def setUp(self): "governance-council": { "gov-member1", "gov-member2", + "gov-member3", + "amithanda", "proxy1", }, }, @@ -259,7 +261,7 @@ def test_specific_team_requirement(self): self.assertFalse(res.is_mergeable) self.assertEqual(res.error, ValidationErrorReason.INSUFFICIENT_APPROVALS) - # gov-member1 is in governance-council, should pass + # gov-member1 and gov-member2 (2 GC members) for non-GC author1 should pass pr_ok = PullRequest( number=1, author="author1", @@ -267,11 +269,95 @@ def test_specific_team_requirement(self): changed_files=["LICENSE"], reviews=[ Review(user="gov-member1", state=ReviewState.APPROVED), + Review(user="gov-member2", state=ReviewState.APPROVED), ], ) res_ok = self.validator.validate(pr_ok) self.assertTrue(res_ok.is_mergeable) + def test_gc_author_non_gc_author_requires_two_approvals(self): + """Non-GC author requires 2 GC approvers.""" + pr_1_app = PullRequest( + number=1, + author="author1", + is_draft=False, + changed_files=["LICENSE"], + reviews=[ + Review(user="gov-member1", state=ReviewState.APPROVED), + ], + ) + res_1 = self.validator.validate(pr_1_app) + self.assertFalse(res_1.is_mergeable) + self.assertEqual(res_1.error, ValidationErrorReason.INSUFFICIENT_APPROVALS) + + pr_2_app = PullRequest( + number=1, + author="author1", + is_draft=False, + changed_files=["LICENSE"], + reviews=[ + Review(user="gov-member1", state=ReviewState.APPROVED), + Review(user="gov-member2", state=ReviewState.APPROVED), + ], + ) + res_2 = self.validator.validate(pr_2_app) + self.assertTrue(res_2.is_mergeable) + + def test_gc_author_amithanda_requires_one_approval(self): + """GC author amithanda requires 1 GC approver.""" + pr_0_app = PullRequest( + number=1, + author="amithanda", + is_draft=False, + changed_files=["LICENSE"], + reviews=[], + ) + res_0 = self.validator.validate(pr_0_app) + self.assertFalse(res_0.is_mergeable) + + pr_1_app = PullRequest( + number=1, + author="amithanda", + is_draft=False, + changed_files=["LICENSE"], + reviews=[ + Review(user="gov-member1", state=ReviewState.APPROVED), + ], + ) + res_1 = self.validator.validate(pr_1_app) + self.assertTrue(res_1.is_mergeable) + + def test_gc_author_other_gc_member_requires_two_approvals_including_amit(self): + """GC author who is not amithanda requires 2 GC approvers, one of which must be amithanda.""" + # 2 GC approvals without amithanda (non-proxy reviewers) -> fails + pr_no_amit = PullRequest( + number=1, + author="gov-member1", + is_draft=False, + changed_files=["LICENSE"], + reviews=[ + Review(user="gov-member2", state=ReviewState.APPROVED), + Review(user="gov-member3", state=ReviewState.APPROVED), + ], + ) + res_no_amit = self.validator.validate(pr_no_amit) + self.assertFalse(res_no_amit.is_mergeable) + self.assertEqual(res_no_amit.error, ValidationErrorReason.INSUFFICIENT_APPROVALS) + + # 2 GC approvals with amithanda -> passes + pr_with_amit = PullRequest( + number=1, + author="gov-member1", + is_draft=False, + changed_files=["LICENSE"], + reviews=[ + Review(user="amithanda", state=ReviewState.APPROVED), + Review(user="gov-member2", state=ReviewState.APPROVED), + ], + ) + res_with_amit = self.validator.validate(pr_with_amit) + self.assertTrue(res_with_amit.is_mergeable) + def test_changes_requested_blocks(self): """Test that changes requested block validation.""" pr = PullRequest( @@ -712,6 +798,107 @@ def test_one_approved_one_changes_requested_assigned_count(self): self.assertFalse(status.is_satisfied) self.assertEqual(status.approvers, ["maint1"]) + def test_commented_reviewer_counts_towards_assigned_count(self): + """Test that a reviewer who added comments is counted in assigned_count.""" + # Setup hierarchy and config requiring 2 approvals from maintainers + hierarchy = { + "maintainers": Team(name="maintainers", level=2), + } + rules = [ + GovernanceRule( + name="Test Rule", + patterns=["*"], + requires_all=[ + RuleRequirement(min_approvals=2, min_team=hierarchy["maintainers"]) + ], + ) + ] + config = GovernanceConfig( + teams=hierarchy, + rules=rules, + fallback=[], + proxy_reviewers=set(), + ) + memberships = TeamMemberships.create( + members_by_team={ + "maintainers": {"maint1", "maint2", "maint3"}, + }, + teams=hierarchy, + ) + validator = PullRequestValidator(config, memberships) + + # maint1 approved, maint2 commented (no longer in review_requests on GitHub) + pr = PullRequest( + number=1, + author="author1", + is_draft=False, + changed_files=["file.txt"], + reviews=[ + Review(user="maint1", state=ReviewState.APPROVED), + Review(user="maint2", state=ReviewState.COMMENTED), + ], + assigned_user_names=[], + ) + + res = validator.validate(pr) + self.assertFalse(res.is_mergeable) + self.assertEqual(res.error, ValidationErrorReason.INSUFFICIENT_APPROVALS) + + self.assertEqual(len(res.requirement_statuses), 1) + status = res.requirement_statuses[0] + self.assertEqual(status.approved_count, 1) + self.assertEqual(status.assigned_count, 1) # maint2 who commented is counted + self.assertFalse(status.is_satisfied) + self.assertEqual(status.approvers, ["maint1"]) + + def test_author_comments_not_counted_in_assigned(self): + """Test that PR author comments do not count towards assigned_count.""" + hierarchy = { + "maintainers": Team(name="maintainers", level=2), + } + rules = [ + GovernanceRule( + name="Test Rule", + patterns=["*"], + requires_all=[ + RuleRequirement(min_approvals=2, min_team=hierarchy["maintainers"]) + ], + ) + ] + config = GovernanceConfig( + teams=hierarchy, + rules=rules, + fallback=[], + proxy_reviewers=set(), + ) + # Author is also a maintainer + memberships = TeamMemberships.create( + members_by_team={ + "maintainers": {"author1", "maint1", "maint2"}, + }, + teams=hierarchy, + ) + validator = PullRequestValidator(config, memberships) + + pr = PullRequest( + number=1, + author="author1", + is_draft=False, + changed_files=["file.txt"], + reviews=[ + Review(user="author1", state=ReviewState.COMMENTED), + Review(user="maint1", state=ReviewState.APPROVED), + ], + assigned_user_names=[], + ) + + res = validator.validate(pr) + self.assertFalse(res.is_mergeable) + status = res.requirement_statuses[0] + self.assertEqual(status.approved_count, 1) + self.assertEqual(status.assigned_count, 0) # Author comments do not count + self.assertEqual(status.approvers, ["maint1"]) + class TestFetchTeamMemberships(unittest.TestCase): """Tests for GitHubClient.fetch_team_memberships method."""