From 383e41cbcde7c3fbae37865d5291ec7096770d0b Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 17 Mar 2026 15:39:14 +0200 Subject: [PATCH 1/5] fix: handle status events to trigger can-be-merged re-evaluation (#1034) External services like pre-commit.ci report via GitHub's Status API. When a status check succeeds, re-evaluate can-be-merged for the PR. --- webhook_server/libs/github_api.py | 51 +++++- webhook_server/tests/test_github_api.py | 219 ++++++++++++++++++++++++ 2 files changed, 268 insertions(+), 2 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 734d1953..85e5d542 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -508,8 +508,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 events, cloning happens later only when needed - if self.github_event != "check_run": + # For check_run and status events, cloning happens later only when needed + if self.github_event not in ("check_run", "status"): await self._clone_repository(pull_request=pull_request) if self.github_event == "issue_comment": @@ -605,6 +605,39 @@ async def process(self) -> Any: await self._update_context_metrics() return None + elif self.github_event == "status": + # Only re-evaluate can-be-merged when a status check succeeds + state = self.hook_data.get("state", "") + if state != "success": + token_metrics = await self._get_token_metrics() + self.logger.info( + f"{self.log_prefix} " + f"Webhook processing completed successfully: status (state={state}, skipped) - " + f"{token_metrics}", + ) + await self._update_context_metrics() + return None + + context_name = self.hook_data.get("context", "") + self.logger.info( + f"{self.log_prefix} Status check '{context_name}' succeeded, 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) + + token_metrics = await self._get_token_metrics() + self.logger.info( + f"{self.log_prefix} Webhook processing completed successfully: status - {token_metrics}", + ) + await self._update_context_metrics() + return None + else: # Log warning when no PR found self.logger.warning( @@ -882,6 +915,20 @@ async def get_pull_request(self, number: int | None = None) -> PullRequest | Non return _pull_request self.logger.debug(f"{self.log_prefix} No open PR found matching check_run head SHA") + if self.github_event == "status": + sha = self.hook_data.get("sha", "") + if sha: + self.logger.debug(f"{self.log_prefix} Searching open PRs for status SHA: {sha}") + for _pull_request in await asyncio.to_thread(self.repository.get_pulls, state="open"): + if _pull_request.head.sha == sha: + self.logger.debug( + f"{self.log_prefix} Found pull request {_pull_request.title} " + f"[{_pull_request.number}] for status context " + f"{self.hook_data.get('context', 'unknown')}" + ) + return _pull_request + self.logger.debug(f"{self.log_prefix} No open PR found matching status SHA") + self.logger.debug(f"{self.log_prefix} All PR lookup strategies exhausted, no PR found") return None diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 3f8655a2..47da079a 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -816,6 +816,225 @@ async def test_process_check_run_event(self) -> None: mock_check_handler.return_value.process_pull_request_check_run_webhook_data.assert_awaited_once() mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() + @pytest.mark.asyncio + async def test_process_status_event_success(self) -> None: + """Test processing status event with state=success triggers can-be-merged re-evaluation.""" + logger = Mock() + status_data = { + "state": "success", + "context": "pre-commit.ci", + "sha": "abc123", + "repository": {"name": "test-repo", "full_name": "org/test-repo"}, + } + headers = Headers({"X-GitHub-Event": "status", "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 repository and get_pulls to return a PR with matching head.sha + 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_pulls.return_value = [mock_pr] + 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(status_data, headers, logger) + with ( + patch.object(webhook, "_clone_repository", new=AsyncMock(return_value=None)), + patch.object( + OwnersFileHandler, + "initialize", + new=AsyncMock(return_value=None), + ), + ): + await webhook.process() + + mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() + + @pytest.mark.asyncio + async def test_process_status_event_pending_skipped(self) -> None: + """Test processing status event with state=pending is skipped.""" + logger = Mock() + status_data = { + "state": "pending", + "context": "pre-commit.ci", + "sha": "abc123", + "repository": {"name": "test-repo", "full_name": "org/test-repo"}, + } + headers = Headers({"X-GitHub-Event": "status", "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_pulls.return_value = [mock_pr] + 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(status_data, headers, logger) + await webhook.process() + + # check_if_can_be_merged should NOT be called for pending state + mock_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() + + @pytest.mark.asyncio + async def test_process_status_event_failure_skipped(self) -> None: + """Test processing status event with state=failure is skipped.""" + logger = Mock() + status_data = { + "state": "failure", + "context": "pre-commit.ci", + "sha": "abc123", + "repository": {"name": "test-repo", "full_name": "org/test-repo"}, + } + headers = Headers({"X-GitHub-Event": "status", "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_pulls.return_value = [mock_pr] + 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(status_data, headers, logger) + await webhook.process() + + # check_if_can_be_merged should NOT be called for failure state + 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, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock + ) -> None: + """Test getting pull request from status event SHA.""" + status_data = { + "state": "success", + "context": "pre-commit.ci", + "sha": "status-sha-123", + "repository": {"name": "test-repo", "full_name": "org/test-repo"}, + } + headers = Headers({"X-GitHub-Event": "status", "X-GitHub-Delivery": "abc"}) + + 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 = {} + + 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") + + with patch("webhook_server.libs.github_api.get_github_repo_api") as mock_get_repo_api: + mock_repo = Mock() + mock_pr = Mock() + mock_pr.head.sha = "status-sha-123" + mock_pr.title = "Test PR" + mock_pr.number = 42 + mock_repo.get_pulls.return_value = [mock_pr] + 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.utils.helpers.get_repository_color_for_log_prefix") as mock_color: + mock_color.return_value = "test-repo" + + gh = GithubWebhook(status_data, headers, logger) + result = await gh.get_pull_request() + assert result == mock_pr + mock_repo.get_pulls.assert_called_once_with(state="open") + @pytest.mark.asyncio async def test_get_pull_request_by_number( self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock From 8c344bf993e3e5820d76231a5c8973bd06e51b8e Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 17 Mar 2026 15:57:04 +0200 Subject: [PATCH 2/5] fix: thread PaginatedList iteration, re-evaluate on failure/error - Wrap PaginatedList iteration and .head.sha access in asyncio.to_thread for both check_run and status SHA lookups - Re-evaluate can-be-merged on failure/error states (terminal), skip only pending (non-terminal) - Remove unused test fixture parameters --- webhook_server/libs/github_api.py | 22 ++++++++++++++-------- webhook_server/tests/test_github_api.py | 23 +++++++++++++++-------- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 85e5d542..54c554bf 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -606,13 +606,13 @@ async def process(self) -> Any: return None elif self.github_event == "status": - # Only re-evaluate can-be-merged when a status check succeeds + # Skip pending state — only terminal states (success, failure, error) trigger re-evaluation state = self.hook_data.get("state", "") - if state != "success": + if state == "pending": token_metrics = await self._get_token_metrics() self.logger.info( f"{self.log_prefix} " - f"Webhook processing completed successfully: status (state={state}, skipped) - " + f"Webhook processing completed successfully: status (state=pending, skipped) - " f"{token_metrics}", ) await self._update_context_metrics() @@ -620,7 +620,8 @@ async def process(self) -> Any: context_name = self.hook_data.get("context", "") self.logger.info( - f"{self.log_prefix} Status check '{context_name}' succeeded, re-evaluating can-be-merged" + f"{self.log_prefix} Status check '{context_name}' reached terminal state ({state}), " + f"re-evaluating can-be-merged" ) await self._clone_repository(pull_request=pull_request) @@ -902,11 +903,15 @@ async def get_pull_request(self, number: int | None = None) -> PullRequest | Non else: self.logger.debug(f"{self.log_prefix} No commit data in webhook payload") + def _get_pr_head_sha(pr: PullRequest) -> str: + return pr.head.sha + if self.github_event == "check_run": head_sha = self.hook_data["check_run"]["head_sha"] self.logger.debug(f"{self.log_prefix} Searching open PRs for check_run head SHA: {head_sha}") - for _pull_request in await asyncio.to_thread(self.repository.get_pulls, state="open"): - if _pull_request.head.sha == head_sha: + open_pulls = await asyncio.to_thread(lambda: list(self.repository.get_pulls(state="open"))) + for _pull_request in open_pulls: + if await asyncio.to_thread(_get_pr_head_sha, _pull_request) == head_sha: self.logger.debug( f"{self.log_prefix} Found pull request {_pull_request.title} " f"[{_pull_request.number}] for check run " @@ -919,8 +924,9 @@ async def get_pull_request(self, number: int | None = None) -> PullRequest | Non sha = self.hook_data.get("sha", "") if sha: self.logger.debug(f"{self.log_prefix} Searching open PRs for status SHA: {sha}") - for _pull_request in await asyncio.to_thread(self.repository.get_pulls, state="open"): - if _pull_request.head.sha == sha: + open_pulls = await asyncio.to_thread(lambda: list(self.repository.get_pulls(state="open"))) + for _pull_request in open_pulls: + if await asyncio.to_thread(_get_pr_head_sha, _pull_request) == sha: self.logger.debug( f"{self.log_prefix} Found pull request {_pull_request.title} " f"[{_pull_request.number}] for status context " diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 47da079a..6d9d313f 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -939,8 +939,8 @@ async def test_process_status_event_pending_skipped(self) -> None: mock_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() @pytest.mark.asyncio - async def test_process_status_event_failure_skipped(self) -> None: - """Test processing status event with state=failure is skipped.""" + async def test_process_status_event_failure_triggers_reevaluation(self) -> None: + """Test processing status event with state=failure triggers can-be-merged re-evaluation.""" logger = Mock() status_data = { "state": "failure", @@ -990,16 +990,23 @@ async def test_process_status_event_failure_skipped(self) -> None: 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(webhook, "_clone_repository", new=AsyncMock(return_value=None)), + patch.object( + OwnersFileHandler, + "initialize", + new=AsyncMock(return_value=None), + ), + ): + await webhook.process() - # check_if_can_be_merged should NOT be called for failure state - mock_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() + # check_if_can_be_merged SHOULD be called for failure state (terminal state) + mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() @pytest.mark.asyncio - async def test_get_pull_request_from_status_sha( - self, minimal_hook_data: dict, minimal_headers: Headers, logger: Mock - ) -> None: + async def test_get_pull_request_from_status_sha(self) -> None: """Test getting pull request from status event SHA.""" + logger = Mock() status_data = { "state": "success", "context": "pre-commit.ci", From 8f427921dcb3d646c844a86ecb8f7fef5b0e7de4 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 17 Mar 2026 16:16:13 +0200 Subject: [PATCH 3/5] fix: direct field access, remove clone, batch SHA lookups, error test - Use direct [] access for required webhook payload fields (fail-fast) - Remove unnecessary clone + OWNERS init from status handler - Batch SHA comparisons with asyncio.gather() for check_run and status - Add state="error" terminal-state test --- webhook_server/libs/github_api.py | 38 ++++++------ webhook_server/tests/test_github_api.py | 77 +++++++++++++++++++------ 2 files changed, 77 insertions(+), 38 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 54c554bf..4b235496 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -607,7 +607,7 @@ async def process(self) -> Any: elif self.github_event == "status": # Skip pending state — only terminal states (success, failure, error) trigger re-evaluation - state = self.hook_data.get("state", "") + state = self.hook_data["state"] if state == "pending": token_metrics = await self._get_token_metrics() self.logger.info( @@ -618,16 +618,13 @@ async def process(self) -> Any: await self._update_context_metrics() return None - context_name = self.hook_data.get("context", "") + 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" ) - 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) @@ -910,8 +907,9 @@ def _get_pr_head_sha(pr: PullRequest) -> str: head_sha = self.hook_data["check_run"]["head_sha"] self.logger.debug(f"{self.log_prefix} Searching open PRs for check_run head SHA: {head_sha}") open_pulls = await asyncio.to_thread(lambda: list(self.repository.get_pulls(state="open"))) - for _pull_request in open_pulls: - if await asyncio.to_thread(_get_pr_head_sha, _pull_request) == head_sha: + head_shas = await asyncio.gather(*(asyncio.to_thread(_get_pr_head_sha, pr) for pr in open_pulls)) + for _pull_request, pr_head_sha in zip(open_pulls, head_shas, strict=False): + if pr_head_sha == head_sha: self.logger.debug( f"{self.log_prefix} Found pull request {_pull_request.title} " f"[{_pull_request.number}] for check run " @@ -921,19 +919,19 @@ def _get_pr_head_sha(pr: PullRequest) -> str: self.logger.debug(f"{self.log_prefix} No open PR found matching check_run head SHA") if self.github_event == "status": - sha = self.hook_data.get("sha", "") - if sha: - self.logger.debug(f"{self.log_prefix} Searching open PRs for status SHA: {sha}") - open_pulls = await asyncio.to_thread(lambda: list(self.repository.get_pulls(state="open"))) - for _pull_request in open_pulls: - if await asyncio.to_thread(_get_pr_head_sha, _pull_request) == sha: - self.logger.debug( - f"{self.log_prefix} Found pull request {_pull_request.title} " - f"[{_pull_request.number}] for status context " - f"{self.hook_data.get('context', 'unknown')}" - ) - return _pull_request - self.logger.debug(f"{self.log_prefix} No open PR found matching status SHA") + sha = self.hook_data["sha"] + self.logger.debug(f"{self.log_prefix} Searching open PRs for status SHA: {sha}") + open_pulls = await asyncio.to_thread(lambda: list(self.repository.get_pulls(state="open"))) + head_shas = await asyncio.gather(*(asyncio.to_thread(_get_pr_head_sha, pr) for pr in open_pulls)) + for _pull_request, pr_head_sha in zip(open_pulls, head_shas, strict=False): + if pr_head_sha == sha: + self.logger.debug( + f"{self.log_prefix} Found pull request {_pull_request.title} " + f"[{_pull_request.number}] for status context " + f"{self.hook_data['context']}" + ) + return _pull_request + self.logger.debug(f"{self.log_prefix} No open PR found matching status SHA") self.logger.debug(f"{self.log_prefix} All PR lookup strategies exhausted, no PR found") return None diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 6d9d313f..7d1f9677 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -869,15 +869,7 @@ async def test_process_status_event_success(self) -> None: mock_pr_handler.return_value.check_if_can_be_merged = AsyncMock(return_value=None) webhook = GithubWebhook(status_data, headers, logger) - with ( - patch.object(webhook, "_clone_repository", new=AsyncMock(return_value=None)), - patch.object( - OwnersFileHandler, - "initialize", - new=AsyncMock(return_value=None), - ), - ): - await webhook.process() + await webhook.process() mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() @@ -990,19 +982,68 @@ async def test_process_status_event_failure_triggers_reevaluation(self) -> None: mock_pr_handler.return_value.check_if_can_be_merged = AsyncMock(return_value=None) webhook = GithubWebhook(status_data, headers, logger) - with ( - patch.object(webhook, "_clone_repository", new=AsyncMock(return_value=None)), - patch.object( - OwnersFileHandler, - "initialize", - new=AsyncMock(return_value=None), - ), - ): - await webhook.process() + await webhook.process() # check_if_can_be_merged SHOULD be called for failure state (terminal state) mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() + @pytest.mark.asyncio + async def test_process_status_event_error_triggers_reevaluation(self) -> None: + """Test processing status event with state=error triggers can-be-merged re-evaluation.""" + logger = Mock() + status_data = { + "state": "error", + "context": "pre-commit.ci", + "sha": "abc123", + "repository": {"name": "test-repo", "full_name": "org/test-repo"}, + } + headers = Headers({"X-GitHub-Event": "status", "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_pulls.return_value = [mock_pr] + 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(status_data, headers, logger) + await webhook.process() + + # check_if_can_be_merged SHOULD be called for error state (terminal state) + mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() + @pytest.mark.asyncio async def test_get_pull_request_from_status_sha(self) -> None: """Test getting pull request from status event SHA.""" From fac346db7fcb5265b0b2d0f963a7f02459367ccc Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 17 Mar 2026 16:34:06 +0200 Subject: [PATCH 4/5] fix: reorder PR lookup, parameterize tests, add commit payload - Move check_run/status SHA lookups before commit.get_pulls() fallback - Parameterize 4 status state tests into single test - Add commit object with stale PR to status SHA test --- webhook_server/libs/github_api.py | 26 +-- webhook_server/tests/test_github_api.py | 207 ++++-------------------- 2 files changed, 43 insertions(+), 190 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 4b235496..7c5cc18e 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -887,19 +887,6 @@ async def get_pull_request(self, number: int | None = None) -> PullRequest | Non else: self.logger.debug(f"{self.log_prefix} No PR data in webhook payload") - commit: dict[str, Any] = self.hook_data.get("commit", {}) - if commit: - self.logger.debug(f"{self.log_prefix} Attempting to get PR from commit SHA: {commit.get('sha', 'unknown')}") - commit_obj = await asyncio.to_thread(self.repository.get_commit, commit["sha"]) - with contextlib.suppress(Exception): - _pulls = await asyncio.to_thread(commit_obj.get_pulls) - if _pulls: - self.logger.debug(f"{self.log_prefix} Found PR from commit SHA: {_pulls[0].number}") - return _pulls[0] - self.logger.debug(f"{self.log_prefix} No PR found for commit SHA") - else: - self.logger.debug(f"{self.log_prefix} No commit data in webhook payload") - def _get_pr_head_sha(pr: PullRequest) -> str: return pr.head.sha @@ -933,6 +920,19 @@ def _get_pr_head_sha(pr: PullRequest) -> str: return _pull_request self.logger.debug(f"{self.log_prefix} No open PR found matching status SHA") + commit: dict[str, Any] = self.hook_data.get("commit", {}) + if commit: + self.logger.debug(f"{self.log_prefix} Attempting to get PR from commit SHA: {commit.get('sha', 'unknown')}") + commit_obj = await asyncio.to_thread(self.repository.get_commit, commit["sha"]) + with contextlib.suppress(Exception): + _pulls = await asyncio.to_thread(commit_obj.get_pulls) + if _pulls: + self.logger.debug(f"{self.log_prefix} Found PR from commit SHA: {_pulls[0].number}") + return _pulls[0] + self.logger.debug(f"{self.log_prefix} No PR found for commit SHA") + else: + self.logger.debug(f"{self.log_prefix} No commit data in webhook payload") + self.logger.debug(f"{self.log_prefix} All PR lookup strategies exhausted, no PR found") return None diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index 7d1f9677..d88610bf 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -817,11 +817,20 @@ async def test_process_check_run_event(self) -> None: mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() @pytest.mark.asyncio - async def test_process_status_event_success(self) -> None: - """Test processing status event with state=success triggers can-be-merged re-evaluation.""" + @pytest.mark.parametrize( + ("state", "should_recheck"), + [ + ("success", True), + ("pending", False), + ("failure", True), + ("error", True), + ], + ) + async def test_process_status_event(self, state: str, should_recheck: bool) -> None: + """Test processing status event with various states.""" logger = Mock() status_data = { - "state": "success", + "state": state, "context": "pre-commit.ci", "sha": "abc123", "repository": {"name": "test-repo", "full_name": "org/test-repo"}, @@ -837,7 +846,6 @@ async def test_process_status_event_success(self) -> None: 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 repository and get_pulls to return a PR with matching head.sha mock_repo = Mock() mock_repo.get_git_tree.return_value.tree = [] mock_pr = Mock() @@ -871,187 +879,24 @@ async def test_process_status_event_success(self) -> None: webhook = GithubWebhook(status_data, headers, logger) await webhook.process() - mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() - - @pytest.mark.asyncio - async def test_process_status_event_pending_skipped(self) -> None: - """Test processing status event with state=pending is skipped.""" - logger = Mock() - status_data = { - "state": "pending", - "context": "pre-commit.ci", - "sha": "abc123", - "repository": {"name": "test-repo", "full_name": "org/test-repo"}, - } - headers = Headers({"X-GitHub-Event": "status", "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_pulls.return_value = [mock_pr] - 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(status_data, headers, logger) - await webhook.process() - - # check_if_can_be_merged should NOT be called for pending state - mock_pr_handler.return_value.check_if_can_be_merged.assert_not_awaited() - - @pytest.mark.asyncio - async def test_process_status_event_failure_triggers_reevaluation(self) -> None: - """Test processing status event with state=failure triggers can-be-merged re-evaluation.""" - logger = Mock() - status_data = { - "state": "failure", - "context": "pre-commit.ci", - "sha": "abc123", - "repository": {"name": "test-repo", "full_name": "org/test-repo"}, - } - headers = Headers({"X-GitHub-Event": "status", "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_pulls.return_value = [mock_pr] - 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(status_data, headers, logger) - await webhook.process() - - # check_if_can_be_merged SHOULD be called for failure state (terminal state) - mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() - - @pytest.mark.asyncio - async def test_process_status_event_error_triggers_reevaluation(self) -> None: - """Test processing status event with state=error triggers can-be-merged re-evaluation.""" - logger = Mock() - status_data = { - "state": "error", - "context": "pre-commit.ci", - "sha": "abc123", - "repository": {"name": "test-repo", "full_name": "org/test-repo"}, - } - headers = Headers({"X-GitHub-Event": "status", "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_pulls.return_value = [mock_pr] - 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(status_data, headers, logger) - await webhook.process() - - # check_if_can_be_merged SHOULD be called for error state (terminal state) - mock_pr_handler.return_value.check_if_can_be_merged.assert_awaited_once() + 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() @pytest.mark.asyncio async def test_get_pull_request_from_status_sha(self) -> None: - """Test getting pull request from status event SHA.""" + """Test getting pull request from status event SHA. + + Verifies that even when commit.get_pulls() returns a stale PR, + the status SHA lookup finds the correct matching PR. + """ logger = Mock() status_data = { "state": "success", "context": "pre-commit.ci", "sha": "status-sha-123", + "commit": {"sha": "status-sha-123"}, # Real payloads include this "repository": {"name": "test-repo", "full_name": "org/test-repo"}, } headers = Headers({"X-GitHub-Event": "status", "X-GitHub-Delivery": "abc"}) @@ -1070,6 +915,14 @@ async def test_get_pull_request_from_status_sha(self) -> None: mock_pr.title = "Test PR" mock_pr.number = 42 mock_repo.get_pulls.return_value = [mock_pr] + + # Stale PR from commit.get_pulls() fallback + stale_pr = Mock() + stale_pr.head.sha = "old-sha" + mock_commit = Mock() + mock_commit.get_pulls.return_value = [stale_pr] + mock_repo.get_commit.return_value = mock_commit + 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: From 2ad2ad919b25a7e8331b6335b7d6909ab3a8c3d2 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 17 Mar 2026 16:46:17 +0200 Subject: [PATCH 5/5] test: assert commit fallback not used when status SHA matches --- webhook_server/tests/test_github_api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/webhook_server/tests/test_github_api.py b/webhook_server/tests/test_github_api.py index d88610bf..ccfe97f8 100644 --- a/webhook_server/tests/test_github_api.py +++ b/webhook_server/tests/test_github_api.py @@ -935,6 +935,7 @@ async def test_get_pull_request_from_status_sha(self) -> None: result = await gh.get_pull_request() assert result == mock_pr mock_repo.get_pulls.assert_called_once_with(state="open") + mock_repo.get_commit.assert_not_called() @pytest.mark.asyncio async def test_get_pull_request_by_number(