diff --git a/.github/reapproval-gate.md b/.github/reapproval-gate.md new file mode 100644 index 0000000000..fb1b8a8686 --- /dev/null +++ b/.github/reapproval-gate.md @@ -0,0 +1,130 @@ +# Reapproval Gate + +The reapproval gate is an automated GitHub Actions workflow that prevents +approved pull requests from being silently merged after significant new code is +introduced without a fresh review. + +## How it works + +The gate runs whenever **new commits are pushed to an open PR** (`synchronize` +event), or whenever a **review is submitted** (so the gate re-evaluates after a +fresh approval). + +Each reviewer's approval is evaluated **independently**: + +``` + For each reviewer who + has approved the PR: + │ + ───────────────┼────────────────── + ▼ ▼ + Approval is the Commits exist after + most recent commit? reviewer's approval? + │ │ + ▼ No │ Yes + VALID ─────────────┼────────────── + ▼ ▼ + VALID Calculate changes* + │ + changes > 20 LOC? + │ + No │ Yes + ─────────────┼────────── + ▼ ▼ + VALID STALE + (re-request that + reviewer only) +``` + +Gate result: +- **PASS** — every approver's approval is VALID. +- **FAIL** — at least one approver is STALE (re-review is re-requested only from + stale approvers; reviewers with a current approval are not bothered). + +\* "Changes" counts **additions + deletions** across commits pushed after the +reviewer's most recent approval, **excluding** merge-from-main commits (e.g. +the commits created by GitHub's "Update branch" button or `git merge main`). +Merge-from-main commits are skipped because they only bring in code that was +already reviewed and merged into the main branch — they represent no new work +by the PR author. + +## Required check + +The job is named **`reapproval-gate`** (the full GitHub check name visible in +branch protection settings is `Reapproval Gate / reapproval-gate`). + +To enforce the gate, add it as a **required status check** in your branch +protection rule: + +1. Go to **Settings → Branches → Branch protection rules** for the `main` + branch. +2. Enable **"Require status checks to pass before merging"**. +3. Search for and add **`Reapproval Gate / reapproval-gate`** (or just + `reapproval-gate` depending on how GitHub resolves the name in your + settings). +4. Optionally enable **"Require branches to be up to date before merging"** + for additional safety. + +## Configuration + +All configuration lives in the workflow file +`.github/workflows/reapproval-gate.yml`. + +| Variable | Default | Description | +|---|---|---| +| `MAIN_BRANCH` | `main` | Name of the default branch. Merge-from-`MAIN_BRANCH` commits are excluded from the changes calculation. | +| `LOC_THRESHOLD` | `20` | Maximum allowed additions + deletions after a reviewer's most recent approval before re-approval is required from that reviewer. Change this constant in `.github/scripts/reapproval_gate.py`. | + +## Merge-from-main detection + +A commit is treated as a "merge from main" (and skipped in the LOC count) when +**both** of the following are true: + +1. The commit has **two or more parents** (it is a merge commit). +2. The commit message matches one of these patterns (case-insensitive): + - `Merge branch 'main'` + - `Merge remote-tracking branch 'origin/main'` + - `Merge remote-tracking branch 'upstream/main'` + - `Merge refs/heads/main` + +These patterns match the messages produced by `git merge main` and by +GitHub's **"Update branch"** button. Custom merge messages that don't follow +these patterns will be treated as regular commits and counted toward the LOC +threshold. + +## What happens when the gate fails + +1. The `reapproval-gate` check turns red and blocks merge (when required). +2. The workflow **re-requests review only from stale approvers** — reviewers + whose approval post-dates all significant commits are not bothered. +3. A detailed log is available in the **Actions** tab of the PR. + +The gate resets automatically: once a reviewer approves the PR again +(triggering the `pull_request_review` event), the workflow re-evaluates and +the check turns green if no further changes are found. + +## Force-pushes + +If the branch is force-pushed, the commit SHA recorded in the approval may no +longer exist in the PR history. In that case the gate **fails conservatively** +and requires re-approval, because it cannot determine the size of the change. + +## Running the script locally + +```bash +export GITHUB_TOKEN=ghp_... +export GITHUB_REPOSITORY=openvinotoolkit/model_server +export PR_NUMBER=1234 +export MAIN_BRANCH=main +python .github/scripts/reapproval_gate.py +``` + +## Running the unit tests + +```bash +python .github/scripts/test_reapproval_gate.py +``` + +The unit tests cover `is_merge_from_main()` and `find_latest_approval_per_user()` — the +two pure functions that contain the core decision logic — without requiring any +GitHub credentials. diff --git a/.github/scripts/reapproval_gate.py b/.github/scripts/reapproval_gate.py new file mode 100644 index 0000000000..50b81191c0 --- /dev/null +++ b/.github/scripts/reapproval_gate.py @@ -0,0 +1,384 @@ +#!/usr/bin/env python3 +# Copyright 2026 Intel Corporation +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +""" +Reapproval Gate — checks, per reviewer, whether significant code changes were +pushed after each reviewer's most recent approval. + +Rules: + - If the PR has no approvals, the gate passes (nothing to protect). + - For each reviewer who has approved, compute the number of lines changed + (additions + deletions) for commits pushed after *their* most recent + approval, skipping any merge-from-main commits. + - If a reviewer's changes > LOC_THRESHOLD (default 20), that reviewer is + marked stale and re-review is re-requested from them specifically. + - The gate PASSES only when every approver's changes are within the threshold. + +This means reviewers whose approval post-dates the latest significant commit +are NOT bothered — only reviewers whose approval is genuinely stale are asked +to re-approve. + +Environment variables consumed: + GITHUB_TOKEN — GitHub token with pull-requests:write permission + GITHUB_REPOSITORY — owner/repo (set automatically by GitHub Actions) + PR_NUMBER — pull request number + MAIN_BRANCH — name of the default/main branch (default: "main") +""" + +import json +import os +import re +import sys +import urllib.error +import urllib.request +from typing import Any, Dict, List, Optional, Set, Tuple + +LOC_THRESHOLD: int = 20 + + +# --------------------------------------------------------------------------- +# GitHub API helpers +# --------------------------------------------------------------------------- + +def make_github_request( + token: str, + method: str, + url: str, + body: Optional[Dict] = None, +) -> Any: + """Make a single GitHub API request and return the parsed JSON response.""" + req = urllib.request.Request(url, method=method) + req.add_header("Authorization", f"token {token}") + req.add_header("Accept", "application/vnd.github.v3+json") + req.add_header("User-Agent", "reapproval-gate/1.0") + if body is not None: + req.add_header("Content-Type", "application/json") + req.data = json.dumps(body).encode("utf-8") + try: + with urllib.request.urlopen(req, timeout=30) as response: + return json.loads(response.read()) + except urllib.error.HTTPError as exc: + print(f"HTTP {exc.code} for {method} {url}: {exc.read().decode()}", flush=True) + raise + + +def github_get_paginated(token: str, base_url: str) -> List[Dict]: + """Fetch all pages from a paginated GitHub API endpoint.""" + results: List[Dict] = [] + page = 1 + while True: + sep = "&" if "?" in base_url else "?" + url = f"{base_url}{sep}per_page=100&page={page}" + data = make_github_request(token, "GET", url) + if not data: + break + results.extend(data) + if len(data) < 100: + break + page += 1 + return results + + +def get_reviews(token: str, repo: str, pr_number: int) -> List[Dict]: + """Return all reviews for a pull request in chronological order.""" + url = f"https://api.github.com/repos/{repo}/pulls/{pr_number}/reviews" + return github_get_paginated(token, url) + + +def get_pr_commits(token: str, repo: str, pr_number: int) -> List[Dict]: + """Return all commits for a pull request in chronological order.""" + url = f"https://api.github.com/repos/{repo}/pulls/{pr_number}/commits" + return github_get_paginated(token, url) + + +def get_commit_details(token: str, repo: str, sha: str) -> Dict: + """Return full commit details including file-level stats and parents.""" + url = f"https://api.github.com/repos/{repo}/commits/{sha}" + return make_github_request(token, "GET", url) + + +def request_re_review( + token: str, + repo: str, + pr_number: int, + reviewers: List[str], +) -> None: + """Re-request reviews from the supplied list of GitHub logins (best-effort). + + Normalises and de-duplicates the list, then posts requests in chunks of 50 + (GitHub API limit). When a chunk returns 422, each reviewer in that chunk + is retried individually so as many re-requests succeed as possible. + """ + # Normalise and de-duplicate while preserving order. + seen: Set[str] = set() + normalized: List[str] = [] + for reviewer in reviewers: + if reviewer and reviewer not in seen: + seen.add(reviewer) + normalized.append(reviewer) + + if not normalized: + return + + url = f"https://api.github.com/repos/{repo}/pulls/{pr_number}/requested_reviewers" + max_per_request = 50 + successful: List[str] = [] + + for i in range(0, len(normalized), max_per_request): + chunk = normalized[i : i + max_per_request] + try: + make_github_request(token, "POST", url, {"reviewers": chunk}) + successful.extend(chunk) + except urllib.error.HTTPError as http_exc: + if http_exc.code == 422: + print( + "Warning: bulk re-request failed with 422; " + "retrying reviewers individually.", + flush=True, + ) + for reviewer in chunk: + try: + make_github_request(token, "POST", url, {"reviewers": [reviewer]}) + successful.append(reviewer) + except urllib.error.HTTPError as inner_exc: + print( + f"Warning: could not re-request review from " + f"{reviewer}: HTTP {inner_exc.code}", + flush=True, + ) + except Exception as inner_exc: # pylint: disable=broad-except + print( + f"Warning: unexpected error while re-requesting " + f"review from {reviewer}: {inner_exc}", + flush=True, + ) + else: + print( + f"Warning: HTTP {http_exc.code} while re-requesting " + f"reviews for chunk {chunk}", + flush=True, + ) + except Exception as exc: # pylint: disable=broad-except + print( + f"Warning: unexpected error while re-requesting reviews for " + f"chunk {chunk}: {exc}", + flush=True, + ) + + if successful: + print(f"Re-requested review from: {', '.join(successful)}", flush=True) + + +# --------------------------------------------------------------------------- +# Business logic +# --------------------------------------------------------------------------- + +def find_latest_approval_per_user(reviews: List[Dict]) -> Dict[str, Dict]: + """ + Return the most recently submitted APPROVED review for each reviewer. + + Reviews are assumed to be in chronological order. The returned mapping + has GitHub login strings as keys and the reviewer's latest APPROVED review + dict as values. Reviewers with no APPROVED review are absent from the + result. + """ + per_user: Dict[str, Dict] = {} + for review in reviews: + if review.get("state") == "APPROVED": + login: str = review["user"]["login"] + per_user[login] = review + return per_user + + +def is_merge_from_main(commit_data: Dict, main_branch: str = "main") -> bool: + """ + Return True when *commit_data* represents a merge of the main branch into + the PR branch. + + Criteria (both must hold): + 1. The commit has two or more parents (i.e. it is a merge commit). + 2. The commit message matches one of the standard patterns produced by + ``git merge``, GitHub's "Update branch" button, or similar tooling. + """ + parents = commit_data.get("parents", []) + if len(parents) < 2: + return False + + message: str = commit_data.get("commit", {}).get("message", "").strip() + escaped = re.escape(main_branch) + patterns = [ + rf"Merge branch '{escaped}'", + rf"Merge remote-tracking branch 'origin/{escaped}'", + rf"Merge remote-tracking branch 'upstream/{escaped}'", + rf"Merge refs/heads/{escaped}", + ] + return any(re.search(pat, message, re.IGNORECASE) for pat in patterns) + + +def calculate_changes_after_approval( + token: str, + repo: str, + pr_commits: List[Dict], + approval_commit_id: str, + main_branch: str = "main", +) -> Tuple[int, int]: + """ + Count lines changed (additions + deletions) for commits pushed **after** + *approval_commit_id*, ignoring any merge-from-main commits. + + Returns + ------- + (changes, evaluated) + changes — total additions + deletions across non-merge commits + evaluated — number of non-merge commits evaluated + Special: changes == -1 signals that the approval commit was not found. + """ + approval_index: Optional[int] = None + for i, commit in enumerate(pr_commits): + if commit["sha"] == approval_commit_id: + approval_index = i + break + + if approval_index is None: + return -1, 0 # approval commit not found in PR history + + commits_after = pr_commits[approval_index + 1:] + if not commits_after: + return 0, 0 + + total_changes = 0 + evaluated = 0 + for commit_info in commits_after: + sha: str = commit_info["sha"] + details = get_commit_details(token, repo, sha) + if is_merge_from_main(details, main_branch): + print(f" {sha[:8]}: merge from '{main_branch}' — skipped", flush=True) + continue + + stats = details.get("stats", {}) + additions: int = stats.get("additions", 0) + deletions: int = stats.get("deletions", 0) + commit_changes = additions + deletions + print(f" {sha[:8]}: +{additions}/-{deletions} = {commit_changes} LOC", flush=True) + total_changes += commit_changes + evaluated += 1 + if total_changes > LOC_THRESHOLD: + print( + f" Total changes {total_changes} exceeds threshold {LOC_THRESHOLD}; " + "stopping further evaluation.", + flush=True, + ) + break + + return total_changes, evaluated + + +def run_gate( + token: str, + repo: str, + pr_number: int, + main_branch: str = "main", +) -> bool: + """ + Execute the full reapproval-gate logic. + + Returns True (pass) or False (fail — re-approval required). + """ + print(f"=== Reapproval Gate PR #{pr_number} repo: {repo} ===", flush=True) + print(f"Main branch: {main_branch} Threshold: {LOC_THRESHOLD} LOC", flush=True) + + reviews = get_reviews(token, repo, pr_number) + approvals_per_user = find_latest_approval_per_user(reviews) + + if not approvals_per_user: + print("No approvals found — gate passes (nothing to protect).", flush=True) + return True + + pr_commits = get_pr_commits(token, repo, pr_number) + print(f"Total commits in PR: {len(pr_commits)}", flush=True) + + stale_approvers: List[str] = [] + for login, approval in approvals_per_user.items(): + approval_commit: str = approval["commit_id"] + print(f"\nEvaluating approval by {login!r} at commit {approval_commit}:", flush=True) + + changes, evaluated = calculate_changes_after_approval( + token, repo, pr_commits, approval_commit, main_branch + ) + + if changes == -1: + print( + f" WARNING: approval commit {approval_commit} not found in PR commit history " + "(the branch may have been force-pushed) — marking stale.", + flush=True, + ) + stale_approvers.append(login) + elif changes > LOC_THRESHOLD: + print( + f" STALE: {changes} LOC of changes after approval exceeds the " + f"{LOC_THRESHOLD} LOC threshold. Commits evaluated: {evaluated}.", + flush=True, + ) + stale_approvers.append(login) + else: + print( + f" VALID: {changes} LOC changed after approval is within the " + f"{LOC_THRESHOLD} LOC threshold. Commits evaluated: {evaluated}.", + flush=True, + ) + + if stale_approvers: + print( + f"\nFAIL: Re-approval required from: {', '.join(stale_approvers)}", + flush=True, + ) + request_re_review(token, repo, pr_number, stale_approvers) + return False + + print("\nPASS: All approvals are current.", flush=True) + return True + + +# --------------------------------------------------------------------------- +# Entry point +# --------------------------------------------------------------------------- + +def main() -> int: + """Read environment variables and run the gate; return an exit code.""" + token = os.environ.get("GITHUB_TOKEN", "") + repo = os.environ.get("GITHUB_REPOSITORY", "") + pr_number_str = os.environ.get("PR_NUMBER", "") + main_branch = os.environ.get("MAIN_BRANCH", "main") + + if not token: + print("ERROR: GITHUB_TOKEN environment variable is not set.", flush=True) + return 1 + if not repo: + print("ERROR: GITHUB_REPOSITORY environment variable is not set.", flush=True) + return 1 + if not pr_number_str: + print("ERROR: PR_NUMBER environment variable is not set.", flush=True) + return 1 + try: + pr_number = int(pr_number_str) + except ValueError: + print(f"ERROR: PR_NUMBER '{pr_number_str}' is not a valid integer.", flush=True) + return 1 + + passed = run_gate(token, repo, pr_number, main_branch) + return 0 if passed else 1 + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/.github/scripts/test_reapproval_gate.py b/.github/scripts/test_reapproval_gate.py new file mode 100644 index 0000000000..731f5093e1 --- /dev/null +++ b/.github/scripts/test_reapproval_gate.py @@ -0,0 +1,305 @@ +#!/usr/bin/env python3 +# Copyright 2026 Intel Corporation +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +""" +Unit tests for the reapproval_gate.py core business logic. + +These tests exercise only pure functions that do **not** call the GitHub API, +so they run offline with no credentials required. + +Run with: + python .github/scripts/test_reapproval_gate.py +""" + +import os +import sys +import unittest +from unittest import mock + +# Ensure the scripts directory is importable regardless of CWD. +sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) + +from reapproval_gate import ( # noqa: E402 (import after sys.path manipulation) + LOC_THRESHOLD, + find_latest_approval_per_user, + is_merge_from_main, + run_gate, +) + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + +def _make_commit(message: str, num_parents: int = 1) -> dict: + """Build a minimal commit payload as returned by the GitHub API.""" + return { + "commit": {"message": message}, + "parents": [{"sha": f"parent{i}"} for i in range(num_parents)], + } + + +def _make_review(state: str, login: str = "reviewer", commit_id: str = "abc123") -> dict: + """Build a minimal review payload as returned by the GitHub API.""" + return {"state": state, "user": {"login": login}, "commit_id": commit_id} + + +# --------------------------------------------------------------------------- +# is_merge_from_main +# --------------------------------------------------------------------------- + +class TestIsMergeFromMain(unittest.TestCase): + """Tests for is_merge_from_main().""" + + def test_regular_commit_single_parent(self): + """A normal commit with one parent is never a merge.""" + commit = _make_commit("Fix a bug", num_parents=1) + self.assertFalse(is_merge_from_main(commit)) + + def test_merge_from_main_standard_message(self): + """Standard 'Merge branch 'main'' message with two parents.""" + commit = _make_commit("Merge branch 'main' into feature-xyz", num_parents=2) + self.assertTrue(is_merge_from_main(commit)) + + def test_merge_from_main_without_into_clause(self): + """Short-form merge message created by some git flows.""" + commit = _make_commit("Merge branch 'main'", num_parents=2) + self.assertTrue(is_merge_from_main(commit)) + + def test_merge_from_main_origin_remote_tracking(self): + """GitHub 'Update branch' button uses this message format.""" + commit = _make_commit( + "Merge remote-tracking branch 'origin/main' into feature", + num_parents=2, + ) + self.assertTrue(is_merge_from_main(commit)) + + def test_merge_from_main_upstream_remote_tracking(self): + """Upstream remote tracking branch variant.""" + commit = _make_commit( + "Merge remote-tracking branch 'upstream/main'", + num_parents=2, + ) + self.assertTrue(is_merge_from_main(commit)) + + def test_merge_from_main_refs_heads(self): + """refs/heads/ variant occasionally produced by tooling.""" + commit = _make_commit("Merge refs/heads/main", num_parents=2) + self.assertTrue(is_merge_from_main(commit)) + + def test_merge_from_other_branch_not_main(self): + """Merging a non-main branch should not match.""" + commit = _make_commit("Merge branch 'feature-other' into feature-xyz", num_parents=2) + self.assertFalse(is_merge_from_main(commit)) + + def test_no_parents_not_merge(self): + """A root commit (no parents) cannot be a merge.""" + commit = _make_commit("Initial commit", num_parents=0) + self.assertFalse(is_merge_from_main(commit)) + + def test_single_parent_with_merge_message_not_merge(self): + """Message alone is not sufficient; two parents are required.""" + commit = _make_commit("Merge branch 'main' into feature", num_parents=1) + self.assertFalse(is_merge_from_main(commit)) + + def test_case_insensitive_matching(self): + """Pattern matching must be case-insensitive.""" + commit = _make_commit("Merge Branch 'main' Into Feature", num_parents=2) + self.assertTrue(is_merge_from_main(commit)) + + def test_custom_main_branch_name(self): + """The main_branch parameter allows different default-branch names.""" + commit = _make_commit("Merge branch 'develop'", num_parents=2) + self.assertTrue(is_merge_from_main(commit, main_branch="develop")) + + def test_custom_main_branch_does_not_match_wrong_branch(self): + """Custom branch name must not accidentally match other branches.""" + commit = _make_commit("Merge branch 'develop'", num_parents=2) + self.assertFalse(is_merge_from_main(commit, main_branch="main")) + + def test_special_regex_chars_in_branch_name_escaped(self): + """Branch names with regex special characters are handled safely.""" + commit = _make_commit("Merge branch 'release/1.0'", num_parents=2) + self.assertTrue(is_merge_from_main(commit, main_branch="release/1.0")) + + def test_three_parent_merge_from_main(self): + """Octopus merges (3+ parents) with main-branch message are detected.""" + commit = _make_commit("Merge branch 'main' into feature", num_parents=3) + self.assertTrue(is_merge_from_main(commit)) + + def test_squash_commit_with_main_mention_not_merge(self): + """A squash commit (1 parent) mentioning 'main' is not a merge.""" + commit = _make_commit( + "Squash and merge: Merge branch 'main' fixes", + num_parents=1, + ) + self.assertFalse(is_merge_from_main(commit)) + + +# --------------------------------------------------------------------------- +# find_latest_approval_per_user +# --------------------------------------------------------------------------- + +class TestFindLatestApprovalPerUser(unittest.TestCase): + """Tests for find_latest_approval_per_user().""" + + def test_empty_reviews_returns_empty_dict(self): + """No reviews → no approvals → empty mapping.""" + self.assertEqual(find_latest_approval_per_user([]), {}) + + def test_single_approval_in_result(self): + """One APPROVED review produces one entry.""" + reviews = [_make_review("APPROVED", login="alice", commit_id="sha1")] + result = find_latest_approval_per_user(reviews) + self.assertIn("alice", result) + self.assertEqual(result["alice"]["commit_id"], "sha1") + + def test_only_changes_requested_returns_empty(self): + """CHANGES_REQUESTED alone never produces an entry.""" + self.assertEqual(find_latest_approval_per_user([_make_review("CHANGES_REQUESTED")]), {}) + + def test_only_comment_returns_empty(self): + """COMMENTED reviews produce no entries.""" + self.assertEqual(find_latest_approval_per_user([_make_review("COMMENTED")]), {}) + + def test_dismissed_not_included(self): + """DISMISSED reviews are not counted as approvals.""" + self.assertEqual(find_latest_approval_per_user([_make_review("DISMISSED")]), {}) + + def test_same_user_multiple_approvals_latest_kept(self): + """When the same user approves twice, only their latest approval is kept.""" + reviews = [ + _make_review("APPROVED", login="alice", commit_id="sha1"), + _make_review("APPROVED", login="alice", commit_id="sha2"), + ] + result = find_latest_approval_per_user(reviews) + self.assertEqual(len(result), 1) + self.assertEqual(result["alice"]["commit_id"], "sha2") + + def test_approval_after_changes_requested_tracked(self): + """A fresh approval after CHANGES_REQUESTED appears in the result.""" + reviews = [ + _make_review("CHANGES_REQUESTED", login="alice", commit_id="sha1"), + _make_review("APPROVED", login="alice", commit_id="sha2"), + ] + result = find_latest_approval_per_user(reviews) + self.assertIn("alice", result) + self.assertEqual(result["alice"]["commit_id"], "sha2") + + def test_changes_requested_after_approval_approval_still_tracked(self): + """An approval is still recorded even if CHANGES_REQUESTED follows.""" + reviews = [ + _make_review("APPROVED", login="alice", commit_id="sha1"), + _make_review("CHANGES_REQUESTED", login="alice", commit_id="sha2"), + ] + result = find_latest_approval_per_user(reviews) + self.assertIn("alice", result) + self.assertEqual(result["alice"]["commit_id"], "sha1") + + def test_commented_reviews_ignored(self): + """COMMENTED reviews have no effect on the result.""" + reviews = [ + _make_review("COMMENTED", login="alice"), + _make_review("APPROVED", login="alice", commit_id="sha1"), + _make_review("COMMENTED", login="alice"), + ] + result = find_latest_approval_per_user(reviews) + self.assertEqual(result["alice"]["commit_id"], "sha1") + + def test_multiple_reviewers_tracked_independently(self): + """Each reviewer's approval is tracked independently.""" + reviews = [ + _make_review("APPROVED", login="alice", commit_id="sha1"), + _make_review("APPROVED", login="bob", commit_id="sha2"), + ] + result = find_latest_approval_per_user(reviews) + self.assertIn("alice", result) + self.assertIn("bob", result) + self.assertEqual(result["alice"]["commit_id"], "sha1") + self.assertEqual(result["bob"]["commit_id"], "sha2") + + def test_one_reviewer_stale_other_valid(self): + """ + The key per-reviewer scenario: A approved at the latest commit (sha2), + B approved at an earlier commit (sha1). Both entries are present with + their respective commit IDs so the gate can evaluate each independently. + """ + reviews = [ + _make_review("APPROVED", login="bob", commit_id="sha1"), + _make_review("APPROVED", login="alice", commit_id="sha2"), + ] + result = find_latest_approval_per_user(reviews) + # Both reviewers are present + self.assertIn("alice", result) + self.assertIn("bob", result) + # Alice's approval is at the newer commit; Bob's is at the older one. + self.assertEqual(result["alice"]["commit_id"], "sha2") + self.assertEqual(result["bob"]["commit_id"], "sha1") + + +# --------------------------------------------------------------------------- +# Threshold decision (run_gate with all network calls mocked) +# --------------------------------------------------------------------------- + +class TestThresholdDecision(unittest.TestCase): + """Verify the gate's pass/fail decision against LOC_THRESHOLD. + + All GitHub API calls are replaced by in-process stubs so these tests run + fully offline with no credentials required. + """ + + @staticmethod + def _make_commit_details(additions: int, deletions: int) -> dict: + """Minimal commit-detail payload as returned by the GitHub API.""" + return { + "sha": "sha2", + "parents": [{"sha": "sha1"}], # single parent → not a merge commit + "commit": {"message": "Ordinary change"}, + "stats": {"additions": additions, "deletions": deletions}, + } + + def _run_gate_with_churn(self, churn: int) -> bool: + """Run the gate for a single-reviewer PR whose one post-approval commit + carries *churn* total lines changed (additions + deletions). + """ + reviews = [_make_review("APPROVED", login="alice", commit_id="sha1")] + pr_commits = [{"sha": "sha1"}, {"sha": "sha2"}] + commit_details = self._make_commit_details( + additions=churn // 2, deletions=churn - churn // 2 + ) + with mock.patch("reapproval_gate.get_reviews", return_value=reviews), \ + mock.patch("reapproval_gate.get_pr_commits", return_value=pr_commits), \ + mock.patch("reapproval_gate.get_commit_details", return_value=commit_details), \ + mock.patch("reapproval_gate.request_re_review"): + return run_gate("fake-token", "owner/repo", 1) + + def test_gate_fails_when_changes_exceed_threshold(self): + """LOC_THRESHOLD+1 lines changed after approval → gate fails (re-approval required).""" + self.assertFalse(self._run_gate_with_churn(LOC_THRESHOLD + 1)) + + def test_gate_passes_when_changes_below_threshold(self): + """LOC_THRESHOLD-1 lines changed after approval → gate passes.""" + self.assertTrue(self._run_gate_with_churn(LOC_THRESHOLD - 1)) + + +# --------------------------------------------------------------------------- +# Main +# --------------------------------------------------------------------------- + +if __name__ == "__main__": + runner = unittest.TextTestRunner(verbosity=2) + result = runner.run(unittest.TestLoader().loadTestsFromModule( + sys.modules[__name__] + )) + sys.exit(0 if result.wasSuccessful() else 1) diff --git a/.github/workflows/reapproval-gate.yml b/.github/workflows/reapproval-gate.yml new file mode 100644 index 0000000000..efb85a2947 --- /dev/null +++ b/.github/workflows/reapproval-gate.yml @@ -0,0 +1,41 @@ +name: Reapproval Gate + +# Run only when new commits are pushed to an open PR, or when a review is +# submitted (so the gate re-evaluates after a fresh approval). +on: + pull_request: + types: [synchronize] + pull_request_review: + types: [submitted] + +# Least-privilege token: read code, write pull-request reviews/checks. +permissions: + contents: read + pull-requests: write + +jobs: + reapproval-gate: + name: reapproval-gate + runs-on: ubuntu-latest + + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.base.sha }} + + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: '3.11' + + - name: Run unit tests for gate logic + run: python .github/scripts/test_reapproval_gate.py + + - name: Run reapproval gate + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + GITHUB_REPOSITORY: ${{ github.repository }} + PR_NUMBER: ${{ github.event.pull_request.number }} + MAIN_BRANCH: main + run: python .github/scripts/reapproval_gate.py diff --git a/spelling-whitelist.txt b/spelling-whitelist.txt index 804eb612de..2cfb90c7bf 100644 --- a/spelling-whitelist.txt +++ b/spelling-whitelist.txt @@ -25,3 +25,4 @@ release_files/thirdparty-licenses/libgt2.LICENSE.txt:1040: aheared ==> adhered release_files/thirdparty-licenses/libgt2.LICENSE.txt:1065: rouines ==> routines release_files/thirdparty-licenses/libgt2.LICENSE.txt:1083: publically ==> publicly src/test/llm/output_parsers/qwen3coder_output_parser_test.cpp +.github/scripts/test_reapproval_gate.py