Skip to content

feat(governance): enforce author-dependent Governance Council approval rules #59

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

Merged
damaz91 merged 7 commits into from
Aug 14, 2026
Merged

feat(governance): enforce author-dependent Governance Council approval rules #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
554ec5f
feat(governance): enforce author-dependent Governance Council approva…
carolinerg1 Aug 10, 2026
bee4374
feat(governance): simplify GC dynamic approval rules to 1 for GC, 2 f…
damaz91 Aug 12, 2026
08157f1
feat(governance): only override GC approvals for GC authors
damaz91 Aug 12, 2026
315d1a5
test(governance): clean up redundant GC author approval tests
damaz91 Aug 12, 2026
f783e45
test(governance): remove amithanda from test GC team
damaz91 Aug 12, 2026
17fea2c
feat(governance): require 2 total approvals when GC rule min_approval…
damaz91 Aug 12, 2026
0f76fbd
style(logger): update pending message phrasing to any member review
damaz91 Aug 12, 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
51 changes: 45 additions & 6 deletions org-tools/governance/scripts/pr_validator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,12 +174,18 @@ 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
Expand Down Expand Up @@ -223,6 +229,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]
Expand All @@ -231,11 +238,41 @@ 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
require_total_approvals = 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:
min_approvals = 1
if min_approvals != req.min_approvals:
effective_req = RuleRequirement(
min_approvals=min_approvals,
team=req.team,
min_team=req.min_team,
)
require_total_approvals = req.min_approvals

is_satisfied = approved_count >= effective_req.min_approvals
if (
require_total_approvals is not None
and len(approver_usernames) < require_total_approvals

@amithanda amithanda Aug 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

len(approver_usernames) < require_total_approvals

Can you please double check and confirm the logic here?
I haven't looked too closely but I think there may be an edge-case bug here: approver_usernames can contain non-approving reviewers (e.g., users who only commented or requested changes), causing len(approver_usernames) to pass even when there isn't a second actual approval.

To fix this, we should count distinct valid approvals (excluding the PR author):

total_valid_approvals = len([u for u in approver_usernames if u != author])
if total_valid_approvals < require_total_approvals:
    is_satisfied = False

@damaz91 damaz91 Aug 13, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for double-checking @amithanda - that’s actually already handled upstream in _get_all_approvers_and_assigned_usernames():

  1. approver_usernames is only populated from reviews where state == ReviewState.APPROVED (commenters and change requestors are placed into assigned_usernames instead).
  2. The author is already filtered out there (if user != pr.author), so approver_usernames is guaranteed to contain only distinct, non-author approving reviewers.

Because of that, len(approver_usernames) accurately reflects the total number of distinct valid approvals on the PR.

):
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),
)

Expand All @@ -244,12 +281,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)

Expand Down Expand Up @@ -305,12 +343,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)

Expand Down
24 changes: 15 additions & 9 deletions org-tools/governance/scripts/validation_logger.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,15 +147,21 @@ def _format_requirement_status(
return f"{req_line}\n{met_line}"

missing = req.min_approvals - status.approved_count
if status.assigned_count >= missing:
hint = f"Waiting for approval from {team_desc}."
if missing <= 0:
pending_line = (
f"{indent} * **Pending:** Governance Council requirement met ({status.approved_count}/{req.min_approvals}), "
f"waiting for additional review from any member to satisfy total required approvals."
)
else:
needed = missing - status.assigned_count
hint = f"Waiting for {needed} more reviewer(s) to be assigned from {team_desc}."
pending_line = (
f"{indent} * **Pending:** Needs {missing} approval{'s' if missing > 1 else ''} "
f"from {team_desc} ({status.assigned_count} eligible "
f"reviewer{'s' if status.assigned_count != 1 else ''} assigned). {hint}"
)
if status.assigned_count >= missing:
hint = f"Waiting for approval from {team_desc}."
else:
needed = missing - status.assigned_count
hint = f"Waiting for {needed} more reviewer(s) to be assigned from {team_desc}."
pending_line = (
f"{indent} * **Pending:** Needs {missing} approval{'s' if missing > 1 else ''} "
f"from {team_desc} ({status.assigned_count} eligible "
f"reviewer{'s' if status.assigned_count != 1 else ''} assigned). {hint}"
)

return f"{req_line}\n{met_line}\n{pending_line}"
125 changes: 123 additions & 2 deletions org-tools/governance/tests/test_pr_validator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def setUp(self):
patterns=["LICENSE", ".github/CODEOWNERS"],
requires_all=[
RuleRequirement(
min_approvals=1, team=self.hierarchy["governance-council"]
min_approvals=2, team=self.hierarchy["governance-council"]
)
],
),
Expand Down Expand Up @@ -120,6 +120,7 @@ def setUp(self):
"governance-council": {
"gov-member1",
"gov-member2",
"gov-member3",
"proxy1",
},
},
Expand Down Expand Up @@ -259,19 +260,139 @@ 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",
is_draft=False,
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_requires_one_approval(self):
"""GC author requires 1 GC approver and 2 total approvers when rule requires 2."""
# gov-member1 (GC) author, 0 approvals -> fails
pr_0_app = PullRequest(
number=1,
author="gov-member1",
is_draft=False,
changed_files=["LICENSE"],
reviews=[],
)
res_0 = self.validator.validate(pr_0_app)
self.assertFalse(res_0.is_mergeable)
self.assertEqual(res_0.error, ValidationErrorReason.INSUFFICIENT_APPROVALS)

# gov-member1 (GC) author, 1 GC approval alone -> fails (needs 2 total approvals)
pr_1_app = PullRequest(
number=1,
author="gov-member1",
is_draft=False,
changed_files=["LICENSE"],
reviews=[
Review(user="gov-member2", 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)

# gov-member1 (GC) author, 1 GC approval + 1 peer/maintainer approval -> passes
pr_1_gc_1_peer = PullRequest(
number=1,
author="gov-member1",
is_draft=False,
changed_files=["LICENSE"],
reviews=[
Review(user="gov-member2", state=ReviewState.APPROVED),
Review(user="maint1", state=ReviewState.APPROVED),
],
)
res_gc_peer = self.validator.validate(pr_1_gc_1_peer)
self.assertTrue(res_gc_peer.is_mergeable)
self.assertEqual(res_gc_peer.mergeable_reason, MergeableReason.RULES_SATISFIED)

# gov-member1 (GC) author, 2 GC approvals -> passes
pr_2_gc = 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_2_gc = self.validator.validate(pr_2_gc)
self.assertTrue(res_2_gc.is_mergeable)
self.assertEqual(res_2_gc.mergeable_reason, MergeableReason.RULES_SATISFIED)

def test_gc_requirement_non_gc_author_respects_yaml_1(self):
"""Test that if YAML requires 1 approval, non-GC author only needs 1 (no override to 2)."""
rules = [
GovernanceRule(
name="Gov 1",
patterns=["LICENSE"],
requires_all=[
RuleRequirement(
min_approvals=1, team=self.hierarchy["governance-council"]
)
],
)
]
config = GovernanceConfig(
teams=self.hierarchy,
rules=rules,
fallback=self.fallback,
proxy_reviewers=self.proxy_reviewers,
)
validator = PullRequestValidator(config, self.memberships)

pr = PullRequest(
number=1,
author="author1", # Not GC
is_draft=False,
changed_files=["LICENSE"],
reviews=[
Review(user="gov-member1", state=ReviewState.APPROVED),
],
)
res = validator.validate(pr)
self.assertTrue(res.is_mergeable)

def test_changes_requested_blocks(self):
"""Test that changes requested block validation."""
pr = PullRequest(
Expand Down
Loading