From e719d10eee31b8a1764f75382f1c4ca69579fb26 Mon Sep 17 00:00:00 2001 From: prathmeshkulkarni-coder Date: Tue, 7 Jul 2026 12:57:35 +0530 Subject: [PATCH] [feature] Flag invalid pull requests that do not reference validated issues #709 - Add pull_request_target edited/ready_for_review triggers - Implement regex issue references extraction helper - Add GraphQL project v2 board assignment check - Validate that PRs from external contributors link to open, labeled, and validated organization issues - Apply 'invalid' label and post warning comment if invalid - Automatically close invalid PRs after 24 hours in the stale PR workflow - Update documentation and add unit tests Closes #709 --- .github/actions/bot-autoassign/base.py | 261 ++++++++++++++++ .../bot-autoassign/issue_assignment_bot.py | 39 ++- .../actions/bot-autoassign/stale_pr_bot.py | 93 ++++-- .../tests/test_issue_assignment_bot.py | 292 ++++++++++++++++++ .../bot-autoassign/tests/test_stale_pr_bot.py | 88 ++++++ .github/actions/bot-autoassign/utils.py | 37 +++ .../bot-autoassign-pr-issue-link.yml | 5 +- .github/workflows/reusable-bot-autoassign.yml | 21 +- docs/developer/reusable-github-utils.rst | 61 +++- setup.py | 2 +- 10 files changed, 853 insertions(+), 46 deletions(-) diff --git a/.github/actions/bot-autoassign/base.py b/.github/actions/bot-autoassign/base.py index 08e050227..b14a5e57f 100644 --- a/.github/actions/bot-autoassign/base.py +++ b/.github/actions/bot-autoassign/base.py @@ -2,6 +2,14 @@ from github import Github +MAINTAINER_ROLES = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) +DEFAULT_EXCLUDE_PR_AUTHORS = "dependabot[bot]" +REQUIRED_CONTRIBUTOR_PROJECTS = ( + "OpenWISP Contributor's Board", + "OpenWISP Priorities for next releases", +) +INVALID_ISSUE_LABELS = frozenset({"invalid", "wontfix"}) + class GitHubBot: def __init__(self): @@ -9,6 +17,11 @@ def __init__(self): self.repository_name = os.environ.get("REPOSITORY") self.event_name = os.environ.get("GITHUB_EVENT_NAME") self.event_payload = None + bot_username = os.environ.get("BOT_USERNAME", "openwisp-companion") + self.bot_username = bot_username + self.bot_login = ( + bot_username if bot_username.endswith("[bot]") else f"{bot_username}[bot]" + ) if self.github_token and self.repository_name: try: @@ -25,3 +38,251 @@ def __init__(self): def load_event_payload(self, event_payload): self.event_payload = event_payload + + @staticmethod + def _normalize_project_title(title: str) -> str: + """Normalize project titles for stable comparisons.""" + return " ".join(title.replace("\u2019", "'").split()).strip() + + @classmethod + def _project_title_key(cls, title: str) -> str: + """Loose match key: casefold + ignore apostrophes.""" + normalized = cls._normalize_project_title(title).casefold() + return normalized.replace("'", "") + + def get_issue_projects(self, owner, repo_name, issue_number): + query = """ + query($owner: String!, $repo: String!, $issueNumber: Int!) { + repository(owner: $owner, name: $repo) { + issue(number: $issueNumber) { + projectItems(first: 10) { + nodes { + project { + title + } + } + } + } + } + } + """ + variables = {"owner": owner, "repo": repo_name, "issueNumber": issue_number} + headers, result = self.github.requester.graphql_query(query, variables) + print(f"DEBUG: Raw GraphQL response: {result}") + + if "errors" in result: + raise ValueError(f"GraphQL API Permission Error: {result['errors']}") + + repo_data = result.get("data", {}).get("repository") + if not repo_data: + raise ValueError( + f"GraphQL could not access repository {owner}/{repo_name}; " + "possible GitHub API or permission error" + ) + issue_node = repo_data.get("issue") + if issue_node is None: + raise ValueError( + f"GraphQL could not access issue {owner}/{repo_name}#{issue_number}; " + "possible GitHub API or permission error" + ) + project_items = issue_node.get("projectItems") + if project_items is None: + raise ValueError( + f"GraphQL could not read project assignments for " + f"{owner}/{repo_name}#{issue_number}; " + "possible GitHub API or permission error" + ) + nodes = project_items.get("nodes") or [] + projects = [] + for node in nodes: + if not node: + continue + project = node.get("project") or {} + title = project.get("title") + if title: + projects.append(self._normalize_project_title(title)) + return projects + + def validate_pr_issues(self, pr): + """Validate if a pull request is from an exempt user or references a validated issue.""" + if not self.github or not self.repository_name: + print("GitHub client or repository name not initialized") + return False + + pr_author = ( + pr.user.login + if pr.user and isinstance(getattr(pr.user, "login", None), str) + else "" + ) + + exclude_authors_env = os.environ.get( + "EXCLUDE_PR_AUTHORS", DEFAULT_EXCLUDE_PR_AUTHORS + ) + excluded_authors = [ + auth.strip() for auth in exclude_authors_env.split(",") if auth.strip() + ] + if pr_author in excluded_authors: + print(f"Author {pr_author} is in the exclude list. Proceeding.") + return True + + author_association = str(getattr(pr, "author_association", "") or "") + if author_association in MAINTAINER_ROLES: + print( + f"Author {pr_author} is exempt due to association: " + f"{author_association}. Proceeding." + ) + return True + + from utils import extract_all_linked_issues + + pr_body = pr.body if isinstance(pr.body, str) else "" + linked_issues = extract_all_linked_issues(pr_body, self.repository_name) + if not linked_issues: + print("No linked issues found in PR body for external contributor.") + return False + + current_org = self.repository_name.split("/")[0].lower() + required_projects = { + self._project_title_key(p) for p in REQUIRED_CONTRIBUTOR_PROJECTS + } + + for owner, repo_name, issue_number in linked_issues: + if owner.lower() != current_org: + print( + f"Issue {owner}/{repo_name}#{issue_number} does not belong " + f"to organization {current_org}, skipping validation." + ) + continue + + try: + from github import GithubException + + target_repo = self.github.get_repo(f"{owner}/{repo_name}") + issue = target_repo.get_issue(issue_number) + except Exception as e: + if isinstance(e, GithubException) and e.status == 404: + print( + f"Issue {owner}/{repo_name}#{issue_number} not found, skipping validation." + ) + continue + print(f"Error fetching issue {owner}/{repo_name}#{issue_number}: {e}") + raise + + if issue.pull_request: + print( + f"Reference {owner}/{repo_name}#{issue_number} is a pull request, skipping validation." + ) + continue + + if issue.state != "open": + print( + f"Issue {owner}/{repo_name}#{issue_number} is not open, skipping validation." + ) + continue + + issue_labels = [label.name.lower() for label in issue.labels] + valid_labels = [ + lbl for lbl in issue_labels if lbl not in INVALID_ISSUE_LABELS + ] + if not valid_labels: + print( + f"Issue {owner}/{repo_name}#{issue_number} has no valid labels, skipping validation." + ) + continue + if any(lbl in INVALID_ISSUE_LABELS for lbl in issue_labels): + print( + f"Issue {owner}/{repo_name}#{issue_number} contains " + "invalid/wontfix label, skipping validation." + ) + continue + + try: + projects = self.get_issue_projects(owner, repo_name, issue_number) + except Exception as e: + print( + f"Error fetching projects for issue {owner}/{repo_name}#{issue_number}: {e}" + ) + raise + + print( + f"DEBUG: Raw projects for {owner}/{repo_name}#{issue_number}: {projects}" + ) + project_keys = [self._project_title_key(p) for p in projects] + print(f"DEBUG: Normalized project keys: {project_keys}") + print(f"DEBUG: Required project keys: {required_projects}") + + has_valid_project = any(key in required_projects for key in project_keys) + if has_valid_project: + print( + f"Issue {owner}/{repo_name}#{issue_number} is validated. PR is valid." + ) + return True + else: + print( + f"Issue {owner}/{repo_name}#{issue_number} is not assigned " + f"to any required project (found: {projects or 'none'}), " + "skipping validation." + ) + + return False + + def get_bot_comment(self, pr, comment_type, after_date=None, issue_comments=None): + """Get the comment of this bot with the given marker if it exists. + + If ``after_date`` is provided, only considers comments posted after that date. + """ + try: + if issue_comments is None: + issue_comments = list(pr.get_issue_comments()) + marker = f"" + for comment in issue_comments: + if ( + comment.user + and comment.user.login == self.bot_login + and marker in comment.body + ): + if after_date and comment.created_at <= after_date: + continue + return comment + return None + except Exception as e: + print(f"Error getting bot comment for PR #{pr.number}: {e}") + return None + + def has_bot_comment(self, pr, comment_type, after_date=None, issue_comments=None): + """Check if PR already has a specific type of bot comment. + + Uses HTML markers. If ``after_date`` is provided, + only considers comments posted after that date. + """ + return bool(self.get_bot_comment(pr, comment_type, after_date, issue_comments)) + + def get_invalid_unvalidated_issue_comment(self, pr_author): + """Returns the comment body warning that the PR is invalid/unvalidated.""" + greeting = f"Hi @{pr_author},\n\n" if pr_author else "Hi,\n\n" + return ( + "\n\n" + f"{greeting}" + "Thank you for your interest in contributing to OpenWISP.\n\n" + "This pull request has been flagged because external contributors " + "must target an issue validated by maintainers before requesting " + "review.\n\n" + "Please link this pull request to a validated issue by adding " + "`Fixes #ISSUE_NUMBER`, `Closes #ISSUE_NUMBER`, or " + "`Related to #ISSUE_NUMBER` to the pull request description. " + "The issue may be in this repository or another OpenWISP " + "repository.\n\n" + "If there is no validated issue yet, please open one first and wait " + "for maintainer validation before continuing with this pull " + "request.\n\n" + "An issue is considered validated when it is open, has an appropriate " + "label other than `invalid` or `wontfix`, and is assigned to one of " + "the OpenWISP contributor project boards mentioned in the " + "contributing guidelines.\n\n" + "Please see the OpenWISP policy on unsolicited and AI-assisted " + "contributions:\n" + "https://openwisp.io/docs/dev/general/code-of-conduct.html\n\n" + "If this is not resolved within 24 hours, this pull request " + "will be closed automatically. " + "Thank you for your understanding." + ) diff --git a/.github/actions/bot-autoassign/issue_assignment_bot.py b/.github/actions/bot-autoassign/issue_assignment_bot.py index 156666df0..1fe3529fe 100644 --- a/.github/actions/bot-autoassign/issue_assignment_bot.py +++ b/.github/actions/bot-autoassign/issue_assignment_bot.py @@ -1,4 +1,3 @@ -import os import re from base import GitHubBot @@ -14,9 +13,6 @@ class IssueAssignmentBot(GitHubBot): - def __init__(self): - super().__init__() - self.bot_username = os.environ.get("BOT_USERNAME", "openwisp-companion") def is_bot_assign_command(self, comment_body): if not comment_body: @@ -440,16 +436,41 @@ def handle_pull_request(self): if not all([pr_number, pr_author]): print("Missing required PR data") return False - if action in ["opened", "reopened"]: - self.auto_assign_issues_from_pr(pr_number, pr_author, pr_body) - # We consider the event handled even if no issues were linked - return True - elif action == "closed": + if action == "closed": if pr.get("merged", False): print(f"PR #{pr_number} was merged, keeping issue assignments") else: self.unassign_issues_from_pr(pr_body, pr_author) return True + + if action in ["opened", "reopened", "edited", "ready_for_review"]: + self.auto_assign_issues_from_pr(pr_number, pr_author, pr_body) + pr_obj = self.repo.get_pull(pr_number) + is_valid = self.validate_pr_issues(pr_obj) + labels_lower = set() + try: + labels_lower = {label.name.lower() for label in pr_obj.labels} + except (TypeError, AttributeError): + pass + + if is_valid: + if "invalid" in labels_lower: + pr_obj.remove_from_labels("invalid") + print(f"Removed 'invalid' label from PR #{pr_number}") + else: + if "invalid" not in labels_lower: + pr_obj.add_to_labels("invalid") + print(f"Added 'invalid' label to PR #{pr_number}") + + if not self.has_bot_comment(pr_obj, "invalid_unvalidated_issue"): + comment_body = self.get_invalid_unvalidated_issue_comment( + pr_author + ) + pr_obj.create_issue_comment(comment_body) + print( + f"Posted unvalidated issue warning comment on PR #{pr_number}" + ) + return True print(f"PR action '{action}' not handled") return True except Exception as e: diff --git a/.github/actions/bot-autoassign/stale_pr_bot.py b/.github/actions/bot-autoassign/stale_pr_bot.py index 3ebb50f63..b7014e58c 100644 --- a/.github/actions/bot-autoassign/stale_pr_bot.py +++ b/.github/actions/bot-autoassign/stale_pr_bot.py @@ -1,16 +1,16 @@ -import os import time from datetime import datetime, timezone -from base import GitHubBot +from base import MAINTAINER_ROLES, GitHubBot from utils import ( extract_linked_issues, get_valid_linked_issues, unassign_linked_issues_helper, ) -# GitHub author_association values that represent project maintainers. -MAINTAINER_ROLES = frozenset({"OWNER", "MEMBER", "COLLABORATOR"}) + +class ValidationAPIError(Exception): + pass class StalePRBot(GitHubBot): @@ -19,10 +19,6 @@ def __init__(self): self.DAYS_BEFORE_STALE_WARNING = 7 self.DAYS_BEFORE_UNASSIGN = 14 self.DAYS_BEFORE_FINAL_FOLLOWUP = 60 - bot_username = os.environ.get("BOT_USERNAME", "openwisp-companion") - self.bot_login = ( - bot_username if bot_username.endswith("[bot]") else f"{bot_username}[bot]" - ) @staticmethod def _commit_activity_date_for_author(commit, pr_author): @@ -195,28 +191,6 @@ def get_last_changes_requested(self, pr, all_reviews=None): default=None, ) - def has_bot_comment(self, pr, comment_type, after_date=None, issue_comments=None): - """Check if this bot has already posted a comment with the given - marker. If ``after_date`` is set, only later comments count. - """ - try: - if issue_comments is None: - issue_comments = list(pr.get_issue_comments()) - marker = f"" - for comment in issue_comments: - if ( - comment.user - and comment.user.login == self.bot_login - and marker in comment.body - ): - if after_date and comment.created_at <= after_date: - continue - return True - return False - except Exception as e: - print(f"Error checking bot comments for PR #{pr.number}: {e}") - return False - def unassign_linked_issues(self, pr): pr_author = pr.user.login if pr.user else None if not pr_author: @@ -395,6 +369,63 @@ def process_stale_prs(self): for pr in open_prs: pr_count += 1 try: + pr_labels = [label.name.lower() for label in pr.labels] + if "invalid" in pr_labels: + try: + is_valid = self.validate_pr_issues(pr) + except Exception as e: + print( + f"Critical issue-project verification error on PR #{pr.number}: {e}" + ) + raise ValidationAPIError(e) + + if is_valid: + pr.remove_from_labels("invalid") + print( + f"PR #{pr.number} is now valid. Removed 'invalid' label." + ) + else: + comments = list(pr.get_issue_comments()) + warning_comment = self.get_bot_comment( + pr, "invalid_unvalidated_issue", issue_comments=comments + ) + if warning_comment: + now = datetime.now(timezone.utc) + if ( + now - warning_comment.created_at + ).total_seconds() >= 24 * 3600: + close_message = ( + "\n\n" + "This pull request has been automatically closed because it has " + "been flagged as invalid (not referencing a validated issue) " + "for more than 24 hours." + ) + try: + pr.create_issue_comment(close_message) + except Exception as comment_error: + print( + f"Warning: Could not post close comment on PR " + f"#{pr.number}: {comment_error}" + ) + pr.edit(state="closed") + print( + f"Closed PR #{pr.number} automatically after 24 hours." + ) + processed_count += 1 + else: + pr_author = pr.user.login if pr.user else None + comment_body = ( + self.get_invalid_unvalidated_issue_comment( + pr_author + ) + ) + pr.create_issue_comment(comment_body) + print( + f"Posted missing warning comment on PR #{pr.number}" + ) + processed_count += 1 + continue + all_reviews = list(pr.get_reviews()) last_changes_requested = self.get_last_changes_requested( pr, all_reviews @@ -479,6 +510,8 @@ def process_stale_prs(self): posted_stale_this_run = True if action(pr, days_inactive): processed_count += 1 + except ValidationAPIError: + raise except Exception as e: print(f"Error processing PR #{pr.number}: {e}") continue diff --git a/.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py b/.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py index dcd48bb69..3bd8b38a1 100644 --- a/.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py +++ b/.github/actions/bot-autoassign/tests/test_issue_assignment_bot.py @@ -885,3 +885,295 @@ def test_bot_fallback_message_does_not_self_trigger(self, bot_env): ) assert bot.handle_issue_comment() bot_env["repo"].get_issue.assert_not_called() + + +class TestExtractAllLinkedIssues: + @pytest.mark.parametrize( + "pr_body,expected", + [ + ("Fixes #123", [("openwisp", "openwisp-utils", 123)]), + ( + "Fixes openwisp/openwisp-utils#709", + [("openwisp", "openwisp-utils", 709)], + ), + ( + "Fixes https://github.com/openwisp/openwisp-utils/issues/709", + [("openwisp", "openwisp-utils", 709)], + ), + ("Related to #99", [("openwisp", "openwisp-utils", 99)]), + ( + "Closes #456 and resolves #789", + [ + ("openwisp", "openwisp-utils", 456), + ("openwisp", "openwisp-utils", 789), + ], + ), + ("Closes openwisp-utils#42", []), + ("", []), + (None, []), + ], + ) + def test_extract_all_linked_issues(self, pr_body, expected, bot_env): + from utils import extract_all_linked_issues + + assert extract_all_linked_issues(pr_body, "openwisp/openwisp-utils") == expected + + +class TestPRValidation: + def test_get_issue_projects_success(self, bot_env): + bot = IssueAssignmentBot() + bot.github.requester.graphql_query.return_value = ( + {}, + { + "data": { + "repository": { + "issue": { + "projectItems": { + "nodes": [ + { + "project": { + "title": "OpenWISP Contributor's Board" + } + }, + {"project": {"title": "Some Other Board"}}, + ] + } + } + } + } + }, + ) + projects = bot.get_issue_projects("openwisp", "openwisp-utils", 123) + assert projects == ["OpenWISP Contributor's Board", "Some Other Board"] + bot.github.requester.graphql_query.assert_called_once() + + def test_get_issue_projects_single_project(self, bot_env): + bot = IssueAssignmentBot() + bot.github.requester.graphql_query.return_value = ( + {}, + { + "data": { + "repository": { + "issue": { + "projectItems": { + "nodes": [ + {"project": {"title": "Classic Project Board"}}, + ] + } + } + } + } + }, + ) + projects = bot.get_issue_projects("openwisp", "openwisp-utils", 123) + assert projects == ["Classic Project Board"] + + def test_get_issue_projects_graphql_error(self, bot_env): + bot = IssueAssignmentBot() + bot.github.requester.graphql_query.side_effect = GithubException( + 400, {"errors": [{"message": "Some error"}]}, {} + ) + with pytest.raises(GithubException): + bot.get_issue_projects("openwisp", "openwisp-utils", 123) + + def test_get_issue_projects_null_project_items(self, bot_env): + bot = IssueAssignmentBot() + bot.github.requester.graphql_query.return_value = ( + {}, + { + "data": { + "repository": { + "issue": { + "projectItems": None, + } + } + } + }, + ) + with pytest.raises(ValueError, match="could not read project assignments"): + bot.get_issue_projects("openwisp", "openwisp-utils", 123) + + def test_get_issue_projects_null_issue(self, bot_env): + bot = IssueAssignmentBot() + bot.github.requester.graphql_query.return_value = ( + {}, + {"data": {"repository": {"issue": None}}}, + ) + with pytest.raises(ValueError, match="could not access issue"): + bot.get_issue_projects("openwisp", "openwisp-utils", 123) + + def test_validate_pr_issues_excluded_author(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "dependabot[bot]" + assert bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_exempt_association(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "some-member" + mock_pr.author_association = "MEMBER" + assert bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_no_issues(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "external-contributor" + mock_pr.author_association = "NONE" + mock_pr.body = "No references here" + assert not bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_cross_org_issue(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "external-contributor" + mock_pr.author_association = "NONE" + mock_pr.body = "Fixes otherorg/repo#12" + assert not bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_is_pr(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "external-contributor" + mock_pr.author_association = "NONE" + mock_pr.body = "Fixes #12" + + mock_issue = Mock() + mock_issue.pull_request = {"url": "..."} + bot_env["repo"].get_issue.return_value = mock_issue + + assert not bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_closed(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "external-contributor" + mock_pr.author_association = "NONE" + mock_pr.body = "Fixes #12" + + mock_issue = Mock() + mock_issue.pull_request = None + mock_issue.state = "closed" + bot_env["repo"].get_issue.return_value = mock_issue + + assert not bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_invalid_labels(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "external-contributor" + mock_pr.author_association = "NONE" + mock_pr.body = "Fixes #12" + + # Test case 1: No labels + mock_issue = Mock() + mock_issue.pull_request = None + mock_issue.state = "open" + mock_issue.labels = [] + bot_env["repo"].get_issue.return_value = mock_issue + assert not bot.validate_pr_issues(mock_pr) + + # Test case 2: Has wontfix/invalid labels + label_wontfix = Mock() + label_wontfix.name = "wontfix" + mock_issue.labels = [label_wontfix] + assert not bot.validate_pr_issues(mock_pr) + + # Test case 3: Has a mix of valid and invalid/wontfix labels + label_bug = Mock() + label_bug.name = "bug" + mock_issue.labels = [label_bug, label_wontfix] + assert not bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_not_in_project(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "external-contributor" + mock_pr.author_association = "NONE" + mock_pr.body = "Fixes #12" + + label_bug = Mock() + label_bug.name = "bug" + mock_issue = Mock() + mock_issue.pull_request = None + mock_issue.state = "open" + mock_issue.labels = [label_bug] + bot_env["repo"].get_issue.return_value = mock_issue + + with patch.object( + bot, "get_issue_projects", return_value=["Some Other Project"] + ): + assert not bot.validate_pr_issues(mock_pr) + + def test_validate_pr_issues_fully_valid(self, bot_env): + bot = IssueAssignmentBot() + mock_pr = Mock() + mock_pr.user.login = "external-contributor" + mock_pr.author_association = "NONE" + mock_pr.body = "Fixes #12" + + label_bug = Mock() + label_bug.name = "bug" + mock_issue = Mock() + mock_issue.pull_request = None + mock_issue.state = "open" + mock_issue.labels = [label_bug] + bot_env["repo"].get_issue.return_value = mock_issue + + with patch.object( + bot, "get_issue_projects", return_value=["OpenWISP Contributor's Board"] + ): + assert bot.validate_pr_issues(mock_pr) + + def test_handle_pull_request_invalid_label_and_comment(self, bot_env): + bot = IssueAssignmentBot() + bot.event_name = "pull_request_target" + bot.load_event_payload( + { + "action": "opened", + "pull_request": { + "number": 100, + "user": {"login": "testuser"}, + "body": "Fixes #123", + }, + } + ) + + mock_pr_obj = Mock() + mock_pr_obj.labels = [] + bot_env["repo"].get_pull.return_value = mock_pr_obj + + with patch.object(bot, "validate_pr_issues", return_value=False), patch.object( + bot, "has_bot_comment", return_value=False + ): + assert bot.handle_pull_request() + mock_pr_obj.add_to_labels.assert_called_once_with("invalid") + mock_pr_obj.create_issue_comment.assert_called_once() + assert ( + "invalid_unvalidated_issue" + in mock_pr_obj.create_issue_comment.call_args[0][0].lower() + ) + + def test_handle_pull_request_valid_removes_label(self, bot_env): + bot = IssueAssignmentBot() + bot.event_name = "pull_request_target" + bot.load_event_payload( + { + "action": "edited", + "pull_request": { + "number": 100, + "user": {"login": "testuser"}, + "body": "Fixes #123", + }, + } + ) + + mock_label = Mock() + mock_label.name = "invalid" + mock_pr_obj = Mock() + mock_pr_obj.labels = [mock_label] + bot_env["repo"].get_pull.return_value = mock_pr_obj + + with patch.object(bot, "validate_pr_issues", return_value=True): + assert bot.handle_pull_request() + mock_pr_obj.remove_from_labels.assert_called_once_with("invalid") diff --git a/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py b/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py index 4cc926d71..252800147 100644 --- a/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py +++ b/.github/actions/bot-autoassign/tests/test_stale_pr_bot.py @@ -860,3 +860,91 @@ def test_no_github_client(self, bot_env): bot.github = None bot.repo = None assert not bot.run() + + +class TestStalePRBotInvalidCheck: + def test_invalid_pr_becomes_valid(self, bot_env): + bot = StalePRBot() + mock_label = Mock() + mock_label.name = "invalid" + + mock_pr = Mock() + mock_pr.labels = [mock_label] + mock_pr.number = 100 + bot_env["repo"].get_pulls.return_value = [mock_pr] + + with patch.object(bot, "validate_pr_issues", return_value=True): + assert bot.process_stale_prs() + mock_pr.remove_from_labels.assert_called_once_with("invalid") + mock_pr.edit.assert_not_called() + + def test_invalid_pr_still_invalid_under_24h(self, bot_env): + bot = StalePRBot() + mock_label = Mock() + mock_label.name = "invalid" + + mock_pr = Mock() + mock_pr.labels = [mock_label] + mock_pr.number = 100 + bot_env["repo"].get_pulls.return_value = [mock_pr] + + mock_comment = Mock() + mock_comment.user.login = bot.bot_login + mock_comment.body = " Warning comment" + mock_comment.created_at = datetime.now(timezone.utc) + mock_pr.get_issue_comments.return_value = [mock_comment] + + with patch.object(bot, "validate_pr_issues", return_value=False): + assert bot.process_stale_prs() + mock_pr.remove_from_labels.assert_not_called() + mock_pr.edit.assert_not_called() + + def test_invalid_pr_still_invalid_over_24h_closes(self, bot_env): + bot = StalePRBot() + mock_label = Mock() + mock_label.name = "invalid" + + mock_pr = Mock() + mock_pr.labels = [mock_label] + mock_pr.number = 100 + bot_env["repo"].get_pulls.return_value = [mock_pr] + + mock_comment = Mock() + mock_comment.user.login = bot.bot_login + mock_comment.body = " Warning comment" + from datetime import timedelta + + mock_comment.created_at = datetime.now(timezone.utc) - timedelta(hours=25) + mock_pr.get_issue_comments.return_value = [mock_comment] + + with patch.object(bot, "validate_pr_issues", return_value=False): + assert bot.process_stale_prs() + mock_pr.remove_from_labels.assert_not_called() + mock_pr.create_issue_comment.assert_called_once() + assert ( + "automatically closed" in mock_pr.create_issue_comment.call_args[0][0] + ) + mock_pr.edit.assert_called_once_with(state="closed") + + def test_invalid_pr_missing_warning_comment_posts_warning(self, bot_env): + bot = StalePRBot() + mock_label = Mock() + mock_label.name = "invalid" + + mock_pr = Mock() + mock_pr.labels = [mock_label] + mock_pr.number = 100 + mock_pr.user.login = "contributor" + bot_env["repo"].get_pulls.return_value = [mock_pr] + + mock_pr.get_issue_comments.return_value = [] + + with patch.object(bot, "validate_pr_issues", return_value=False): + assert bot.process_stale_prs() + mock_pr.remove_from_labels.assert_not_called() + mock_pr.create_issue_comment.assert_called_once() + assert ( + "" + in mock_pr.create_issue_comment.call_args[0][0] + ) + mock_pr.edit.assert_not_called() diff --git a/.github/actions/bot-autoassign/utils.py b/.github/actions/bot-autoassign/utils.py index fe921a858..42cfe9642 100644 --- a/.github/actions/bot-autoassign/utils.py +++ b/.github/actions/bot-autoassign/utils.py @@ -110,3 +110,40 @@ def unassign_linked_issues_helper(repo, repository_name, pr_body, pr_author): except Exception as e: print(f"Error unassigning issue #{issue_number}: {e}") return unassigned_issues + + +def extract_all_linked_issues(pr_body, default_repo_full_name): + """Extract all linked issues (including cross-repository and URLs). + + Returns a list of tuples: (owner, repo_name, issue_number) + """ + if not pr_body: + return [] + # Pattern to match: + # 1. URL: https://github.com/owner/repo/issues/num + # 2. owner/repo#num + # 3. #num + pattern = ( + r"\b(?:fix(?:e[sd])?|close[sd]?|resolve[sd]?|relat(?:e[sd]?|ed)\s+to)\s*:?\s*" + r"(?:" + r"https://github\.com/([\w-]+)/([\w.-]+)/issues/(\d+)" + r"|([\w-]+)/([\w.-]+)#(\d+)" + r"|#(\d+)" + r")" + ) + matches = re.findall(pattern, pr_body, re.IGNORECASE) + results = [] + default_owner, default_repo = default_repo_full_name.split("/") + for m in matches: + if m[0] and m[1] and m[2]: # URL + results.append((m[0], m[1], int(m[2]))) + elif m[3] and m[4] and m[5]: # owner/repo#num + results.append((m[3], m[4], int(m[5]))) + elif m[6]: # #num + results.append((default_owner, default_repo, int(m[6]))) + # Remove duplicates while preserving order + unique_results = [] + for r in results: + if r not in unique_results: + unique_results.append(r) + return unique_results diff --git a/.github/workflows/bot-autoassign-pr-issue-link.yml b/.github/workflows/bot-autoassign-pr-issue-link.yml index b1860ac3b..f76fa986f 100644 --- a/.github/workflows/bot-autoassign-pr-issue-link.yml +++ b/.github/workflows/bot-autoassign-pr-issue-link.yml @@ -2,12 +2,12 @@ name: PR Issue Auto-Assignment on: pull_request_target: - types: [opened, reopened, closed] + types: [opened, reopened, closed, edited, ready_for_review] permissions: contents: read issues: write - pull-requests: read + pull-requests: write concurrency: group: bot-autoassign-pr-link-${{ github.repository }}-${{ github.event.pull_request.number }}-${{ github.event.action }} @@ -44,6 +44,7 @@ jobs: REPOSITORY: ${{ github.repository }} GITHUB_EVENT_NAME: ${{ github.event_name }} BOT_USERNAME: openwisp-companion + EXCLUDE_PR_AUTHORS: dependabot[bot] run: > python .github/actions/bot-autoassign/__main__.py issue_assignment "$GITHUB_EVENT_PATH" diff --git a/.github/workflows/reusable-bot-autoassign.yml b/.github/workflows/reusable-bot-autoassign.yml index 53c30be88..6f511d051 100644 --- a/.github/workflows/reusable-bot-autoassign.yml +++ b/.github/workflows/reusable-bot-autoassign.yml @@ -12,6 +12,21 @@ on: type: string default: "openwisp-companion" description: "The GitHub App username used to detect bot mentions and bot-authored comments" + openwisp_utils_repo: + required: false + type: string + default: "openwisp/openwisp-utils" + description: "The repository to check out" + openwisp_utils_ref: + required: false + type: string + default: "master" + description: "The ref to check out" + exclude_pr_authors: + required: false + type: string + default: "dependabot[bot]" + description: "Comma-separated GitHub usernames exempt from external PR validation" secrets: OPENWISP_BOT_APP_ID: required: true @@ -28,12 +43,13 @@ jobs: with: app-id: ${{ secrets.OPENWISP_BOT_APP_ID }} private-key: ${{ secrets.OPENWISP_BOT_PRIVATE_KEY }} + owner: ${{ github.repository_owner }} - name: Checkout openwisp-utils uses: actions/checkout@v7 with: - repository: openwisp/openwisp-utils - ref: master + repository: ${{ inputs.openwisp_utils_repo }} + ref: ${{ inputs.openwisp_utils_ref }} path: openwisp-utils - name: Set up Python @@ -51,6 +67,7 @@ jobs: GITHUB_EVENT_NAME: ${{ github.event_name }} BOT_COMMAND: ${{ inputs.bot_command }} BOT_USERNAME: ${{ inputs.bot_username }} + EXCLUDE_PR_AUTHORS: ${{ inputs.exclude_pr_authors }} run: | if [ -n "$GITHUB_EVENT_PATH" ]; then python openwisp-utils/.github/actions/bot-autoassign/__main__.py "$BOT_COMMAND" "$GITHUB_EVENT_PATH" diff --git a/docs/developer/reusable-github-utils.rst b/docs/developer/reusable-github-utils.rst index b7182d5e5..cbb255320 100644 --- a/docs/developer/reusable-github-utils.rst +++ b/docs/developer/reusable-github-utils.rst @@ -73,6 +73,56 @@ OpenWISP repositories. The bot provides the following features: encouragement after 60 days. The bot does not auto-close PRs. - **PR reopen reassignment**: When a stale PR is reopened, linked issues are reassigned back to the author. +- **PR validation**: Flags invalid pull requests from external + contributors that do not reference a validated issue belonging to the + OpenWISP organization (an open issue assigned to one of the required + project boards with at least one label other than ``invalid`` or + ``wontfix``). Invalid pull requests are labeled ``invalid``, receive a + warning comment, and are automatically closed after 24 hours if the + issue is not resolved. + +**How External PR Validation Works** + +The PR issue-link workflow runs on ``pull_request_target`` events +(``opened``, ``reopened``, ``closed``, ``edited``, and +``ready_for_review``). For each pull request: + +1. **Maintainer exemption.** Pull requests authored by users with + ``OWNER``, ``MEMBER``, or ``COLLABORATOR`` ``author_association`` + values proceed normally without issue validation. The same applies to + authors listed in the ``EXCLUDE_PR_AUTHORS`` environment variable + (``dependabot[bot]`` by default). +2. **Validated issue requirement.** External contributors must reference + at least one validated issue in the pull request description using + ``Fixes #ISSUE_NUMBER``, ``Closes #ISSUE_NUMBER``, ``Related to + #ISSUE_NUMBER``, or an equivalent cross-repository reference. Only the + pull request description is checked (not commit messages or the title). +3. **Validated issue criteria.** A linked issue is considered validated + when all of the following are true: + + - it is open; + - it belongs to the same GitHub organization as the repository; + - it has at least one label other than ``invalid`` or ``wontfix``; + - it is assigned to one of these OpenWISP contributor project boards: + + - OpenWISP Contributor's Board + - OpenWISP Priorities for next releases + +4. **Invalid pull request handling.** When validation fails, the bot adds + the standard ``invalid`` label, posts a single explanatory comment + (tracked with a hidden HTML marker), and explains how to fix the pull + request. Draft and documentation-only pull requests are not exempt. +5. **Recovery and auto-close.** When a contributor updates the pull + request description to reference a validated issue, the next workflow + run removes the ``invalid`` label automatically. The daily stale PR job + re-checks pull requests labeled ``invalid``: if validation still fails + 24 hours after the warning comment, the pull request is closed + automatically. Maintainers should validate the linked issue (open, + labeled, and assigned to a contributor project board) rather than + removing the ``invalid`` label manually. +6. **API failures.** If the bot cannot verify project-board assignment + because of a GitHub API or permission error, the workflow fails and is + retried on the next run as usual. **How Stale PR Detection Works** @@ -117,6 +167,13 @@ defaults to ``openwisp-companion``). - ``OPENWISP_BOT_PRIVATE_KEY`` (required): OpenWISP Bot GitHub App private key. +The OpenWISP Bot GitHub App must also have **Projects: Read** permission +(enabled at the organization level). PR validation uses a lightweight +GraphQL query on each linked issue's ``projectItems`` to read only the +assigned project title — it does not traverse or modify project boards. +Without this permission, the API returns no project data and external pull +requests may be falsely flagged as ``invalid``. + **Setup for Other Repositories** To enable the auto-assignment bot in another OpenWISP repository, you must @@ -167,11 +224,11 @@ Create the following workflow files in your repository. name: PR Issue Auto-Assignment on: pull_request_target: - types: [opened, reopened, closed] + types: [opened, reopened, closed, edited, ready_for_review] permissions: contents: read issues: write - pull-requests: read + pull-requests: write concurrency: group: bot-autoassign-pr-link-${{ github.repository }}-${{ github.event.pull_request.number }}-${{ github.event.action }} cancel-in-progress: true diff --git a/setup.py b/setup.py index 295d42e29..d84149484 100644 --- a/setup.py +++ b/setup.py @@ -78,7 +78,7 @@ ], "github_actions": [ "google-genai>=1.62.0,<3.0.0", - "PyGithub>=2.0.0,<3.0.0", + "PyGithub>=2.5.0,<3.0.0", ], }, classifiers=[