From b066f2bd43afdb664e4110e684aa1cb4058a239b Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 26 Feb 2026 15:06:49 +0200 Subject: [PATCH 1/5] fix: prevent has-conflicts label flapping when mergeable is None MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When GitHub is still computing merge status, pull_request.mergeable returns None. Previously this was treated as "no conflicts", causing the has-conflicts label to be incorrectly removed and then re-added once GitHub finished computing — resulting in label flapping. Now polls mergeable using TimeoutSampler (wait_timeout=15, sleep=3) wrapped in asyncio.to_thread() until GitHub returns a definitive True/False. Only skips the label update if still None after retries. Closes #999 --- .../libs/handlers/pull_request_handler.py | 30 ++++++++++ .../tests/test_pull_request_handler.py | 60 ++++++++++++++----- 2 files changed, 74 insertions(+), 16 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index a26eb4e7..17462516 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -8,6 +8,7 @@ from github import GithubException from github.PullRequest import PullRequest from github.Repository import Repository +from timeout_sampler import TimeoutSampler from webhook_server.libs.handlers.check_run_handler import CheckRunHandler, CheckRunOutput from webhook_server.libs.handlers.labels_handler import LabelsHandler @@ -963,7 +964,36 @@ async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> needs_rebase_label_exists = NEEDS_REBASE_LABEL_STR in current_labels # Step 1: Check for conflicts first + # GitHub may return mergeable=None while computing - poll until definitive mergeable = await asyncio.to_thread(lambda: pull_request.mergeable) + + if mergeable is None: + self.logger.debug( + f"{self.log_prefix} PR mergeable status is None, polling until GitHub computes status" + ) + pr_number = pull_request.number + repository = self.github_webhook.repository + + def _poll_mergeable() -> bool | None: + for sample in TimeoutSampler( + wait_timeout=15, + sleep=3, + func=lambda: repository.get_pull(pr_number).mergeable, + ): + if sample is not None: + return sample + return None # pragma: no cover + + try: + mergeable = await asyncio.to_thread(_poll_mergeable) + except Exception: + self.logger.warning( + f"{self.log_prefix} PR mergeable status still None after retries, skipping label update" + ) + if self.ctx: + self.ctx.complete_step("label_merge_state", mergeable_unknown=True) + return + has_conflicts = mergeable is False if has_conflicts: diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 7c34d1e1..deb66726 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -2322,22 +2322,13 @@ 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=None. + """Test label_pull_request_by_merge_state when mergeable=None after retries. - When mergeable=None (not yet computed), has_conflicts is False. - If Compare API shows behind_by > 0, needs-rebase label should be added. + When mergeable=None (not yet computed by GitHub) and TimeoutSampler + times out, the method should return without adding or removing any labels. """ 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" - - # Mock existing labels - PR has no labels currently - mock_pull_request.labels = [] - - # 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)) + mock_pull_request.number = 123 with ( patch.object( @@ -2345,11 +2336,48 @@ async def test_label_pull_request_by_merge_state_unknown( "pull_request_labels_names", new=AsyncMock(return_value=[]), ), - patch.object(pull_request_handler.labels_handler, "_add_label", new_callable=AsyncMock) as mock_add_label, + patch.object(pull_request_handler.labels_handler, "_add_label", new=AsyncMock()) as mock_add_label, + patch.object(pull_request_handler.labels_handler, "_remove_label", new=AsyncMock()) as mock_remove_label, + patch( + "webhook_server.libs.handlers.pull_request_handler.TimeoutSampler", + side_effect=TimeoutError("Timed out"), + ), ): 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) + # Neither add nor remove should be called when mergeable is None + mock_add_label.assert_not_awaited() + mock_remove_label.assert_not_awaited() + + @pytest.mark.asyncio + async def test_label_pull_request_by_merge_state_mergeable_none_with_existing_conflicts_label( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that has-conflicts label is NOT removed when mergeable is None after retries. + + When mergeable=None (GitHub still computing) and has-conflicts label + already exists, the label must be preserved (not incorrectly removed) + even after TimeoutSampler times out. + """ + mock_pull_request.mergeable = None # Not yet computed by GitHub + mock_pull_request.number = 123 + + with ( + patch.object( + pull_request_handler.labels_handler, + "pull_request_labels_names", + new=AsyncMock(return_value=[HAS_CONFLICTS_LABEL_STR]), + ), + patch.object(pull_request_handler.labels_handler, "_add_label", new=AsyncMock()) as mock_add_label, + patch.object(pull_request_handler.labels_handler, "_remove_label", new=AsyncMock()) as mock_remove_label, + patch( + "webhook_server.libs.handlers.pull_request_handler.TimeoutSampler", + side_effect=TimeoutError("Timed out"), + ), + ): + await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) + # Neither add nor remove should be called when mergeable is None + mock_add_label.assert_not_awaited() + mock_remove_label.assert_not_awaited() @pytest.mark.asyncio async def test_label_pull_request_by_merge_state_diverged( From e6c64a4a0fbd98f24567bdbb8777f6d6e957ee1c Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 26 Feb 2026 15:27:53 +0200 Subject: [PATCH 2/5] fix: improve exception handling and add success-path polling tests - Catch TimeoutExpiredError specifically instead of broad Exception - Re-raise asyncio.CancelledError to preserve task cancellation - Use logger.exception() for unexpected errors (includes traceback) - Add post-poll None guard to prevent label flapping on edge cases - Add tests for polling resolving to True (mergeable) and False (conflicts) --- .../libs/handlers/pull_request_handler.py | 19 +++++- .../tests/test_pull_request_handler.py | 66 ++++++++++++++++++- 2 files changed, 81 insertions(+), 4 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 17462516..aae8d91f 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -8,7 +8,7 @@ from github import GithubException from github.PullRequest import PullRequest from github.Repository import Repository -from timeout_sampler import TimeoutSampler +from timeout_sampler import TimeoutExpiredError, TimeoutSampler from webhook_server.libs.handlers.check_run_handler import CheckRunHandler, CheckRunOutput from webhook_server.libs.handlers.labels_handler import LabelsHandler @@ -986,13 +986,28 @@ def _poll_mergeable() -> bool | None: try: mergeable = await asyncio.to_thread(_poll_mergeable) - except Exception: + except asyncio.CancelledError: + raise + except TimeoutExpiredError: self.logger.warning( f"{self.log_prefix} PR mergeable status still None after retries, skipping label update" ) if self.ctx: self.ctx.complete_step("label_merge_state", mergeable_unknown=True) return + except Exception as ex: + self.logger.exception(f"{self.log_prefix} Unexpected error polling PR mergeable status") + if self.ctx: + self.ctx.fail_step("label_merge_state", exception=ex, traceback_str=traceback.format_exc()) + raise + + if mergeable is None: + self.logger.warning( + f"{self.log_prefix} PR mergeable status still None after polling, skipping label update" + ) + if self.ctx: + self.ctx.complete_step("label_merge_state", mergeable_unknown=True) + return has_conflicts = mergeable is False diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index deb66726..ea5cfe8c 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -8,6 +8,7 @@ import pytest from github import GithubException from github.PullRequest import PullRequest +from timeout_sampler import TimeoutExpiredError from webhook_server.libs.github_api import GithubWebhook from webhook_server.libs.handlers.owners_files_handler import OwnersFileHandler @@ -2340,7 +2341,7 @@ async def test_label_pull_request_by_merge_state_unknown( patch.object(pull_request_handler.labels_handler, "_remove_label", new=AsyncMock()) as mock_remove_label, patch( "webhook_server.libs.handlers.pull_request_handler.TimeoutSampler", - side_effect=TimeoutError("Timed out"), + side_effect=TimeoutExpiredError("Timed out"), ), ): await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) @@ -2371,7 +2372,7 @@ async def test_label_pull_request_by_merge_state_mergeable_none_with_existing_co patch.object(pull_request_handler.labels_handler, "_remove_label", new=AsyncMock()) as mock_remove_label, patch( "webhook_server.libs.handlers.pull_request_handler.TimeoutSampler", - side_effect=TimeoutError("Timed out"), + side_effect=TimeoutExpiredError("Timed out"), ), ): await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) @@ -2379,6 +2380,67 @@ async def test_label_pull_request_by_merge_state_mergeable_none_with_existing_co mock_add_label.assert_not_awaited() mock_remove_label.assert_not_awaited() + @pytest.mark.asyncio + async def test_label_pull_request_by_merge_state_polling_resolves_to_conflicts( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that polling resolves mergeable=False correctly adds has-conflicts label.""" + mock_pull_request.mergeable = None # Triggers polling + mock_pull_request.number = 123 + + 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=AsyncMock()) as mock_add, + patch.object(pull_request_handler.labels_handler, "_remove_label", new=AsyncMock()) as mock_remove, + patch( + "webhook_server.libs.handlers.pull_request_handler.TimeoutSampler", + return_value=iter([False]), + ), + ): + await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) + # has-conflicts should be added (mergeable=False means conflicts) + mock_add.assert_awaited_once_with(pull_request=mock_pull_request, label=HAS_CONFLICTS_LABEL_STR) + mock_remove.assert_not_awaited() + + @pytest.mark.asyncio + async def test_label_pull_request_by_merge_state_polling_resolves_to_mergeable( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that polling resolves mergeable=True correctly removes has-conflicts label.""" + mock_pull_request.mergeable = None # Triggers polling + mock_pull_request.number = 123 + mock_pull_request.base.ref = "main" + mock_pull_request.head.user.login = "test-user" + mock_pull_request.head.ref = "feature-branch" + + with ( + patch.object( + pull_request_handler.labels_handler, + "pull_request_labels_names", + new=AsyncMock(return_value=[HAS_CONFLICTS_LABEL_STR]), + ), + patch.object(pull_request_handler.labels_handler, "_add_label", new=AsyncMock()) as mock_add, + patch.object(pull_request_handler.labels_handler, "_remove_label", new=AsyncMock()) as mock_remove, + patch( + "webhook_server.libs.handlers.pull_request_handler.TimeoutSampler", + return_value=iter([True]), + ), + patch.object( + pull_request_handler, + "_compare_branches", + new=AsyncMock(return_value={"behind_by": 0, "status": "identical"}), + ), + ): + await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) + # has-conflicts should be removed (mergeable=True means no conflicts) + mock_remove.assert_awaited_once_with(pull_request=mock_pull_request, label=HAS_CONFLICTS_LABEL_STR) + # No labels should be added (no conflicts, no rebase needed) + mock_add.assert_not_awaited() + @pytest.mark.asyncio async def test_label_pull_request_by_merge_state_diverged( self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock From 67f229f9a6448d427b06586ad224f2a4a179398a Mon Sep 17 00:00:00 2001 From: rnetser Date: Sun, 1 Mar 2026 13:52:56 +0200 Subject: [PATCH 3/5] fix: remove duplicate fail_step call in mergeable polling The inner except block's ctx.fail_step() duplicated the outer method-level handler's failure recording. Let the outer handler own step-failure recording for a single source of truth. --- webhook_server/libs/handlers/pull_request_handler.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 0cae48cf..c6456e64 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -1021,10 +1021,8 @@ def _poll_mergeable() -> bool | None: if self.ctx: self.ctx.complete_step("label_merge_state", mergeable_unknown=True) return - except Exception as ex: + except Exception: self.logger.exception(f"{self.log_prefix} Unexpected error polling PR mergeable status") - if self.ctx: - self.ctx.fail_step("label_merge_state", exception=ex, traceback_str=traceback.format_exc()) raise if mergeable is None: From 73e088945516d544edc4f6332864ee1a24fa7b05 Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 3 Mar 2026 22:59:09 +0200 Subject: [PATCH 4/5] fix: prevent has-conflicts label flapping with mergeable polling Poll mergeable status using TimeoutSampler (30s timeout, 5s interval) when GitHub returns None. Guard has-conflicts update with mergeable None check to prevent removing label on unknown state. Exit early when conflicts confirmed (skip needs-rebase check). --- .../libs/handlers/pull_request_handler.py | 52 +++++++---------- .../tests/test_pull_request_handler.py | 57 ++++++++++--------- 2 files changed, 52 insertions(+), 57 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index ac62af9d..4611d9d1 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -983,8 +983,9 @@ 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, exit - 3. Else → remove has-conflicts, check Compare API for rebase status + 2. If has conflicts → add has-conflicts label, exit + 3. If mergeable unknown → skip has-conflicts update + 4. If no conflicts → 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) @@ -1017,8 +1018,8 @@ async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> def _poll_mergeable() -> bool | None: for sample in TimeoutSampler( - wait_timeout=15, - sleep=3, + wait_timeout=30, + sleep=5, func=lambda: repository.get_pull(pr_number).mergeable, ): if sample is not None: @@ -1035,37 +1036,28 @@ def _poll_mergeable() -> bool | None: ) if self.ctx: self.ctx.complete_step("label_merge_state", mergeable_unknown=True) - return - except Exception: - self.logger.exception(f"{self.log_prefix} Unexpected error polling PR mergeable status") - raise - - if mergeable is None: - self.logger.warning( - f"{self.log_prefix} PR mergeable status still None after polling, skipping label update" - ) - if self.ctx: - self.ctx.complete_step("label_merge_state", mergeable_unknown=True) - return - has_conflicts = mergeable is False + if mergeable is not None: + 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=}") + 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 + if self.ctx: + self.ctx.complete_step("label_merge_state", has_conflicts=True) + 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 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) + else: + self.logger.debug(f"{self.log_prefix} Mergeable status unknown, skipping has-conflicts label update") # Step 3: Check if needs rebase via Compare API base_ref, head_user_login, head_ref = await asyncio.gather( diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 56d0eed5..3d4582f6 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -751,20 +751,14 @@ async def test_label_pull_request_by_merge_state_has_conflicts( """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). + When mergeable is False, has-conflicts label is set and method returns early + without checking Compare API for 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 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, @@ -774,7 +768,7 @@ async def test_label_pull_request_by_merge_state_has_conflicts( 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) + # When mergeable is False, has-conflicts label is set and method exits early mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=HAS_CONFLICTS_LABEL_STR) @pytest.mark.asyncio @@ -2447,10 +2441,18 @@ async def test_label_pull_request_by_merge_state_unknown( """Test label_pull_request_by_merge_state when mergeable=None after retries. When mergeable=None (not yet computed by GitHub) and TimeoutSampler - times out, the method should return without adding or removing any labels. + times out, the has-conflicts label is left unchanged but the + needs-rebase check still runs via Compare API. """ mock_pull_request.mergeable = None # Not yet computed by GitHub mock_pull_request.number = 123 + 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 (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( @@ -2466,7 +2468,7 @@ async def test_label_pull_request_by_merge_state_unknown( ), ): await pull_request_handler.label_pull_request_by_merge_state(mock_pull_request) - # Neither add nor remove should be called when mergeable is None + # has-conflicts label unchanged (mergeable is None), no rebase needed mock_add_label.assert_not_awaited() mock_remove_label.assert_not_awaited() @@ -2478,10 +2480,17 @@ async def test_label_pull_request_by_merge_state_mergeable_none_with_existing_co When mergeable=None (GitHub still computing) and has-conflicts label already exists, the label must be preserved (not incorrectly removed) - even after TimeoutSampler times out. + even after TimeoutSampler times out. The needs-rebase check still runs. """ mock_pull_request.mergeable = None # Not yet computed by GitHub mock_pull_request.number = 123 + 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 (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( @@ -2497,7 +2506,7 @@ async def test_label_pull_request_by_merge_state_mergeable_none_with_existing_co ), ): await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) - # Neither add nor remove should be called when mergeable is None + # has-conflicts label preserved (mergeable is None), no rebase needed mock_add_label.assert_not_awaited() mock_remove_label.assert_not_awaited() @@ -2505,7 +2514,10 @@ async def test_label_pull_request_by_merge_state_mergeable_none_with_existing_co async def test_label_pull_request_by_merge_state_polling_resolves_to_conflicts( self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock ) -> None: - """Test that polling resolves mergeable=False correctly adds has-conflicts label.""" + """Test that polling resolves mergeable=False correctly adds has-conflicts label. + + After adding has-conflicts, the method returns early without checking Compare API. + """ mock_pull_request.mergeable = None # Triggers polling mock_pull_request.number = 123 @@ -2516,16 +2528,14 @@ async def test_label_pull_request_by_merge_state_polling_resolves_to_conflicts( new=AsyncMock(return_value=[]), ), patch.object(pull_request_handler.labels_handler, "_add_label", new=AsyncMock()) as mock_add, - patch.object(pull_request_handler.labels_handler, "_remove_label", new=AsyncMock()) as mock_remove, patch( "webhook_server.libs.handlers.pull_request_handler.TimeoutSampler", return_value=iter([False]), ), ): await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request) - # has-conflicts should be added (mergeable=False means conflicts) + # has-conflicts should be added (mergeable=False means conflicts), then early return mock_add.assert_awaited_once_with(pull_request=mock_pull_request, label=HAS_CONFLICTS_LABEL_STR) - mock_remove.assert_not_awaited() @pytest.mark.asyncio async def test_label_pull_request_by_merge_state_polling_resolves_to_mergeable( @@ -2635,21 +2645,14 @@ async def test_label_pull_request_by_merge_state_behind_and_conflicts( """Test labeling pull request when behind and has conflicts. 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). + When conflicts are detected, has-conflicts label is set and method returns early + without checking Compare API for 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 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, @@ -2659,7 +2662,7 @@ async def test_label_pull_request_by_merge_state_behind_and_conflicts( 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) + # Only has-conflicts label is set; method returns early without checking Compare API mock_add_label.assert_called_once_with(pull_request=mock_pull_request, label=HAS_CONFLICTS_LABEL_STR) @pytest.mark.asyncio From e319c812078449a688d418ae1b1ea4684acfafcd Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 10 Mar 2026 21:04:17 +0200 Subject: [PATCH 5/5] fix: prevent label flapping by using add_only mode after merges When checking all open PRs after a merge, use add_only=True to only ADD has-conflicts label, never remove it. GitHub can return stale mergeable data right after base branch changes, causing incorrect label removal and flapping. Label removal only happens when the PR itself is updated (opened/synchronized). needs-rebase removal is unaffected as Compare API data is reliable. --- .../libs/handlers/pull_request_handler.py | 18 ++++-- .../tests/test_pull_request_handler.py | 64 +++++++++++++++++++ 2 files changed, 75 insertions(+), 7 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 4611d9d1..eeebd32a 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -563,7 +563,7 @@ async def label_all_opened_pull_requests_merge_state_after_merged(self) -> None: pulls = await asyncio.to_thread(lambda: list(self.repository.get_pulls(state="open"))) for pull_request in pulls: self.logger.info(f"{self.log_prefix} check label pull request after merge") - await self.label_pull_request_by_merge_state(pull_request=pull_request) + await self.label_pull_request_by_merge_state(pull_request=pull_request, add_only=True) async def delete_remote_tag_for_merged_or_closed_pr(self, pull_request: PullRequest) -> None: self.logger.debug(f"{self.log_prefix} Checking if need to delete remote tag for {pull_request.number}") @@ -978,14 +978,16 @@ async def _compare_branches(self, base_ref: str, head_ref_full: str) -> dict[str self.logger.exception(f"{self.log_prefix} Unexpected error calling Compare API") return None - async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> None: + async def label_pull_request_by_merge_state(self, pull_request: PullRequest, add_only: bool = False) -> None: """Label pull request based on merge state. - Simple flow: + Flow: 1. Check pull_request.mergeable for conflicts 2. If has conflicts → add has-conflicts label, exit 3. If mergeable unknown → skip has-conflicts update - 4. If no conflicts → remove has-conflicts, check Compare API for rebase status + 4. If no conflicts and not add_only → remove has-conflicts + 5. Check Compare API for rebase status + 6. Update needs-rebase label (add only when add_only=True) Uses both GitHub APIs for accurate labeling: - has-conflicts: pull_request.mergeable == False (true merge conflict detection) @@ -995,6 +997,8 @@ async def label_pull_request_by_merge_state(self, pull_request: PullRequest) -> Args: pull_request: The GitHub pull request object to label. + add_only: When True, only add labels, never remove them. Used when + checking all open PRs after a merge, where GitHub data may be stale. """ if self.ctx: self.ctx.start_step("label_merge_state") @@ -1052,8 +1056,8 @@ def _poll_mergeable() -> bool | None: self.ctx.complete_step("label_merge_state", has_conflicts=True) return # Exit early - conflicts take precedence - # Step 2: No conflicts - remove has-conflicts label if present - if has_conflicts_label_exists: + # No conflicts - remove has-conflicts label if present (skip in add_only mode) + if has_conflicts_label_exists and not add_only: 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) else: @@ -1088,7 +1092,7 @@ def _poll_mergeable() -> bool | None: 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: + elif not needs_rebase and needs_rebase_label_exists and not add_only: 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) diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 3d4582f6..9ecfadab 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -579,6 +579,8 @@ async def test_label_all_opened_pull_requests_merge_state_after_merged( with patch.object(pull_request_handler, "label_pull_request_by_merge_state", new=AsyncMock()) as mock_label: with patch("asyncio.sleep", new=AsyncMock()): await pull_request_handler.label_all_opened_pull_requests_merge_state_after_merged() + mock_label.assert_any_await(pull_request=mock_pr1, add_only=True) + mock_label.assert_any_await(pull_request=mock_pr2, add_only=True) assert mock_label.await_count == 2 @pytest.mark.asyncio @@ -2769,6 +2771,68 @@ async def test_label_pull_request_by_merge_state_incomplete_compare_data( pull_request=mock_pull_request, label=HAS_CONFLICTS_LABEL_STR ) + @pytest.mark.asyncio + async def test_label_merge_state_add_only_does_not_remove_has_conflicts( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that add_only=True prevents removal of has-conflicts label. + + When mergeable=True and has-conflicts label exists, normal mode removes it. + With add_only=True, the label should be preserved (stale data protection). + """ + 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 - up-to-date (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, + "pull_request_labels_names", + new=AsyncMock(return_value=[HAS_CONFLICTS_LABEL_STR]), + ), + patch.object(pull_request_handler.labels_handler, "_remove_label", new=AsyncMock()) as mock_remove_label, + patch.object(pull_request_handler.labels_handler, "_add_label", new=AsyncMock()) as mock_add_label, + ): + await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request, add_only=True) + mock_remove_label.assert_not_awaited() + mock_add_label.assert_not_awaited() + + @pytest.mark.asyncio + async def test_label_merge_state_add_only_does_not_remove_needs_rebase( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that add_only=True prevents removal of needs-rebase label. + + When Compare API shows no rebase needed and needs-rebase label exists, + normal mode removes it. With add_only=True, the label should be preserved. + """ + 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 - up-to-date (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, + "pull_request_labels_names", + new=AsyncMock(return_value=[NEEDS_REBASE_LABEL_STR]), + ), + patch.object(pull_request_handler.labels_handler, "_remove_label", new=AsyncMock()) as mock_remove_label, + patch.object(pull_request_handler.labels_handler, "_add_label", new=AsyncMock()) as mock_add_label, + ): + await pull_request_handler.label_pull_request_by_merge_state(pull_request=mock_pull_request, add_only=True) + mock_remove_label.assert_not_awaited() + mock_add_label.assert_not_awaited() + @pytest.mark.asyncio async def test_regenerate_welcome_message_existing_comment_updated( self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock