diff --git a/examples/.github-webhook-server.yaml b/examples/.github-webhook-server.yaml index 43b2519f..2da78707 100644 --- a/examples/.github-webhook-server.yaml +++ b/examples/.github-webhook-server.yaml @@ -21,10 +21,12 @@ pypi: events: - push - pull_request - - issue_comment - - check_run - pull_request_review - pull_request_review_comment + - pull_request_review_thread + - issue_comment + - check_run + - status # Tox configuration tox: diff --git a/examples/config.yaml b/examples/config.yaml index 26c6ce45..4be978e6 100644 --- a/examples/config.yaml +++ b/examples/config.yaml @@ -150,9 +150,11 @@ repositories: events: # To listen to all events do not send events - push - pull_request + - pull_request_review + - pull_request_review_thread - issue_comment - check_run - - pull_request_review + - status tox: main: all # Run all tests in tox.ini when pull request parent branch is main dev: testenv1,testenv2 # Run testenv1 and testenv2 tests in tox.ini when pull request parent branch is dev diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 7c5cc18e..2aa76044 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -409,7 +409,59 @@ def redact_output(value: str) -> str: self.ctx.fail_step("repo_clone", ex, traceback.format_exc()) raise RuntimeError(f"Repository clone failed: {ex}") from ex + async def _recheck_merge_eligibility(self, pull_request: PullRequest) -> None: + """Clone repo and re-evaluate can-be-merged for the PR. + + Clone required: check_if_can_be_merged evaluates ALL conditions + (approvals, OWNERS, labels, checks, conversations) on every call, + and OWNERS file processing requires a local checkout. + """ + await self._clone_repository(pull_request=pull_request) + owners_file_handler = OwnersFileHandler(github_webhook=self) + owners_file_handler = await owners_file_handler.initialize(pull_request=pull_request) + await PullRequestHandler(github_webhook=self, owners_file_handler=owners_file_handler).check_if_can_be_merged( + pull_request=pull_request + ) + async def process(self) -> Any: + # Early exit for pull_request_review_thread events that don't need processing. + # Must run BEFORE add_api_users_to_auto_verified_and_merged_users() to avoid + # burning rate limit on get_user() calls for skipped events. + if self.github_event == "pull_request_review_thread": + action = self.hook_data["action"] + if action not in ("resolved", "unresolved") or not self.required_conversation_resolution: + if self.ctx: + self.ctx.start_step("webhook_routing", event_type=self.github_event) + skip_reason = ( + f"action={action}, skipped" + if action not in ("resolved", "unresolved") + else "required_conversation_resolution disabled" + ) + self.logger.info( + f"{self.log_prefix} " + f"Webhook processing completed successfully: pull_request_review_thread " + f"({skip_reason}) - no metrics collected", + ) + await self._update_context_metrics() + return None + + # Early exit for status events with pending state — only terminal states + # (success, failure, error) need processing. + # Must run BEFORE add_api_users_to_auto_verified_and_merged_users() to avoid + # burning rate limit on get_user() calls for skipped events. + if self.github_event == "status": + state = self.hook_data["state"] + if state == "pending": + if self.ctx: + self.ctx.start_step("webhook_routing", event_type=self.github_event) + self.logger.info( + f"{self.log_prefix} " + f"Webhook processing completed successfully: status " + f"(state=pending, skipped) - no metrics collected", + ) + await self._update_context_metrics() + return None + # Initialize auto-verified users from API users (async operation) await self.add_api_users_to_auto_verified_and_merged_users() @@ -508,8 +560,9 @@ async def process(self) -> Any: self.last_committer = getattr(self.last_commit.committer, "login", self.parent_committer) # Clone repository for local file processing (OWNERS, changed files) - # For check_run and status events, cloning happens later only when needed - if self.github_event not in ("check_run", "status"): + # For check_run, status, and pull_request_review_thread events, + # cloning happens later only when needed (inside their respective handlers) + if self.github_event not in ("check_run", "status", "pull_request_review_thread"): await self._clone_repository(pull_request=pull_request) if self.github_event == "issue_comment": @@ -606,28 +659,15 @@ async def process(self) -> Any: return None elif self.github_event == "status": - # Skip pending state — only terminal states (success, failure, error) trigger re-evaluation + # Pending state already filtered by early exit above — only terminal states reach here state = self.hook_data["state"] - if state == "pending": - token_metrics = await self._get_token_metrics() - self.logger.info( - f"{self.log_prefix} " - f"Webhook processing completed successfully: status (state=pending, skipped) - " - f"{token_metrics}", - ) - await self._update_context_metrics() - return None - context_name = self.hook_data["context"] self.logger.info( f"{self.log_prefix} Status check '{context_name}' reached terminal state ({state}), " f"re-evaluating can-be-merged" ) - owners_file_handler = OwnersFileHandler(github_webhook=self) - await PullRequestHandler( - github_webhook=self, owners_file_handler=owners_file_handler - ).check_if_can_be_merged(pull_request=pull_request) + await self._recheck_merge_eligibility(pull_request=pull_request) token_metrics = await self._get_token_metrics() self.logger.info( @@ -636,6 +676,21 @@ async def process(self) -> Any: await self._update_context_metrics() return None + elif self.github_event == "pull_request_review_thread": + # Action already filtered above (only resolved/unresolved reach here) + action = self.hook_data["action"] + self.logger.info(f"{self.log_prefix} Review thread {action}, re-evaluating can-be-merged") + + await self._recheck_merge_eligibility(pull_request=pull_request) + + token_metrics = await self._get_token_metrics() + self.logger.info( + f"{self.log_prefix} Webhook processing completed successfully: " + f"pull_request_review_thread - {token_metrics}", + ) + await self._update_context_metrics() + return None + else: # Log warning when no PR found self.logger.warning( diff --git a/webhook_server/tests/manifests/config.yaml b/webhook_server/tests/manifests/config.yaml index 88b5fb4c..f89d0c35 100644 --- a/webhook_server/tests/manifests/config.yaml +++ b/webhook_server/tests/manifests/config.yaml @@ -38,9 +38,11 @@ repositories: events: # To listen to all events do not send events - push - pull_request + - pull_request_review + - pull_request_review_thread - issue_comment - check_run - - pull_request_review + - status tox-python-version: "3.8" tox: main: all # Run all tests in tox.ini when pull request parent branch is main diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index ccfe97f8..0c691c08 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -876,13 +876,48 @@ async def test_process_status_event(self, state: str, should_recheck: bool) -> N with patch("webhook_server.libs.github_api.PullRequestHandler") as mock_pr_handler: mock_pr_handler.return_value.check_if_can_be_merged = AsyncMock(return_value=None) - webhook = GithubWebhook(status_data, headers, logger) - await webhook.process() - - if should_recheck: - mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() - else: - mock_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() + with patch( + "webhook_server.libs.github_api.OwnersFileHandler" + ) as mock_owners_handler: + mock_owners_instance = Mock() + mock_owners_instance.initialize = AsyncMock(return_value=mock_owners_instance) + mock_owners_handler.return_value = mock_owners_instance + + with patch.object( + GithubWebhook, "_clone_repository", new_callable=AsyncMock + ) as mock_clone: + webhook = GithubWebhook(status_data, headers, logger) + + with patch.object( + webhook, + "add_api_users_to_auto_verified_and_merged_users", + new_callable=AsyncMock, + ) as mock_add_api_users: + with patch.object( + webhook, + "get_pull_request", + new_callable=AsyncMock, + ) as mock_get_pr: + mock_get_pr.return_value = mock_pr + await webhook.process() + + if should_recheck: + mock_add_api_users.assert_awaited_once() + mock_get_pr.assert_awaited() + mock_clone.assert_awaited_once() + mock_owners_instance.initialize.assert_awaited_once() + mock_pr_handler.assert_called_once_with( + github_webhook=webhook, + owners_file_handler=mock_owners_instance, + ) + mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() + else: + mock_add_api_users.assert_not_awaited() + mock_get_pr.assert_not_awaited() + mock_clone.assert_not_awaited() + mock_owners_instance.initialize.assert_not_awaited() + mock_pr_handler.assert_not_called() + mock_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() @pytest.mark.asyncio async def test_get_pull_request_from_status_sha(self) -> None: @@ -2205,3 +2240,157 @@ def get_value_side_effect( call.args[0] for call in mock_logger.warning.call_args_list if call.args ] assert any("Duplicate custom check name 'my-check'" in msg for msg in warning_messages) + + @pytest.mark.asyncio + @pytest.mark.parametrize( + ("action", "should_recheck"), + [ + ("resolved", True), + ("unresolved", True), + ("created", False), + ], + ) + async def test_process_review_thread_event(self, action: str, should_recheck: bool) -> None: + """Test processing pull_request_review_thread event with various actions.""" + logger = Mock() + review_thread_data = { + "action": action, + "pull_request": {"number": 42}, + "repository": {"name": "test-repo", "full_name": "org/test-repo"}, + } + headers = Headers({"X-GitHub-Event": "pull_request_review_thread", "X-GitHub-Delivery": "abc"}) + + with tempfile.TemporaryDirectory() as temp_dir: + with patch("webhook_server.libs.github_api.Config") as mock_config: + mock_config.return_value.repository = True + mock_config.return_value.repository_local_data.return_value = {} + mock_config.return_value.data_dir = temp_dir + + with patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit") as mock_get_api: + mock_get_api.return_value = (Mock(), "token", "apiuser") + + mock_repo = Mock() + mock_repo.get_git_tree.return_value.tree = [] + mock_pr = Mock() + mock_pr.head.sha = "abc123" + mock_pr.title = "Test PR" + mock_pr.number = 42 + mock_pr.draft = False + mock_pr.user.login = "testuser" + mock_pr.base.ref = "main" + mock_pr.get_commits.return_value = [Mock()] + mock_pr.get_files.return_value = [] + mock_repo.get_pull.return_value = mock_pr + with patch("webhook_server.libs.github_api.get_github_repo_api") as mock_get_repo_api: + mock_get_repo_api.return_value = mock_repo + + with patch("webhook_server.libs.github_api.get_repository_github_app_api") as mock_get_app_api: + mock_get_app_api.return_value = Mock() + + with patch( + "webhook_server.libs.github_api.get_apis_and_tokes_from_config" + ) as mock_get_apis: + mock_api1 = Mock() + mock_api1.rate_limiting = [0, 5000] + mock_api1.get_user.return_value.login = "user1" + mock_get_apis.return_value = [(mock_api1, "token1")] + + with patch("webhook_server.libs.github_api.PullRequestHandler") as mock_pr_handler: + mock_pr_handler.return_value.check_if_can_be_merged = AsyncMock(return_value=None) + + with patch( + "webhook_server.libs.github_api.OwnersFileHandler" + ) as mock_owners_handler: + mock_owners_instance = Mock() + mock_owners_instance.initialize = AsyncMock(return_value=mock_owners_instance) + mock_owners_handler.return_value = mock_owners_instance + + with patch.object( + GithubWebhook, "_clone_repository", new_callable=AsyncMock + ) as mock_clone: + webhook = GithubWebhook(review_thread_data, headers, logger) + + with patch.object( + webhook, + "add_api_users_to_auto_verified_and_merged_users", + new_callable=AsyncMock, + ) as mock_add_api_users: + with patch.object( + webhook, "get_pull_request", new_callable=AsyncMock + ) as mock_get_pr: + mock_get_pr.return_value = mock_pr + await webhook.process() + + if should_recheck: + mock_add_api_users.assert_awaited_once() + mock_get_pr.assert_awaited() + mock_clone.assert_awaited_once() + mock_owners_instance.initialize.assert_awaited_once() + mock_pr_handler.assert_called_once_with( + github_webhook=webhook, + owners_file_handler=mock_owners_instance, + ) + mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() + else: + # Early exit before add_api_users saves rate limit + mock_add_api_users.assert_not_awaited() + mock_get_pr.assert_not_awaited() + mock_clone.assert_not_awaited() + mock_owners_instance.initialize.assert_not_awaited() + mock_pr_handler.assert_not_called() + mock_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() + + @pytest.mark.asyncio + @pytest.mark.parametrize( + "action", + ["resolved", "unresolved"], + ) + async def test_process_review_thread_skips_when_conversation_resolution_disabled(self, action: str) -> None: + """Test review thread events skipped when required_conversation_resolution is disabled.""" + logger = Mock() + review_thread_data = { + "action": action, + "pull_request": {"number": 42}, + "repository": {"name": "test-repo", "full_name": "org/test-repo"}, + } + headers = Headers({"X-GitHub-Event": "pull_request_review_thread", "X-GitHub-Delivery": "abc"}) + + with tempfile.TemporaryDirectory() as temp_dir: + with patch("webhook_server.libs.github_api.Config") as mock_config: + mock_config.return_value.repository = True + mock_config.return_value.repository_local_data.return_value = {} + mock_config.return_value.data_dir = temp_dir + + with patch("webhook_server.libs.github_api.get_api_with_highest_rate_limit") as mock_get_api: + mock_get_api.return_value = (Mock(), "token", "apiuser") + + mock_repo = Mock() + mock_repo.get_git_tree.return_value.tree = [] + with patch("webhook_server.libs.github_api.get_github_repo_api") as mock_get_repo_api: + mock_get_repo_api.return_value = mock_repo + + with patch("webhook_server.libs.github_api.get_repository_github_app_api") as mock_get_app_api: + mock_get_app_api.return_value = Mock() + + with patch( + "webhook_server.libs.github_api.get_apis_and_tokes_from_config" + ) as mock_get_apis: + mock_get_apis.return_value = [] + + webhook = GithubWebhook(review_thread_data, headers, logger) + # Override required_conversation_resolution to False + webhook.required_conversation_resolution = False + + with patch.object( + webhook, + "add_api_users_to_auto_verified_and_merged_users", + new_callable=AsyncMock, + ) as mock_add_api_users: + with patch.object( + webhook, "get_pull_request", new_callable=AsyncMock + ) as mock_get_pr: + await webhook.process() + + # Early exit: no rate limit burned, no PR fetched + mock_add_api_users.assert_not_awaited() + mock_get_pr.assert_not_awaited() diff --git a/webhook_server/tests/test_webhook.py b/webhook_server/tests/test_webhook.py index 893a4d8d..dd92e505 100644 --- a/webhook_server/tests/test_webhook.py +++ b/webhook_server/tests/test_webhook.py @@ -122,10 +122,11 @@ def test_process_github_webhook_existing_hook_same_config( apis_dict: dict[str, dict[str, Any]], mock_repo: Mock, ) -> None: - """Test when webhook already exists with same configuration.""" - # Mock existing hook with matching URL + """Test when webhook already exists with same configuration and events.""" + # Mock existing hook with matching URL and same events existing_hook = Mock() existing_hook.config = {"url": "http://example.com", "content_type": "json"} + existing_hook.events = ["push", "pull_request"] mock_repo.get_hooks.return_value = [existing_hook] mock_get_repo_api.return_value = mock_repo @@ -137,9 +138,10 @@ def test_process_github_webhook_existing_hook_same_config( assert "Hook already exists" in message assert "test-user" in message - # Verify no new hook was created + # Verify no new hook was created and no edit was called mock_repo.create_hook.assert_not_called() existing_hook.delete.assert_not_called() + existing_hook.edit.assert_not_called() @patch("webhook_server.utils.webhook.get_github_repo_api") def test_process_github_webhook_secret_mismatch_deletes_old_hook( @@ -292,6 +294,7 @@ def test_process_github_webhook_multiple_existing_hooks( # Mock multiple existing hooks matching_hook = Mock() matching_hook.config = {"url": "http://example.com", "content_type": "json"} + matching_hook.events = ["push", "pull_request"] non_matching_hook = Mock() non_matching_hook.config = {"url": "http://different.com", "content_type": "json"} @@ -306,11 +309,65 @@ def test_process_github_webhook_multiple_existing_hooks( assert success is True assert "Hook already exists" in message - # Verify no hooks were deleted or created + # Verify no hooks were deleted, edited, or created matching_hook.delete.assert_not_called() + matching_hook.edit.assert_not_called() non_matching_hook.delete.assert_not_called() mock_repo.create_hook.assert_not_called() + @pytest.mark.parametrize( + ("hook_events", "config_events", "expect_update"), + [ + pytest.param(["push"], ["push", "pull_request"], True, id="missing-event"), + pytest.param(["*"], ["push", "pull_request"], True, id="wildcard-to-specific"), + pytest.param(["push", "pull_request"], ["*"], True, id="specific-to-wildcard"), + pytest.param(["pull_request", "push"], ["push", "pull_request"], False, id="same-events-different-order"), + ], + ) + @patch("webhook_server.utils.webhook.get_github_repo_api") + def test_process_github_webhook_existing_hook_event_update( + self, + mock_get_repo_api: Mock, + apis_dict: dict[str, dict[str, Any]], + mock_repo: Mock, + hook_events: list[str], + config_events: list[str], + expect_update: bool, + ) -> None: + """Test webhook event update/no-op for various existing vs configured event combinations.""" + existing_hook = Mock() + existing_hook.config = {"url": "http://example.com", "content_type": "json"} + existing_hook.events = hook_events + + mock_repo.get_hooks.return_value = [existing_hook] + mock_get_repo_api.return_value = mock_repo + + data: dict[str, Any] = {"name": "owner/test-repo", "events": config_events} + + success, message, _ = process_github_webhook( + repository_name="test-repo", + data=data, + webhook_ip="http://example.com", + apis_dict=apis_dict, + ) + + assert success is True + + if expect_update: + assert "Hook updated with new events" in message + existing_hook.edit.assert_called_once_with( + name="web", + config={"url": "http://example.com", "content_type": "json"}, + events=sorted(set(config_events)), + active=True, + ) + else: + assert "Hook already exists" in message + existing_hook.edit.assert_not_called() + existing_hook.delete.assert_not_called() + + mock_repo.create_hook.assert_not_called() + class TestCreateWebhook: """Test suite for create_webhook function.""" diff --git a/webhook_server/utils/webhook.py b/webhook_server/utils/webhook.py index a9503024..bf09cb38 100644 --- a/webhook_server/utils/webhook.py +++ b/webhook_server/utils/webhook.py @@ -56,6 +56,22 @@ def process_github_webhook( _hook.delete() else: + # Check if events need updating + hook_events = sorted(set(_hook.events)) + config_events = sorted(set(events)) + if hook_events != config_events: + LOGGER.info( + f"[API user {api_user}] - {full_repository_name}: " + f"Updating webhook events: {hook_events} -> {config_events}" + ) + _hook.edit(name="web", config=config_, events=config_events, active=True) + return ( + True, + f"[API user {api_user}] - {full_repository_name}: " + f"Hook updated with new events - {_hook.config['url']}", + LOGGER.info, + ) + return ( True, f"[API user {api_user}] - {full_repository_name}: Hook already exists - {_hook.config['url']}",