From 6f5aaf22f4c2a1840d303a874fce573e0e4622e8 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 17 Mar 2026 19:36:43 +0200 Subject: [PATCH 1/7] fix: re-evaluate can-be-merged when review threads are resolved (#1038) Add pull_request_review_thread event handler so resolved/unresolved thread actions trigger can-be-merged re-evaluation via GraphQL API. --- webhook_server/libs/github_api.py | 29 ++++++++++- webhook_server/tests/test_github_api.py | 65 +++++++++++++++++++++++++ 2 files changed, 93 insertions(+), 1 deletion(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 7c5cc18e..48721d24 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -509,7 +509,7 @@ async def process(self) -> Any: # 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"): + 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": @@ -636,6 +636,33 @@ async def process(self) -> Any: await self._update_context_metrics() return None + elif self.github_event == "pull_request_review_thread": + action = self.hook_data["action"] + if action not in ("resolved", "unresolved"): + token_metrics = await self._get_token_metrics() + self.logger.info( + f"{self.log_prefix} " + f"Webhook processing completed successfully: pull_request_review_thread " + f"(action={action}, skipped) - {token_metrics}", + ) + await self._update_context_metrics() + return None + + self.logger.info(f"{self.log_prefix} Review thread {action}, 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) + + 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/test_github_api.py b/webhook_server/tests/test_github_api.py index ccfe97f8..d648a9d2 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -2205,3 +2205,68 @@ 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) + + webhook = GithubWebhook(review_thread_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() From fee8907c1255fb5bce710d17887970a4ef2cbcf3 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 17 Mar 2026 19:56:18 +0200 Subject: [PATCH 2/7] fix: hoist action filter, add clone + OwnersFileHandler.initialize - Early return for non-actionable thread events before PR fetch - Clone repo and initialize OwnersFileHandler for OWNERS data - Test asserts initialize() and clone called for resolved/unresolved --- webhook_server/libs/github_api.py | 29 +++++++++++++++---------- webhook_server/tests/test_github_api.py | 28 ++++++++++++++++++------ 2 files changed, 39 insertions(+), 18 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 48721d24..1789896f 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -462,6 +462,18 @@ async def process(self) -> Any: await self._update_context_metrics() return None + if self.github_event == "pull_request_review_thread": + action = self.hook_data["action"] + if action not in ("resolved", "unresolved"): + token_metrics = await self._get_token_metrics() + self.logger.info( + f"{self.log_prefix} " + f"Webhook processing completed successfully: pull_request_review_thread " + f"(action={action}, skipped) - {token_metrics}", + ) + await self._update_context_metrics() + return None + pull_request = await self.get_pull_request() if pull_request: # Update context with PR info @@ -508,7 +520,8 @@ 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 + # 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) @@ -637,20 +650,14 @@ async def process(self) -> Any: return None elif self.github_event == "pull_request_review_thread": + # Action already filtered above (only resolved/unresolved reach here) action = self.hook_data["action"] - if action not in ("resolved", "unresolved"): - token_metrics = await self._get_token_metrics() - self.logger.info( - f"{self.log_prefix} " - f"Webhook processing completed successfully: pull_request_review_thread " - f"(action={action}, skipped) - {token_metrics}", - ) - await self._update_context_metrics() - return None - self.logger.info(f"{self.log_prefix} Review thread {action}, re-evaluating can-be-merged") + 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) diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index d648a9d2..25cd0129 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -2263,10 +2263,24 @@ async def test_process_review_thread_event(self, action: str, should_recheck: bo 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(review_thread_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(review_thread_data, headers, logger) + await webhook.process() + + if should_recheck: + mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() + mock_owners_instance.initialize.assert_awaited_once() + mock_clone.assert_awaited_once() + else: + mock_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() + mock_owners_instance.initialize.assert_not_awaited() + mock_clone.assert_not_awaited() From d6feff339a32e565175ac8fd4651fbfd70fd818e Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 17 Mar 2026 20:18:18 +0200 Subject: [PATCH 3/7] fix: clone for status events, update webhook events, add comments - Add clone + OwnersFileHandler.initialize for status event handler - Update webhook if configured events differ from existing - Add inline comments explaining why clone is needed - Add pull_request_review_thread and status to example configs --- examples/.github-webhook-server.yaml | 6 +- examples/config.yaml | 4 +- webhook_server/libs/github_api.py | 9 ++ webhook_server/tests/manifests/config.yaml | 4 +- webhook_server/tests/test_github_api.py | 26 +++- webhook_server/tests/test_webhook.py | 152 ++++++++++++++++++++- webhook_server/utils/webhook.py | 16 +++ 7 files changed, 203 insertions(+), 14 deletions(-) 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 1789896f..868bac33 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -637,7 +637,13 @@ async def process(self) -> Any: f"re-evaluating can-be-merged" ) + # 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) @@ -654,6 +660,9 @@ async def process(self) -> Any: action = self.hook_data["action"] self.logger.info(f"{self.log_prefix} Review thread {action}, re-evaluating can-be-merged") + # 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) 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 25cd0129..81135142 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -876,13 +876,27 @@ 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() + with patch.object( + GithubWebhook, + "_clone_repository", + new=AsyncMock(return_value=None), + ) as mock_clone: + with patch.object( + OwnersFileHandler, + "initialize", + new=AsyncMock(return_value=None), + ) as mock_owners_init: + 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() + if should_recheck: + mock_clone.assert_awaited_once() + mock_owners_init.assert_awaited_once() + mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() + else: + mock_clone.assert_not_awaited() + mock_owners_init.assert_not_awaited() + 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: diff --git a/webhook_server/tests/test_webhook.py b/webhook_server/tests/test_webhook.py index 893a4d8d..c67eb404 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,152 @@ 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() + @patch("webhook_server.utils.webhook.get_github_repo_api") + def test_process_github_webhook_existing_hook_different_events( + self, + mock_get_repo_api: Mock, + sample_data: dict[str, Any], + apis_dict: dict[str, dict[str, Any]], + mock_repo: Mock, + ) -> None: + """Test webhook update when existing hook has different events than configured.""" + existing_hook = Mock() + existing_hook.config = {"url": "http://example.com", "content_type": "json"} + existing_hook.events = ["push"] # Missing "pull_request" from sample_data + + mock_repo.get_hooks.return_value = [existing_hook] + mock_get_repo_api.return_value = mock_repo + + success, message, _ = process_github_webhook( + repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", apis_dict=apis_dict + ) + + assert success is True + assert "Hook updated with new events" in message + assert "test-user" in message + + # Verify hook was edited with the new events + existing_hook.edit.assert_called_once_with( + name="web", + config={"url": "http://example.com", "content_type": "json"}, + events=["push", "pull_request"], + active=True, + ) + + # Verify no new hook was created and old hook was not deleted + mock_repo.create_hook.assert_not_called() + existing_hook.delete.assert_not_called() + + @patch("webhook_server.utils.webhook.get_github_repo_api") + def test_process_github_webhook_existing_hook_wildcard_vs_specific_events( + self, + mock_get_repo_api: Mock, + apis_dict: dict[str, dict[str, Any]], + mock_repo: Mock, + ) -> None: + """Test webhook update when hook has wildcard events but config has specific events.""" + existing_hook = Mock() + existing_hook.config = {"url": "http://example.com", "content_type": "json"} + existing_hook.events = ["*"] + + mock_repo.get_hooks.return_value = [existing_hook] + mock_get_repo_api.return_value = mock_repo + + data_with_specific_events: dict[str, Any] = {"name": "owner/test-repo", "events": ["push", "pull_request"]} + + success, message, _ = process_github_webhook( + repository_name="test-repo", + data=data_with_specific_events, + webhook_ip="http://example.com", + apis_dict=apis_dict, + ) + + assert success is True + assert "Hook updated with new events" in message + + # Verify hook was edited with the specific events + existing_hook.edit.assert_called_once_with( + name="web", + config={"url": "http://example.com", "content_type": "json"}, + events=["push", "pull_request"], + active=True, + ) + mock_repo.create_hook.assert_not_called() + + @patch("webhook_server.utils.webhook.get_github_repo_api") + def test_process_github_webhook_existing_hook_specific_vs_wildcard_events( + self, + mock_get_repo_api: Mock, + apis_dict: dict[str, dict[str, Any]], + mock_repo: Mock, + ) -> None: + """Test webhook update when hook has specific events but config has wildcard.""" + 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 + + data_with_wildcard: dict[str, Any] = {"name": "owner/test-repo", "events": ["*"]} + + success, message, _ = process_github_webhook( + repository_name="test-repo", + data=data_with_wildcard, + webhook_ip="http://example.com", + apis_dict=apis_dict, + ) + + assert success is True + assert "Hook updated with new events" in message + + # Verify hook was edited with the wildcard events + existing_hook.edit.assert_called_once_with( + name="web", + config={"url": "http://example.com", "content_type": "json"}, + events=["*"], + active=True, + ) + mock_repo.create_hook.assert_not_called() + + @patch("webhook_server.utils.webhook.get_github_repo_api") + def test_process_github_webhook_existing_hook_events_same_order_differs( + self, + mock_get_repo_api: Mock, + apis_dict: dict[str, dict[str, Any]], + mock_repo: Mock, + ) -> None: + """Test that events in different order are treated as equal (no update needed).""" + existing_hook = Mock() + existing_hook.config = {"url": "http://example.com", "content_type": "json"} + existing_hook.events = ["pull_request", "push"] # Reversed order + + mock_repo.get_hooks.return_value = [existing_hook] + mock_get_repo_api.return_value = mock_repo + + # sample_data has events ["push", "pull_request"] + data: dict[str, Any] = {"name": "owner/test-repo", "events": ["push", "pull_request"]} + + success, message, _ = process_github_webhook( + repository_name="test-repo", + data=data, + webhook_ip="http://example.com", + apis_dict=apis_dict, + ) + + assert success is True + assert "Hook already exists" in message + + # Verify no edit or create was called + existing_hook.edit.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..e1e8dcc7 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(_hook.events) + config_events = sorted(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=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']}", From 39014ad8599dd6ad44736cb689b0cad14b262999 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 17 Mar 2026 20:40:00 +0200 Subject: [PATCH 4/7] refactor: extract recheck helper, parameterize tests, dedupe events - Extract _recheck_merge_eligibility() shared by status and review_thread - Parameterize webhook event-update tests - Deduplicate events with set() before comparison - Assert skipped actions don't fetch PR - Add delete assertion in no-op webhook test --- webhook_server/libs/github_api.py | 36 +++--- webhook_server/tests/test_github_api.py | 25 ++-- webhook_server/tests/test_webhook.py | 145 +++++------------------- webhook_server/utils/webhook.py | 6 +- 4 files changed, 64 insertions(+), 148 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 868bac33..ebdd9ab7 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -409,6 +409,20 @@ 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: # Initialize auto-verified users from API users (async operation) await self.add_api_users_to_auto_verified_and_merged_users() @@ -637,16 +651,7 @@ async def process(self) -> Any: f"re-evaluating can-be-merged" ) - # 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) + await self._recheck_merge_eligibility(pull_request=pull_request) token_metrics = await self._get_token_metrics() self.logger.info( @@ -660,16 +665,7 @@ async def process(self) -> Any: action = self.hook_data["action"] self.logger.info(f"{self.log_prefix} Review thread {action}, re-evaluating can-be-merged") - # 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) + await self._recheck_merge_eligibility(pull_request=pull_request) token_metrics = await self._get_token_metrics() self.logger.info( diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 81135142..914f8309 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -2288,13 +2288,20 @@ async def test_process_review_thread_event(self, action: str, should_recheck: bo GithubWebhook, "_clone_repository", new_callable=AsyncMock ) as mock_clone: webhook = GithubWebhook(review_thread_data, headers, logger) - await webhook.process() - if should_recheck: - mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() - mock_owners_instance.initialize.assert_awaited_once() - mock_clone.assert_awaited_once() - else: - mock_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() - mock_owners_instance.initialize.assert_not_awaited() - mock_clone.assert_not_awaited() + 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_get_pr.assert_awaited() + mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() + mock_owners_instance.initialize.assert_awaited_once() + mock_clone.assert_awaited_once() + else: + mock_get_pr.assert_not_awaited() + mock_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() + mock_owners_instance.initialize.assert_not_awaited() + mock_clone.assert_not_awaited() diff --git a/webhook_server/tests/test_webhook.py b/webhook_server/tests/test_webhook.py index c67eb404..dd92e505 100644 --- a/webhook_server/tests/test_webhook.py +++ b/webhook_server/tests/test_webhook.py @@ -315,131 +315,34 @@ def test_process_github_webhook_multiple_existing_hooks( 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_different_events( - self, - mock_get_repo_api: Mock, - sample_data: dict[str, Any], - apis_dict: dict[str, dict[str, Any]], - mock_repo: Mock, - ) -> None: - """Test webhook update when existing hook has different events than configured.""" - existing_hook = Mock() - existing_hook.config = {"url": "http://example.com", "content_type": "json"} - existing_hook.events = ["push"] # Missing "pull_request" from sample_data - - mock_repo.get_hooks.return_value = [existing_hook] - mock_get_repo_api.return_value = mock_repo - - success, message, _ = process_github_webhook( - repository_name="test-repo", data=sample_data, webhook_ip="http://example.com", apis_dict=apis_dict - ) - - assert success is True - assert "Hook updated with new events" in message - assert "test-user" in message - - # Verify hook was edited with the new events - existing_hook.edit.assert_called_once_with( - name="web", - config={"url": "http://example.com", "content_type": "json"}, - events=["push", "pull_request"], - active=True, - ) - - # Verify no new hook was created and old hook was not deleted - mock_repo.create_hook.assert_not_called() - existing_hook.delete.assert_not_called() - - @patch("webhook_server.utils.webhook.get_github_repo_api") - def test_process_github_webhook_existing_hook_wildcard_vs_specific_events( - self, - mock_get_repo_api: Mock, - apis_dict: dict[str, dict[str, Any]], - mock_repo: Mock, - ) -> None: - """Test webhook update when hook has wildcard events but config has specific events.""" - existing_hook = Mock() - existing_hook.config = {"url": "http://example.com", "content_type": "json"} - existing_hook.events = ["*"] - - mock_repo.get_hooks.return_value = [existing_hook] - mock_get_repo_api.return_value = mock_repo - - data_with_specific_events: dict[str, Any] = {"name": "owner/test-repo", "events": ["push", "pull_request"]} - - success, message, _ = process_github_webhook( - repository_name="test-repo", - data=data_with_specific_events, - webhook_ip="http://example.com", - apis_dict=apis_dict, - ) - - assert success is True - assert "Hook updated with new events" in message - - # Verify hook was edited with the specific events - existing_hook.edit.assert_called_once_with( - name="web", - config={"url": "http://example.com", "content_type": "json"}, - events=["push", "pull_request"], - active=True, - ) - mock_repo.create_hook.assert_not_called() - - @patch("webhook_server.utils.webhook.get_github_repo_api") - def test_process_github_webhook_existing_hook_specific_vs_wildcard_events( - self, - mock_get_repo_api: Mock, - apis_dict: dict[str, dict[str, Any]], - mock_repo: Mock, - ) -> None: - """Test webhook update when hook has specific events but config has wildcard.""" - 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 - - data_with_wildcard: dict[str, Any] = {"name": "owner/test-repo", "events": ["*"]} - - success, message, _ = process_github_webhook( - repository_name="test-repo", - data=data_with_wildcard, - webhook_ip="http://example.com", - apis_dict=apis_dict, - ) - - assert success is True - assert "Hook updated with new events" in message - - # Verify hook was edited with the wildcard events - existing_hook.edit.assert_called_once_with( - name="web", - config={"url": "http://example.com", "content_type": "json"}, - events=["*"], - active=True, - ) - mock_repo.create_hook.assert_not_called() - - @patch("webhook_server.utils.webhook.get_github_repo_api") - def test_process_github_webhook_existing_hook_events_same_order_differs( + 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 that events in different order are treated as equal (no update needed).""" + """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 = ["pull_request", "push"] # Reversed order + existing_hook.events = hook_events mock_repo.get_hooks.return_value = [existing_hook] mock_get_repo_api.return_value = mock_repo - # sample_data has events ["push", "pull_request"] - data: dict[str, Any] = {"name": "owner/test-repo", "events": ["push", "pull_request"]} + data: dict[str, Any] = {"name": "owner/test-repo", "events": config_events} success, message, _ = process_github_webhook( repository_name="test-repo", @@ -449,10 +352,20 @@ def test_process_github_webhook_existing_hook_events_same_order_differs( ) assert success is True - assert "Hook already exists" in message - # Verify no edit or create was called - existing_hook.edit.assert_not_called() + 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() diff --git a/webhook_server/utils/webhook.py b/webhook_server/utils/webhook.py index e1e8dcc7..bf09cb38 100644 --- a/webhook_server/utils/webhook.py +++ b/webhook_server/utils/webhook.py @@ -57,14 +57,14 @@ def process_github_webhook( else: # Check if events need updating - hook_events = sorted(_hook.events) - config_events = sorted(events) + 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=events, active=True) + _hook.edit(name="web", config=config_, events=config_events, active=True) return ( True, f"[API user {api_user}] - {full_repository_name}: " From 414f268e79f4119c752ba0fd9d97284885db4fd3 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 17 Mar 2026 21:14:52 +0200 Subject: [PATCH 5/7] fix: hoist status and review_thread guards before bootstrap - Move pull_request_review_thread skip before add_api_users bootstrap - Move status pending skip before add_api_users bootstrap - Skip review_thread when required_conversation_resolution is disabled - Add tests for skipped bootstrap calls --- webhook_server/libs/github_api.py | 56 +++++++++------ webhook_server/tests/test_github_api.py | 93 +++++++++++++++++++++---- 2 files changed, 111 insertions(+), 38 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index ebdd9ab7..b06f1cd8 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -424,6 +424,38 @@ async def _recheck_merge_eligibility(self, pull_request: PullRequest) -> None: ) 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: + 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", + ) + 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": + self.logger.info( + f"{self.log_prefix} " + f"Webhook processing completed successfully: status " + f"(state=pending, skipped) - no metrics collected", + ) + return None + # Initialize auto-verified users from API users (async operation) await self.add_api_users_to_auto_verified_and_merged_users() @@ -476,18 +508,6 @@ async def process(self) -> Any: await self._update_context_metrics() return None - if self.github_event == "pull_request_review_thread": - action = self.hook_data["action"] - if action not in ("resolved", "unresolved"): - token_metrics = await self._get_token_metrics() - self.logger.info( - f"{self.log_prefix} " - f"Webhook processing completed successfully: pull_request_review_thread " - f"(action={action}, skipped) - {token_metrics}", - ) - await self._update_context_metrics() - return None - pull_request = await self.get_pull_request() if pull_request: # Update context with PR info @@ -633,18 +653,8 @@ 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}), " diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 914f8309..a5dc63d0 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -2290,18 +2290,81 @@ async def test_process_review_thread_event(self, action: str, should_recheck: bo webhook = GithubWebhook(review_thread_data, headers, logger) 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_get_pr.assert_awaited() - mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() - mock_owners_instance.initialize.assert_awaited_once() - mock_clone.assert_awaited_once() - else: - mock_get_pr.assert_not_awaited() - mock_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() - mock_owners_instance.initialize.assert_not_awaited() - mock_clone.assert_not_awaited() + 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_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() + mock_owners_instance.initialize.assert_awaited_once() + mock_clone.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_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() + mock_owners_instance.initialize.assert_not_awaited() + mock_clone.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() From 94438f2d837d9075ad2a70c5c9d3bdcfdd8a6112 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 17 Mar 2026 21:36:11 +0200 Subject: [PATCH 6/7] fix: add context tracking to early skips, assert bootstrap skipped - Track webhook_routing step and metrics for early-exit branches - Assert add_api_users and get_pull_request not called on pending skip --- webhook_server/libs/github_api.py | 6 +++++ webhook_server/tests/test_github_api.py | 36 ++++++++++++++++++------- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index b06f1cd8..2aa76044 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -430,6 +430,8 @@ async def process(self) -> Any: 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") @@ -440,6 +442,7 @@ async def process(self) -> Any: 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 @@ -449,11 +452,14 @@ async def process(self) -> Any: 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) diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index a5dc63d0..591ede42 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -887,16 +887,32 @@ async def test_process_status_event(self, state: str, should_recheck: bool) -> N new=AsyncMock(return_value=None), ) as mock_owners_init: webhook = GithubWebhook(status_data, headers, logger) - await webhook.process() - - if should_recheck: - mock_clone.assert_awaited_once() - mock_owners_init.assert_awaited_once() - mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() - else: - mock_clone.assert_not_awaited() - mock_owners_init.assert_not_awaited() - mock_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() + + 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_init.assert_awaited_once() + 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_init.assert_not_awaited() + 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: From 349e26b12815899cc5be048813e9bffad1cbbb71 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 17 Mar 2026 22:00:39 +0200 Subject: [PATCH 7/7] test: verify PullRequestHandler gets correct owners_file_handler --- webhook_server/tests/test_github_api.py | 40 +++++++++++++++---------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 591ede42..0c691c08 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -876,16 +876,16 @@ 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) - with patch.object( - GithubWebhook, - "_clone_repository", - new=AsyncMock(return_value=None), - ) as mock_clone: + 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( - OwnersFileHandler, - "initialize", - new=AsyncMock(return_value=None), - ) as mock_owners_init: + GithubWebhook, "_clone_repository", new_callable=AsyncMock + ) as mock_clone: webhook = GithubWebhook(status_data, headers, logger) with patch.object( @@ -905,13 +905,18 @@ async def test_process_status_event(self, state: str, should_recheck: bool) -> N mock_add_api_users.assert_awaited_once() mock_get_pr.assert_awaited() mock_clone.assert_awaited_once() - mock_owners_init.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_init.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 @@ -2319,16 +2324,21 @@ async def test_process_review_thread_event(self, action: str, should_recheck: bo if should_recheck: mock_add_api_users.assert_awaited_once() mock_get_pr.assert_awaited() - mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() - mock_owners_instance.initialize.assert_awaited_once() 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_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() - mock_owners_instance.initialize.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(