From 85cf8d8d4ac3ea2bcbcfa1d77505f6f15b421697 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 1 Jan 2026 20:51:31 +0200 Subject: [PATCH 1/6] feat: use Compare API for accurate needs-rebase detection Replace unreliable mergeable_state with GitHub Compare API for detecting when PRs need rebasing. Conflict detection now uses mergeable property directly. - Add _update_conflict_label() helper for consistent label handling - Use Compare API behind_by and status fields for rebase detection - Conflict takes priority over rebase (shows one issue at a time) - Handle API failures gracefully with conflict-only fallback - Add comprehensive test coverage for all scenarios --- .../libs/handlers/pull_request_handler.py | 127 ++++++++++++++-- .../tests/test_pull_request_handler.py | 142 ++++++++++++++++-- 2 files changed, 250 insertions(+), 19 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 126b4671..8d8855de 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -864,22 +864,131 @@ async def remove_labels_when_pull_request_sync(self, pull_request: PullRequest) if isinstance(result, Exception): self.logger.error(f"{self.log_prefix} Async task failed: {result}") + async def _update_conflict_label(self, pull_request: PullRequest, has_conflicts: bool) -> None: + """Update has-conflicts label based on mergeable status. + + When there's a conflict, we also remove the needs-rebase label since conflict is the primary blocker. + Once the conflict is resolved, the webhook fires again and rebase status is re-evaluated. + """ + if has_conflicts: + await self.labels_handler._add_label(pull_request=pull_request, label=HAS_CONFLICTS_LABEL_STR) + # Remove needs-rebase label if present - conflict is the primary blocker + await self.labels_handler._remove_label(pull_request=pull_request, label=NEEDS_REBASE_LABEL_STR) + else: + await self.labels_handler._remove_label(pull_request=pull_request, label=HAS_CONFLICTS_LABEL_STR) + async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> None: - merge_state = await asyncio.to_thread(lambda: pull_request.mergeable_state) - self.logger.debug(f"{self.log_prefix} Mergeable state is {merge_state}") - if merge_state == "unknown": + """Label pull request based on merge state using Compare API. + + Uses GitHub's Compare API for accurate behind/diverged detection instead of + the limited `mergeable_state` property. + + Detection Logic: + - Conflicts: `pull_request.mergeable == False` + - Needs rebase: Compare API `behind_by > 0` OR `status == "diverged"` + + Label Priority: + Conflict is the primary blocker. When present, only the `has-conflicts` label is shown. + + Decision Flow: + 1. mergeable is None → skip (not yet computed by GitHub) + 2. has_conflicts → add has-conflicts, remove needs-rebase, exit + 3. no conflicts → remove has-conflicts, check rebase: + - needs_rebase → add needs-rebase + - no rebase → remove needs-rebase + + API Fallback: + If Compare API fails or returns incomplete data, conflict label is + still updated based on available `mergeable` data. + + Args: + pull_request: The GitHub pull request object to label. + + Compare API Reference: + GET /repos/{owner}/{repo}/compare/{base}...{head} + Response fields used: + - behind_by: int - commits behind base branch + - status: str - "ahead", "behind", "diverged", "identical" + """ + # Get mergeable status - may be None if not yet computed by GitHub + mergeable = await asyncio.to_thread(lambda: pull_request.mergeable) + if mergeable is None: + self.logger.debug(f"{self.log_prefix} Mergeable status is None (not yet computed), skipping") + return + + # Check for conflicts + has_conflicts = mergeable is False + self.logger.debug(f"{self.log_prefix} Has conflicts: {has_conflicts}") + + # Use Compare API to check if PR is behind or diverged + base_ref, head_user_login, head_ref = await asyncio.gather( + asyncio.to_thread(lambda: pull_request.base.ref), + asyncio.to_thread(lambda: pull_request.head.user.login), + asyncio.to_thread(lambda: pull_request.head.ref), + ) + head_ref_full = f"{head_user_login}:{head_ref}" + + # Call Compare API to get accurate behind/diverged status + def _compare_branches() -> dict[str, Any] | None: + try: + _headers, data = self.repository._requester.requestJsonAndCheck( + "GET", + f"{self.repository.url}/compare/{base_ref}...{head_ref_full}", + ) + return data + except GithubException: + self.logger.exception(f"{self.log_prefix} Failed to call Compare API for {base_ref}...{head_ref_full}") + return None + except Exception: + self.logger.exception(f"{self.log_prefix} Unexpected error calling Compare API") + return None + + compare_data = await asyncio.to_thread(_compare_branches) + if compare_data is None: + self.logger.warning( + f"{self.log_prefix} Compare API failed, updating conflict label only (rebase status unknown)" + ) + # Still process conflict label since we have mergeable data + await self._update_conflict_label(pull_request=pull_request, has_conflicts=has_conflicts) + return + + # Validate Compare API response structure + behind_by = compare_data.get("behind_by") + status = compare_data.get("status") + + if behind_by is None or status is None: + self.logger.warning( + f"{self.log_prefix} Compare API returned incomplete data (behind_by={behind_by}, status={status}), " + "skipping rebase label updates" + ) + # Still process conflict label since we have mergeable data + await self._update_conflict_label(pull_request=pull_request, has_conflicts=has_conflicts) return - if merge_state == "behind": + is_behind = behind_by > 0 + is_diverged = status == "diverged" + needs_rebase = is_behind or is_diverged + + self.logger.debug( + f"{self.log_prefix} Compare API results - behind_by: {behind_by}, " + f"status: {status}, needs_rebase: {needs_rebase}" + ) + + # Apply labels with priority: conflict is the primary blocker + # If there's a conflict, that's the blocker - user needs to resolve it first + # Once conflict is resolved, the webhook fires again and checks rebase status + await self._update_conflict_label(pull_request=pull_request, has_conflicts=has_conflicts) + + # If there's a conflict, we're done (conflict is the primary blocker) + if has_conflicts: + return + + # No conflicts - check rebase status + if needs_rebase: await self.labels_handler._add_label(pull_request=pull_request, label=NEEDS_REBASE_LABEL_STR) else: await self.labels_handler._remove_label(pull_request=pull_request, label=NEEDS_REBASE_LABEL_STR) - if merge_state == "dirty": - await self.labels_handler._add_label(pull_request=pull_request, label=HAS_CONFLICTS_LABEL_STR) - else: - await self.labels_handler._remove_label(pull_request=pull_request, label=HAS_CONFLICTS_LABEL_STR) - async def _process_verified_for_update_or_new_pull_request(self, pull_request: PullRequest) -> None: if not self.github_webhook.verified_job: return diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 93e1a3bd..f59739f8 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -1,4 +1,5 @@ import asyncio +from typing import Any from unittest.mock import AsyncMock, MagicMock, Mock, patch import pytest @@ -525,8 +526,16 @@ async def test_remove_labels_when_pull_request_sync( async def test_label_pull_request_by_merge_state_mergeable( self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock ) -> None: + """Test labeling pull request when mergeable and up-to-date.""" mock_pull_request.mergeable = True - mock_pull_request.mergeable_state = "clean" + mock_pull_request.base.ref = "main" + mock_pull_request.head.user.login = "test-user" + mock_pull_request.head.ref = "feature-branch" + + # Mock Compare API response - up-to-date + mock_compare_data = {"behind_by": 0, "status": "ahead"} + pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) + with patch.object(pull_request_handler.labels_handler, "_remove_label", new=AsyncMock()) as mock_remove_label: await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) assert mock_remove_label.await_count == 2 @@ -537,7 +546,13 @@ async def test_label_pull_request_by_merge_state_needs_rebase( ) -> None: """Test labeling pull request by merge state when needs rebase.""" mock_pull_request.mergeable = True - mock_pull_request.mergeable_state = "behind" + mock_pull_request.base.ref = "main" + mock_pull_request.head.user.login = "test-user" + mock_pull_request.head.ref = "feature-branch" + + # Mock Compare API response - behind + mock_compare_data = {"behind_by": 5, "status": "behind"} + pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) with patch.object(pull_request_handler.labels_handler, "_add_label") as mock_add_label: await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) @@ -549,7 +564,13 @@ async def test_label_pull_request_by_merge_state_has_conflicts( ) -> None: """Test labeling pull request by merge state when has conflicts.""" mock_pull_request.mergeable = False - mock_pull_request.mergeable_state = "dirty" + mock_pull_request.base.ref = "main" + mock_pull_request.head.user.login = "test-user" + mock_pull_request.head.ref = "feature-branch" + + # Mock Compare API response - up-to-date but has conflicts + mock_compare_data = {"behind_by": 0, "status": "ahead"} + pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) with patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label: await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) @@ -1738,16 +1759,65 @@ async def test_set_pull_request_automerge_exception( async def test_label_pull_request_by_merge_state_unknown( self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock ) -> None: - """Test label_pull_request_by_merge_state when unknown.""" - mock_pull_request.mergeable_state = "unknown" + """Test label_pull_request_by_merge_state when mergeable is None.""" + mock_pull_request.mergeable = None # Not yet computed by GitHub - with patch( - "asyncio.to_thread", side_effect=lambda f, *args, **kwargs: f(*args, **kwargs) if callable(f) else None - ): - await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) + await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) - # Should return early + # Should return early, no label operations pull_request_handler.labels_handler._add_label.assert_not_called() + pull_request_handler.labels_handler._remove_label.assert_not_called() + + @pytest.mark.asyncio + async def test_label_pull_request_by_merge_state_diverged( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test labeling pull request when diverged from base.""" + mock_pull_request.mergeable = True + mock_pull_request.base.ref = "main" + mock_pull_request.head.user.login = "test-user" + mock_pull_request.head.ref = "feature-branch" + + # Mock Compare API response - diverged + mock_compare_data = {"behind_by": 3, "status": "diverged"} + pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) + + with patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label: + await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) + mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=NEEDS_REBASE_LABEL_STR) + + @pytest.mark.asyncio + async def test_label_pull_request_by_merge_state_behind_and_conflicts( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test labeling pull request when behind and has conflicts. + + When a PR has conflicts, only the conflict label should be added. + The needs-rebase label is removed if present, since conflict is the primary blocker. + Once the conflict is resolved, the webhook fires again and rebase status is re-evaluated. + """ + mock_pull_request.mergeable = False + mock_pull_request.base.ref = "main" + mock_pull_request.head.user.login = "test-user" + mock_pull_request.head.ref = "feature-branch" + + # Mock Compare API response - behind + mock_compare_data = {"behind_by": 2, "status": "behind"} + pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) + + with ( + patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label, + patch.object( + pull_request_handler.labels_handler, "_remove_label", new_callable=AsyncMock + ) as mock_remove_label, + ): + await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) + # Should add only conflict label (conflict is the primary blocker) + assert mock_add_label.call_count == 1 + assert mock_add_label.call_args.kwargs["label"] == HAS_CONFLICTS_LABEL_STR + # Should remove needs-rebase label (conflict takes priority) + assert mock_remove_label.call_count == 1 + assert mock_remove_label.call_args.kwargs["label"] == NEEDS_REBASE_LABEL_STR @pytest.mark.asyncio async def test_delete_registry_tag_via_regctl_failure( @@ -1782,3 +1852,55 @@ async def test_delete_registry_tag_via_regctl_failure( pull_request_handler.logger.error.assert_called_with( "[TEST] Failed to delete tag: tag. OUT:Delete failed. ERR:Error" ) + + @pytest.mark.asyncio + async def test_label_pull_request_by_merge_state_compare_api_failure( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test handling of Compare API failure - should still update conflict label.""" + mock_pull_request.mergeable = True # No conflicts + mock_pull_request.base.ref = "main" + mock_pull_request.head.user.login = "test-user" + mock_pull_request.head.ref = "feature-branch" + + # Mock Compare API to raise GithubException + pull_request_handler.repository._requester.requestJsonAndCheck = Mock( + side_effect=GithubException(500, {"message": "API error"}, None) + ) + + # Reset mocks + pull_request_handler.labels_handler._add_label.reset_mock() + pull_request_handler.labels_handler._remove_label.reset_mock() + + await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) + + # Should still remove has-conflicts label even if Compare API fails (mergeable=True) + pull_request_handler.labels_handler._remove_label.assert_called_once_with( + pull_request=mock_pull_request, label=HAS_CONFLICTS_LABEL_STR + ) + pull_request_handler.labels_handler._add_label.assert_not_called() + + @pytest.mark.asyncio + async def test_label_pull_request_by_merge_state_incomplete_compare_data( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test handling of incomplete Compare API response - should still handle conflict label.""" + mock_pull_request.mergeable = False # Has conflicts + mock_pull_request.base.ref = "main" + mock_pull_request.head.user.login = "test-user" + mock_pull_request.head.ref = "feature-branch" + + # Mock Compare API with missing behind_by key + mock_compare_data: dict[str, Any] = {"status": "behind"} # Missing behind_by + pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) + + # Reset mocks + pull_request_handler.labels_handler._add_label.reset_mock() + pull_request_handler.labels_handler._remove_label.reset_mock() + + await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) + + # Should still add has-conflicts label even if compare data incomplete + pull_request_handler.labels_handler._add_label.assert_called_once_with( + pull_request=mock_pull_request, label=HAS_CONFLICTS_LABEL_STR + ) From 09be41700aeb2fabe57ca0aec17e397a9b9f7fe0 Mon Sep 17 00:00:00 2001 From: rnetser Date: Mon, 5 Jan 2026 12:07:39 +0200 Subject: [PATCH 2/6] refactor: simplify merge state labeling with early exit pattern MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Simplified label_pull_request_by_merge_state() with clearer logic: - Early exit for conflicts (conflicts take precedence over needs-rebase) - Check pull_request.mergeable for conflicts, Compare API for rebase status - Only add/remove labels when state changes (optimization) - Type safety for Compare API response (default to int/str) - Extracted _compare_branches() method for reusability - Updated tests to match new logic flow and early exit behavior Improvements: - Reduced complexity: 90 → 75 lines in main method - Better separation of concerns (conflicts vs rebase detection) - More efficient label operations (check before add/remove) - Clearer code flow with step-by-step comments --- .../libs/handlers/pull_request_handler.py | 171 +++++++++--------- .../tests/test_pull_request_handler.py | 145 +++++++++++---- 2 files changed, 187 insertions(+), 129 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 8d8855de..139382a0 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -864,63 +864,87 @@ async def remove_labels_when_pull_request_sync(self, pull_request: PullRequest) if isinstance(result, Exception): self.logger.error(f"{self.log_prefix} Async task failed: {result}") - async def _update_conflict_label(self, pull_request: PullRequest, has_conflicts: bool) -> None: - """Update has-conflicts label based on mergeable status. + async def _compare_branches(self, base_ref: str, head_ref_full: str) -> dict[str, Any] | None: + """Call GitHub Compare API to get branch comparison data for rebase detection. - When there's a conflict, we also remove the needs-rebase label since conflict is the primary blocker. - Once the conflict is resolved, the webhook fires again and rebase status is re-evaluated. - """ - if has_conflicts: - await self.labels_handler._add_label(pull_request=pull_request, label=HAS_CONFLICTS_LABEL_STR) - # Remove needs-rebase label if present - conflict is the primary blocker - await self.labels_handler._remove_label(pull_request=pull_request, label=NEEDS_REBASE_LABEL_STR) - else: - await self.labels_handler._remove_label(pull_request=pull_request, label=HAS_CONFLICTS_LABEL_STR) + This API is used ONLY for detecting if a PR is behind/diverged from base branch. + It does NOT provide conflict information - use pull_request.mergeable for conflicts. - async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> None: - """Label pull request based on merge state using Compare API. + Args: + base_ref: Base branch reference (e.g., "main"). + head_ref_full: Full head reference including owner (e.g., "user:branch"). - Uses GitHub's Compare API for accurate behind/diverged detection instead of - the limited `mergeable_state` property. + Returns: + Compare API response data or None if API call fails. - Detection Logic: - - Conflicts: `pull_request.mergeable == False` - - Needs rebase: Compare API `behind_by > 0` OR `status == "diverged"` + Compare API Reference: + GET /repos/{owner}/{repo}/compare/{base}...{head} + Response fields used: + - behind_by: int - commits behind base branch + - status: str - "ahead", "behind", "diverged", "identical" - Label Priority: - Conflict is the primary blocker. When present, only the `has-conflicts` label is shown. + NOTE: This API does NOT return conflict information (mergeable/mergeable_state). + """ + try: + _headers, data = await asyncio.to_thread( + self.repository._requester.requestJsonAndCheck, + "GET", + f"{self.repository.url}/compare/{base_ref}...{head_ref_full}", + ) + return data + except GithubException: + self.logger.exception(f"{self.log_prefix} Failed to call Compare API for {base_ref}...{head_ref_full}") + return None + except Exception: + self.logger.exception(f"{self.log_prefix} Unexpected error calling Compare API") + return None - Decision Flow: - 1. mergeable is None → skip (not yet computed by GitHub) - 2. has_conflicts → add has-conflicts, remove needs-rebase, exit - 3. no conflicts → remove has-conflicts, check rebase: - - needs_rebase → add needs-rebase - - no rebase → remove needs-rebase + async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> None: + """Label pull request based on merge state. + + Simple flow: + 1. Check pull_request.mergeable for conflicts + 2. If has conflicts → add has-conflicts, remove needs-rebase, exit + 3. Else → remove has-conflicts, check Compare API for rebase status - API Fallback: - If Compare API fails or returns incomplete data, conflict label is - still updated based on available `mergeable` data. + Uses both GitHub APIs for accurate labeling: + - has-conflicts: pull_request.mergeable == False (true merge conflict detection) + - needs-rebase: Compare API behind_by > 0 or status == "diverged" Args: pull_request: The GitHub pull request object to label. - - Compare API Reference: - GET /repos/{owner}/{repo}/compare/{base}...{head} - Response fields used: - - behind_by: int - commits behind base branch - - status: str - "ahead", "behind", "diverged", "identical" """ - # Get mergeable status - may be None if not yet computed by GitHub - mergeable = await asyncio.to_thread(lambda: pull_request.mergeable) - if mergeable is None: - self.logger.debug(f"{self.log_prefix} Mergeable status is None (not yet computed), skipping") - return + # Get current labels (single API call for optimization) + current_labels = await asyncio.to_thread(lambda: {label.name for label in pull_request.labels}) + has_conflicts_label_exists = HAS_CONFLICTS_LABEL_STR in current_labels + needs_rebase_label_exists = NEEDS_REBASE_LABEL_STR in current_labels - # Check for conflicts + # Step 1: Check for conflicts first + mergeable = await asyncio.to_thread(lambda: pull_request.mergeable) has_conflicts = mergeable is False - self.logger.debug(f"{self.log_prefix} Has conflicts: {has_conflicts}") - # Use Compare API to check if PR is behind or diverged + if has_conflicts: + # Has conflicts - add has-conflicts label, remove needs-rebase, and exit + self.logger.debug(f"{self.log_prefix} PR has conflicts (mergeable=False)") + + if not has_conflicts_label_exists: + self.logger.debug(f"{self.log_prefix} Adding {HAS_CONFLICTS_LABEL_STR} label") + await self.labels_handler._add_label(pull_request=pull_request, label=HAS_CONFLICTS_LABEL_STR) + + if needs_rebase_label_exists: + self.logger.debug( + f"{self.log_prefix} Removing {NEEDS_REBASE_LABEL_STR} label (conflict takes precedence)" + ) + await self.labels_handler._remove_label(pull_request=pull_request, label=NEEDS_REBASE_LABEL_STR) + + return # Exit early - conflicts take precedence + + # Step 2: No conflicts - remove has-conflicts label if present + if has_conflicts_label_exists: + self.logger.debug(f"{self.log_prefix} Removing {HAS_CONFLICTS_LABEL_STR} label") + await self.labels_handler._remove_label(pull_request=pull_request, label=HAS_CONFLICTS_LABEL_STR) + + # Step 3: Check if needs rebase via Compare API base_ref, head_user_login, head_ref = await asyncio.gather( asyncio.to_thread(lambda: pull_request.base.ref), asyncio.to_thread(lambda: pull_request.head.user.login), @@ -928,65 +952,32 @@ async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> ) head_ref_full = f"{head_user_login}:{head_ref}" - # Call Compare API to get accurate behind/diverged status - def _compare_branches() -> dict[str, Any] | None: - try: - _headers, data = self.repository._requester.requestJsonAndCheck( - "GET", - f"{self.repository.url}/compare/{base_ref}...{head_ref_full}", - ) - return data - except GithubException: - self.logger.exception(f"{self.log_prefix} Failed to call Compare API for {base_ref}...{head_ref_full}") - return None - except Exception: - self.logger.exception(f"{self.log_prefix} Unexpected error calling Compare API") - return None - - compare_data = await asyncio.to_thread(_compare_branches) + compare_data = await self._compare_branches(base_ref=base_ref, head_ref_full=head_ref_full) if compare_data is None: - self.logger.warning( - f"{self.log_prefix} Compare API failed, updating conflict label only (rebase status unknown)" - ) - # Still process conflict label since we have mergeable data - await self._update_conflict_label(pull_request=pull_request, has_conflicts=has_conflicts) + self.logger.warning(f"{self.log_prefix} Compare API failed, skipping rebase label update") return - # Validate Compare API response structure behind_by = compare_data.get("behind_by") status = compare_data.get("status") - if behind_by is None or status is None: - self.logger.warning( - f"{self.log_prefix} Compare API returned incomplete data (behind_by={behind_by}, status={status}), " - "skipping rebase label updates" - ) - # Still process conflict label since we have mergeable data - await self._update_conflict_label(pull_request=pull_request, has_conflicts=has_conflicts) - return + # Type safety + if not isinstance(behind_by, int): + behind_by = 0 + if not isinstance(status, str): + status = "" - is_behind = behind_by > 0 - is_diverged = status == "diverged" - needs_rebase = is_behind or is_diverged + needs_rebase = behind_by > 0 or status == "diverged" self.logger.debug( - f"{self.log_prefix} Compare API results - behind_by: {behind_by}, " - f"status: {status}, needs_rebase: {needs_rebase}" + f"{self.log_prefix} Compare API - behind_by: {behind_by}, status: {status}, needs_rebase: {needs_rebase}" ) - # Apply labels with priority: conflict is the primary blocker - # If there's a conflict, that's the blocker - user needs to resolve it first - # Once conflict is resolved, the webhook fires again and checks rebase status - await self._update_conflict_label(pull_request=pull_request, has_conflicts=has_conflicts) - - # If there's a conflict, we're done (conflict is the primary blocker) - if has_conflicts: - return - - # No conflicts - check rebase status - if needs_rebase: + # Step 4: Update needs-rebase label + if needs_rebase and not needs_rebase_label_exists: + self.logger.debug(f"{self.log_prefix} Adding {NEEDS_REBASE_LABEL_STR} label") await self.labels_handler._add_label(pull_request=pull_request, label=NEEDS_REBASE_LABEL_STR) - else: + elif not needs_rebase and needs_rebase_label_exists: + self.logger.debug(f"{self.log_prefix} Removing {NEEDS_REBASE_LABEL_STR} label") await self.labels_handler._remove_label(pull_request=pull_request, label=NEEDS_REBASE_LABEL_STR) async def _process_verified_for_update_or_new_pull_request(self, pull_request: PullRequest) -> None: diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index f59739f8..3500222a 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -532,6 +532,13 @@ async def test_label_pull_request_by_merge_state_mergeable( mock_pull_request.head.user.login = "test-user" mock_pull_request.head.ref = "feature-branch" + # Mock existing labels - PR currently has both labels that need to be removed + mock_label1 = Mock() + mock_label1.name = HAS_CONFLICTS_LABEL_STR + mock_label2 = Mock() + mock_label2.name = NEEDS_REBASE_LABEL_STR + mock_pull_request.labels = [mock_label1, mock_label2] + # Mock Compare API response - up-to-date mock_compare_data = {"behind_by": 0, "status": "ahead"} pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) @@ -550,6 +557,9 @@ async def test_label_pull_request_by_merge_state_needs_rebase( mock_pull_request.head.user.login = "test-user" mock_pull_request.head.ref = "feature-branch" + # Mock existing labels - PR has no labels currently + mock_pull_request.labels = [] + # Mock Compare API response - behind mock_compare_data = {"behind_by": 5, "status": "behind"} pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) @@ -562,18 +572,26 @@ async def test_label_pull_request_by_merge_state_needs_rebase( async def test_label_pull_request_by_merge_state_has_conflicts( self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock ) -> None: - """Test labeling pull request by merge state when has conflicts.""" - mock_pull_request.mergeable = False + """Test labeling pull request by merge state when has conflicts. + + Uses pull_request.mergeable == False to detect conflicts. + When mergeable is False, ONLY has-conflicts label is set (conflicts take precedence over needs-rebase). + """ + mock_pull_request.mergeable = False # Conflict detected via mergeable mock_pull_request.base.ref = "main" mock_pull_request.head.user.login = "test-user" mock_pull_request.head.ref = "feature-branch" - # Mock Compare API response - up-to-date but has conflicts + # Mock existing labels - PR has no labels currently + mock_pull_request.labels = [] + + # Mock Compare API response - clean (no rebase needed) mock_compare_data = {"behind_by": 0, "status": "ahead"} pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) with patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label: await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) + # When mergeable is False, only has-conflicts label is set (conflicts take precedence) mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=HAS_CONFLICTS_LABEL_STR) @pytest.mark.asyncio @@ -1759,31 +1777,78 @@ async def test_set_pull_request_automerge_exception( async def test_label_pull_request_by_merge_state_unknown( self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock ) -> None: - """Test label_pull_request_by_merge_state when mergeable is None.""" + """Test label_pull_request_by_merge_state when mergeable=None. + + When mergeable=None (not yet computed), has_conflicts is False. + If Compare API shows behind_by > 0, needs-rebase label should be added. + """ mock_pull_request.mergeable = None # Not yet computed by GitHub + mock_pull_request.base.ref = "main" + mock_pull_request.head.user.login = "test-user" + mock_pull_request.head.ref = "feature-branch" - await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) + # Mock existing labels - PR has no labels currently + mock_pull_request.labels = [] - # Should return early, no label operations - pull_request_handler.labels_handler._add_label.assert_not_called() - pull_request_handler.labels_handler._remove_label.assert_not_called() + # Mock Compare API response - behind by 5 commits + mock_compare_data = {"behind_by": 5, "status": "behind"} + pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) + + with patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label: + await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) + # Should add needs-rebase label since behind_by > 0 and no conflicts (mergeable=None means no conflict) + mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=NEEDS_REBASE_LABEL_STR) @pytest.mark.asyncio async def test_label_pull_request_by_merge_state_diverged( self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock ) -> None: - """Test labeling pull request when diverged from base.""" - mock_pull_request.mergeable = True + """Test labeling pull request when diverged from base. + + Uses Compare API status='diverged' to detect needs-rebase. + When status='diverged', only needs-rebase label is set (no conflicts via mergeable). + """ + mock_pull_request.mergeable = True # No conflicts mock_pull_request.base.ref = "main" mock_pull_request.head.user.login = "test-user" mock_pull_request.head.ref = "feature-branch" - # Mock Compare API response - diverged + # Mock existing labels - PR has no labels currently + mock_pull_request.labels = [] + + # Mock Compare API response - diverged (needs rebase) mock_compare_data = {"behind_by": 3, "status": "diverged"} pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) with patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label: await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) + # When diverged and no conflicts, only needs-rebase label is set + mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=NEEDS_REBASE_LABEL_STR) + + @pytest.mark.asyncio + async def test_label_pull_request_by_merge_state_diverged_zero_behind( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test diverged status with zero behind_by (edge case). + + When status is 'diverged' but behind_by is 0, needs_rebase should still be True + because diverged means the branch has both commits ahead AND commits that differ + from the base branch. + """ + mock_pull_request.mergeable = True # No conflicts + mock_pull_request.base.ref = "main" + mock_pull_request.head.user.login = "test-user" + mock_pull_request.head.ref = "feature-branch" + mock_pull_request.labels = [] # No existing labels + + # Edge case: diverged but behind_by=0 + mock_compare_data = {"behind_by": 0, "status": "diverged"} + pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) + + with patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label: + await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) + + # Should add needs-rebase because status="diverged" (even with behind_by=0) mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=NEEDS_REBASE_LABEL_STR) @pytest.mark.asyncio @@ -1792,32 +1857,26 @@ async def test_label_pull_request_by_merge_state_behind_and_conflicts( ) -> None: """Test labeling pull request when behind and has conflicts. - When a PR has conflicts, only the conflict label should be added. - The needs-rebase label is removed if present, since conflict is the primary blocker. - Once the conflict is resolved, the webhook fires again and rebase status is re-evaluated. + Uses pull_request.mergeable == False to detect conflicts. + Uses Compare API status='diverged' to detect needs-rebase. + When both exist, ONLY has-conflicts label is set (conflicts take precedence over needs-rebase). """ - mock_pull_request.mergeable = False + mock_pull_request.mergeable = False # Conflict detected via mergeable mock_pull_request.base.ref = "main" mock_pull_request.head.user.login = "test-user" mock_pull_request.head.ref = "feature-branch" - # Mock Compare API response - behind - mock_compare_data = {"behind_by": 2, "status": "behind"} + # Mock existing labels - PR has no labels currently + mock_pull_request.labels = [] + + # Mock Compare API response - diverged (needs rebase) + mergeable=False (conflicts) + mock_compare_data = {"behind_by": 2, "status": "diverged"} pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) - with ( - patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label, - patch.object( - pull_request_handler.labels_handler, "_remove_label", new_callable=AsyncMock - ) as mock_remove_label, - ): + with patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label: await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) - # Should add only conflict label (conflict is the primary blocker) - assert mock_add_label.call_count == 1 - assert mock_add_label.call_args.kwargs["label"] == HAS_CONFLICTS_LABEL_STR - # Should remove needs-rebase label (conflict takes priority) - assert mock_remove_label.call_count == 1 - assert mock_remove_label.call_args.kwargs["label"] == NEEDS_REBASE_LABEL_STR + # When mergeable is False (conflicts), only has-conflicts label is set (conflicts take precedence) + mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=HAS_CONFLICTS_LABEL_STR) @pytest.mark.asyncio async def test_delete_registry_tag_via_regctl_failure( @@ -1857,12 +1916,15 @@ async def test_delete_registry_tag_via_regctl_failure( async def test_label_pull_request_by_merge_state_compare_api_failure( self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock ) -> None: - """Test handling of Compare API failure - should still update conflict label.""" - mock_pull_request.mergeable = True # No conflicts + """Test handling of Compare API failure - should log warning and return without updating labels.""" + mock_pull_request.mergeable = True # No conflicts (not used anymore) mock_pull_request.base.ref = "main" mock_pull_request.head.user.login = "test-user" mock_pull_request.head.ref = "feature-branch" + # Mock existing labels + mock_pull_request.labels = [] + # Mock Compare API to raise GithubException pull_request_handler.repository._requester.requestJsonAndCheck = Mock( side_effect=GithubException(500, {"message": "API error"}, None) @@ -1874,23 +1936,28 @@ async def test_label_pull_request_by_merge_state_compare_api_failure( await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) - # Should still remove has-conflicts label even if Compare API fails (mergeable=True) - pull_request_handler.labels_handler._remove_label.assert_called_once_with( - pull_request=mock_pull_request, label=HAS_CONFLICTS_LABEL_STR - ) + # With new simplified logic: if Compare API fails, no label updates at all + pull_request_handler.labels_handler._remove_label.assert_not_called() pull_request_handler.labels_handler._add_label.assert_not_called() @pytest.mark.asyncio async def test_label_pull_request_by_merge_state_incomplete_compare_data( self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock ) -> None: - """Test handling of incomplete Compare API response - should still handle conflict label.""" - mock_pull_request.mergeable = False # Has conflicts + """Test handling of incomplete Compare API response. + + With combined logic, pull_request.mergeable is used for conflicts. + If Compare API has missing behind_by but mergeable is False, conflict label is still added. + """ + mock_pull_request.mergeable = False # Conflict detected via mergeable mock_pull_request.base.ref = "main" mock_pull_request.head.user.login = "test-user" mock_pull_request.head.ref = "feature-branch" - # Mock Compare API with missing behind_by key + # Mock existing labels - PR has no labels currently + mock_pull_request.labels = [] + + # Mock Compare API with missing behind_by key - status is 'behind' (not diverged) mock_compare_data: dict[str, Any] = {"status": "behind"} # Missing behind_by pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) @@ -1900,7 +1967,7 @@ async def test_label_pull_request_by_merge_state_incomplete_compare_data( await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) - # Should still add has-conflicts label even if compare data incomplete + # mergeable is False, so conflict label should be added pull_request_handler.labels_handler._add_label.assert_called_once_with( pull_request=mock_pull_request, label=HAS_CONFLICTS_LABEL_STR ) From ab5f7b3af96b0986802cd24266af838961b49d17 Mon Sep 17 00:00:00 2001 From: rnetser Date: Mon, 5 Jan 2026 12:45:01 +0200 Subject: [PATCH 3/6] refactor: address review comments - use existing helpers and simplify Simplify needs-rebase detection logic by using existing helper methods and removing redundant code. --- .../libs/handlers/pull_request_handler.py | 14 +--- .../tests/test_pull_request_handler.py | 77 ++++++++++++++++--- 2 files changed, 72 insertions(+), 19 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 139382a0..da2707c0 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -915,7 +915,7 @@ async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> pull_request: The GitHub pull request object to label. """ # Get current labels (single API call for optimization) - current_labels = await asyncio.to_thread(lambda: {label.name for label in pull_request.labels}) + current_labels = await self.labels_handler.pull_request_labels_names(pull_request=pull_request) has_conflicts_label_exists = HAS_CONFLICTS_LABEL_STR in current_labels needs_rebase_label_exists = NEEDS_REBASE_LABEL_STR in current_labels @@ -925,7 +925,7 @@ async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> if has_conflicts: # Has conflicts - add has-conflicts label, remove needs-rebase, and exit - self.logger.debug(f"{self.log_prefix} PR has conflicts (mergeable=False)") + self.logger.debug(f"{self.log_prefix} PR has conflicts. `mergeable` = {mergeable})") if not has_conflicts_label_exists: self.logger.debug(f"{self.log_prefix} Adding {HAS_CONFLICTS_LABEL_STR} label") @@ -957,14 +957,8 @@ async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> self.logger.warning(f"{self.log_prefix} Compare API failed, skipping rebase label update") return - behind_by = compare_data.get("behind_by") - status = compare_data.get("status") - - # Type safety - if not isinstance(behind_by, int): - behind_by = 0 - if not isinstance(status, str): - status = "" + behind_by = compare_data.get("behind_by", 0) + status = compare_data.get("status", "") needs_rebase = behind_by > 0 or status == "diverged" diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 3500222a..04238912 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -543,7 +543,14 @@ async def test_label_pull_request_by_merge_state_mergeable( mock_compare_data = {"behind_by": 0, "status": "ahead"} pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) - with patch.object(pull_request_handler.labels_handler, "_remove_label", new=AsyncMock()) as mock_remove_label: + with ( + patch.object( + pull_request_handler.labels_handler, + "pull_request_labels_names", + new=AsyncMock(return_value=[HAS_CONFLICTS_LABEL_STR, NEEDS_REBASE_LABEL_STR]), + ), + patch.object(pull_request_handler.labels_handler, "_remove_label", new=AsyncMock()) as mock_remove_label, + ): await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) assert mock_remove_label.await_count == 2 @@ -564,7 +571,14 @@ async def test_label_pull_request_by_merge_state_needs_rebase( mock_compare_data = {"behind_by": 5, "status": "behind"} pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) - with patch.object(pull_request_handler.labels_handler, "_add_label") as mock_add_label: + with ( + patch.object( + pull_request_handler.labels_handler, + "pull_request_labels_names", + new=AsyncMock(return_value=[]), + ), + patch.object(pull_request_handler.labels_handler, "_add_label") as mock_add_label, + ): await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=NEEDS_REBASE_LABEL_STR) @@ -589,7 +603,14 @@ async def test_label_pull_request_by_merge_state_has_conflicts( mock_compare_data = {"behind_by": 0, "status": "ahead"} pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) - with patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label: + with ( + patch.object( + pull_request_handler.labels_handler, + "pull_request_labels_names", + new=AsyncMock(return_value=[]), + ), + patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label, + ): await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) # When mergeable is False, only has-conflicts label is set (conflicts take precedence) mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=HAS_CONFLICTS_LABEL_STR) @@ -1794,7 +1815,14 @@ async def test_label_pull_request_by_merge_state_unknown( mock_compare_data = {"behind_by": 5, "status": "behind"} pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) - with patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label: + with ( + patch.object( + pull_request_handler.labels_handler, + "pull_request_labels_names", + new=AsyncMock(return_value=[]), + ), + patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label, + ): await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) # Should add needs-rebase label since behind_by > 0 and no conflicts (mergeable=None means no conflict) mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=NEEDS_REBASE_LABEL_STR) @@ -1820,7 +1848,14 @@ async def test_label_pull_request_by_merge_state_diverged( mock_compare_data = {"behind_by": 3, "status": "diverged"} pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) - with patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label: + with ( + patch.object( + pull_request_handler.labels_handler, + "pull_request_labels_names", + new=AsyncMock(return_value=[]), + ), + patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label, + ): await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) # When diverged and no conflicts, only needs-rebase label is set mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=NEEDS_REBASE_LABEL_STR) @@ -1845,7 +1880,14 @@ async def test_label_pull_request_by_merge_state_diverged_zero_behind( mock_compare_data = {"behind_by": 0, "status": "diverged"} pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) - with patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label: + with ( + patch.object( + pull_request_handler.labels_handler, + "pull_request_labels_names", + new=AsyncMock(return_value=[]), + ), + patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label, + ): await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) # Should add needs-rebase because status="diverged" (even with behind_by=0) @@ -1873,7 +1915,14 @@ async def test_label_pull_request_by_merge_state_behind_and_conflicts( mock_compare_data = {"behind_by": 2, "status": "diverged"} pull_request_handler.repository._requester.requestJsonAndCheck = Mock(return_value=({}, mock_compare_data)) - with patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label: + with ( + patch.object( + pull_request_handler.labels_handler, + "pull_request_labels_names", + new=AsyncMock(return_value=[]), + ), + patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label, + ): await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) # When mergeable is False (conflicts), only has-conflicts label is set (conflicts take precedence) mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=HAS_CONFLICTS_LABEL_STR) @@ -1934,7 +1983,12 @@ async def test_label_pull_request_by_merge_state_compare_api_failure( pull_request_handler.labels_handler._add_label.reset_mock() pull_request_handler.labels_handler._remove_label.reset_mock() - await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) + with patch.object( + pull_request_handler.labels_handler, + "pull_request_labels_names", + new=AsyncMock(return_value=[]), + ): + await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) # With new simplified logic: if Compare API fails, no label updates at all pull_request_handler.labels_handler._remove_label.assert_not_called() @@ -1965,7 +2019,12 @@ async def test_label_pull_request_by_merge_state_incomplete_compare_data( pull_request_handler.labels_handler._add_label.reset_mock() pull_request_handler.labels_handler._remove_label.reset_mock() - await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) + with patch.object( + pull_request_handler.labels_handler, + "pull_request_labels_names", + new=AsyncMock(return_value=[]), + ): + await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) # mergeable is False, so conflict label should be added pull_request_handler.labels_handler._add_label.assert_called_once_with( From b56a0a3e9d969839f90128f4194eb4d66d3170e4 Mon Sep 17 00:00:00 2001 From: rnetser Date: Mon, 5 Jan 2026 17:10:48 +0200 Subject: [PATCH 4/6] feat: add structured logging to label_merge_state method Add context step tracking to label_pull_request_by_merge_state following the established pattern used elsewhere in the file. Tracks has_conflicts, needs_rebase, and compare_api_failed states. --- webhook_server/libs/handlers/pull_request_handler.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 85a14062..686c3470 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -782,6 +782,9 @@ async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> Args: pull_request: The GitHub pull request object to label. """ + if self.ctx: + self.ctx.start_step("label_merge_state") + # Get current labels (single API call for optimization) current_labels = await self.labels_handler.pull_request_labels_names(pull_request=pull_request) has_conflicts_label_exists = HAS_CONFLICTS_LABEL_STR in current_labels @@ -805,6 +808,8 @@ async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> ) await self.labels_handler._remove_label(pull_request=pull_request, label=NEEDS_REBASE_LABEL_STR) + if self.ctx: + self.ctx.complete_step("label_merge_state", has_conflicts=True, needs_rebase=False) return # Exit early - conflicts take precedence # Step 2: No conflicts - remove has-conflicts label if present @@ -823,6 +828,8 @@ async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> compare_data = await self._compare_branches(base_ref=base_ref, head_ref_full=head_ref_full) if compare_data is None: self.logger.warning(f"{self.log_prefix} Compare API failed, skipping rebase label update") + if self.ctx: + self.ctx.complete_step("label_merge_state", compare_api_failed=True) return behind_by = compare_data.get("behind_by", 0) @@ -842,6 +849,9 @@ async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> self.logger.debug(f"{self.log_prefix} Removing {NEEDS_REBASE_LABEL_STR} label") await self.labels_handler._remove_label(pull_request=pull_request, label=NEEDS_REBASE_LABEL_STR) + if self.ctx: + self.ctx.complete_step("label_merge_state", has_conflicts=False, needs_rebase=needs_rebase) + async def _process_verified_for_update_or_new_pull_request(self, pull_request: PullRequest) -> None: if not self.github_webhook.verified_job: return From 8cf578eb9b43d66ab9dac630b198f306735a4779 Mon Sep 17 00:00:00 2001 From: rnetser Date: Mon, 5 Jan 2026 17:30:34 +0200 Subject: [PATCH 5/6] refactor: allow has-conflicts and needs-rebase labels to coexist Both labels now reflect the actual PR state independently. Removes unnecessary API call to delete needs-rebase when conflicts are detected. --- webhook_server/libs/handlers/pull_request_handler.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 686c3470..998c90b2 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -772,13 +772,15 @@ async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> Simple flow: 1. Check pull_request.mergeable for conflicts - 2. If has conflicts → add has-conflicts, remove needs-rebase, exit + 2. If has conflicts → add has-conflicts, exit 3. Else → remove has-conflicts, check Compare API for rebase status Uses both GitHub APIs for accurate labeling: - has-conflicts: pull_request.mergeable == False (true merge conflict detection) - needs-rebase: Compare API behind_by > 0 or status == "diverged" + Both labels can coexist - they both reflect the actual PR state. + Args: pull_request: The GitHub pull request object to label. """ @@ -795,19 +797,13 @@ async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> has_conflicts = mergeable is False if has_conflicts: - # Has conflicts - add has-conflicts label, remove needs-rebase, and exit + # Has conflicts - add has-conflicts label and exit self.logger.debug(f"{self.log_prefix} PR has conflicts. `mergeable` = {mergeable})") if not has_conflicts_label_exists: self.logger.debug(f"{self.log_prefix} Adding {HAS_CONFLICTS_LABEL_STR} label") await self.labels_handler._add_label(pull_request=pull_request, label=HAS_CONFLICTS_LABEL_STR) - if needs_rebase_label_exists: - self.logger.debug( - f"{self.log_prefix} Removing {NEEDS_REBASE_LABEL_STR} label (conflict takes precedence)" - ) - await self.labels_handler._remove_label(pull_request=pull_request, label=NEEDS_REBASE_LABEL_STR) - if self.ctx: self.ctx.complete_step("label_merge_state", has_conflicts=True, needs_rebase=False) return # Exit early - conflicts take precedence From 72846e56f5c508f2d4f5f84483a2a49ee593a234 Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 6 Jan 2026 16:08:12 +0200 Subject: [PATCH 6/6] fix: add proper exception handling for structured logging Add try-except wrapper to label_pull_request_by_merge_state with ctx.fail_step() to match PR #968 structured logging patterns. Also fix log message format. --- .../libs/handlers/pull_request_handler.py | 111 ++++++++++-------- 1 file changed, 61 insertions(+), 50 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 998c90b2..0e3da70b 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -787,66 +787,77 @@ async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> if self.ctx: self.ctx.start_step("label_merge_state") - # Get current labels (single API call for optimization) - current_labels = await self.labels_handler.pull_request_labels_names(pull_request=pull_request) - has_conflicts_label_exists = HAS_CONFLICTS_LABEL_STR in current_labels - needs_rebase_label_exists = NEEDS_REBASE_LABEL_STR in current_labels + try: + # Get current labels (single API call for optimization) + current_labels = await self.labels_handler.pull_request_labels_names(pull_request=pull_request) + has_conflicts_label_exists = HAS_CONFLICTS_LABEL_STR in current_labels + needs_rebase_label_exists = NEEDS_REBASE_LABEL_STR in current_labels - # Step 1: Check for conflicts first - mergeable = await asyncio.to_thread(lambda: pull_request.mergeable) - has_conflicts = mergeable is False + # Step 1: Check for conflicts first + mergeable = await asyncio.to_thread(lambda: pull_request.mergeable) + has_conflicts = mergeable is False - if has_conflicts: - # Has conflicts - add has-conflicts label and exit - self.logger.debug(f"{self.log_prefix} PR has conflicts. `mergeable` = {mergeable})") + if has_conflicts: + # Has conflicts - add has-conflicts label and exit + self.logger.debug(f"{self.log_prefix} PR has conflicts. {mergeable=}") - if not has_conflicts_label_exists: - self.logger.debug(f"{self.log_prefix} Adding {HAS_CONFLICTS_LABEL_STR} label") - await self.labels_handler._add_label(pull_request=pull_request, label=HAS_CONFLICTS_LABEL_STR) + if not has_conflicts_label_exists: + self.logger.debug(f"{self.log_prefix} Adding {HAS_CONFLICTS_LABEL_STR} label") + await self.labels_handler._add_label(pull_request=pull_request, label=HAS_CONFLICTS_LABEL_STR) - if self.ctx: - self.ctx.complete_step("label_merge_state", has_conflicts=True, needs_rebase=False) - return # Exit early - conflicts take precedence - - # Step 2: No conflicts - remove has-conflicts label if present - if has_conflicts_label_exists: - self.logger.debug(f"{self.log_prefix} Removing {HAS_CONFLICTS_LABEL_STR} label") - await self.labels_handler._remove_label(pull_request=pull_request, label=HAS_CONFLICTS_LABEL_STR) - - # Step 3: Check if needs rebase via Compare API - base_ref, head_user_login, head_ref = await asyncio.gather( - asyncio.to_thread(lambda: pull_request.base.ref), - asyncio.to_thread(lambda: pull_request.head.user.login), - asyncio.to_thread(lambda: pull_request.head.ref), - ) - head_ref_full = f"{head_user_login}:{head_ref}" + if self.ctx: + self.ctx.complete_step("label_merge_state", has_conflicts=True, needs_rebase=False) + return # Exit early - conflicts take precedence + + # Step 2: No conflicts - remove has-conflicts label if present + if has_conflicts_label_exists: + self.logger.debug(f"{self.log_prefix} Removing {HAS_CONFLICTS_LABEL_STR} label") + await self.labels_handler._remove_label(pull_request=pull_request, label=HAS_CONFLICTS_LABEL_STR) + + # Step 3: Check if needs rebase via Compare API + base_ref, head_user_login, head_ref = await asyncio.gather( + asyncio.to_thread(lambda: pull_request.base.ref), + asyncio.to_thread(lambda: pull_request.head.user.login), + asyncio.to_thread(lambda: pull_request.head.ref), + ) + head_ref_full = f"{head_user_login}:{head_ref}" - compare_data = await self._compare_branches(base_ref=base_ref, head_ref_full=head_ref_full) - if compare_data is None: - self.logger.warning(f"{self.log_prefix} Compare API failed, skipping rebase label update") - if self.ctx: - self.ctx.complete_step("label_merge_state", compare_api_failed=True) - return + compare_data = await self._compare_branches(base_ref=base_ref, head_ref_full=head_ref_full) + if compare_data is None: + self.logger.warning(f"{self.log_prefix} Compare API failed, skipping rebase label update") + if self.ctx: + self.ctx.complete_step("label_merge_state", compare_api_failed=True) + return - behind_by = compare_data.get("behind_by", 0) - status = compare_data.get("status", "") + behind_by = compare_data.get("behind_by", 0) + status = compare_data.get("status", "") - needs_rebase = behind_by > 0 or status == "diverged" + needs_rebase = behind_by > 0 or status == "diverged" - self.logger.debug( - f"{self.log_prefix} Compare API - behind_by: {behind_by}, status: {status}, needs_rebase: {needs_rebase}" - ) + self.logger.debug( + f"{self.log_prefix} Compare API - behind_by: {behind_by}, " + f"status: {status}, needs_rebase: {needs_rebase}" + ) - # Step 4: Update needs-rebase label - if needs_rebase and not needs_rebase_label_exists: - self.logger.debug(f"{self.log_prefix} Adding {NEEDS_REBASE_LABEL_STR} label") - await self.labels_handler._add_label(pull_request=pull_request, label=NEEDS_REBASE_LABEL_STR) - elif not needs_rebase and needs_rebase_label_exists: - self.logger.debug(f"{self.log_prefix} Removing {NEEDS_REBASE_LABEL_STR} label") - await self.labels_handler._remove_label(pull_request=pull_request, label=NEEDS_REBASE_LABEL_STR) + # Step 4: Update needs-rebase label + if needs_rebase and not needs_rebase_label_exists: + self.logger.debug(f"{self.log_prefix} Adding {NEEDS_REBASE_LABEL_STR} label") + await self.labels_handler._add_label(pull_request=pull_request, label=NEEDS_REBASE_LABEL_STR) + elif not needs_rebase and needs_rebase_label_exists: + self.logger.debug(f"{self.log_prefix} Removing {NEEDS_REBASE_LABEL_STR} label") + await self.labels_handler._remove_label(pull_request=pull_request, label=NEEDS_REBASE_LABEL_STR) - if self.ctx: - self.ctx.complete_step("label_merge_state", has_conflicts=False, needs_rebase=needs_rebase) + if self.ctx: + self.ctx.complete_step("label_merge_state", has_conflicts=False, needs_rebase=needs_rebase) + + except asyncio.CancelledError: + self.logger.debug(f"{self.log_prefix} Label merge state check cancelled") + raise + except Exception as ex: + self.logger.exception(f"{self.log_prefix} Failed to label merge state") + if self.ctx: + self.ctx.fail_step("label_merge_state", ex, traceback.format_exc()) + raise async def _process_verified_for_update_or_new_pull_request(self, pull_request: PullRequest) -> None: if not self.github_webhook.verified_job: