From 0668b43c92521f50b74daf01338df10eb37275b8 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Wed, 25 Mar 2026 00:04:25 +0200 Subject: [PATCH] fix: prevent duplicate cherry-pick PRs via label check Skip cherry-pick for branches whose cherry-pick label already exists on the PR. Posts comment telling user to remove the label to re-trigger. Works for both merged and unmerged PRs. Closes #1054 --- .../libs/handlers/issue_comment_handler.py | 79 +++++-- .../tests/test_issue_comment_handler.py | 198 +++++++++++++++++- 2 files changed, 253 insertions(+), 24 deletions(-) diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index e873803e..1b67d02f 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -391,7 +391,7 @@ async def process_cherry_pick_command( """Process cherry-pick command for pull requests. This method handles cherry-pick requests for both unmerged and merged PRs. - Cherry-pick labels (cherry-pick/) are added in BOTH scenarios: + Cherry-pick labels (cherry-pick-) are added in BOTH scenarios: **Unmerged PRs:** - Labels indicate which branches need cherry-picking after the PR is merged @@ -411,12 +411,17 @@ async def process_cherry_pick_command( Example: # Unmerged PR: /cherry-pick v1.0 v2.0 - # - Adds labels: cherry-pick/v1.0, cherry-pick/v2.0 + # - Adds labels: cherry-pick-v1.0, cherry-pick-v2.0 # - Posts comment explaining labels will trigger auto cherry-pick on merge # Merged PR: /cherry-pick v1.0 v2.0 + # - Adds labels: cherry-pick-v1.0, cherry-pick-v2.0 as idempotency markers # - Executes cherry-pick to v1.0 and v2.0 immediately - # - Adds labels: cherry-pick/v1.0, cherry-pick/v2.0 to track completion + # - Remove the label to retry a failed cherry-pick + + Duplicate prevention: + If a cherry-pick label already exists for a target branch, that branch is + skipped. To re-trigger a cherry-pick, remove the label and run the command again. """ _target_branches: list[str] = command_args.split() _exits_target_branches: set[str] = set() @@ -438,29 +443,63 @@ async def process_cherry_pick_command( self.logger.info(f"{self.log_prefix} {_non_exits_target_branches_msg}") await asyncio.to_thread(pull_request.create_issue_comment, _non_exits_target_branches_msg) - cp_labels: list[str] = [ - f"{CHERRY_PICK_LABEL_PREFIX}{_target_branch}" for _target_branch in _exits_target_branches - ] + if not _exits_target_branches: + return + + # Filter out branches that already have cherry-pick labels + existing_labels = {label.name for label in await asyncio.to_thread(lambda: list(pull_request.labels))} + _already_cherry_picked: list[str] = [] + _branches_to_process: set[str] = set() + + for _branch in _exits_target_branches: + label_name = f"{CHERRY_PICK_LABEL_PREFIX}{_branch}" + if label_name in existing_labels: + _already_cherry_picked.append(_branch) + else: + _branches_to_process.add(_branch) + + if _already_cherry_picked: + already_msg = ", ".join(f"`{b}`" for b in _already_cherry_picked) + await asyncio.to_thread( + pull_request.create_issue_comment, + f"Cherry-pick label already present for: {already_msg}\n" + "To re-trigger, remove the cherry-pick label(s) and run the command again.", + ) + + if not _branches_to_process: + return - if _exits_target_branches: - if not self.hook_data["issue"].get("pull_request", {}).get("merged_at"): - info_msg: str = f""" + cp_labels: list[str] = [f"{CHERRY_PICK_LABEL_PREFIX}{_branch}" for _branch in _branches_to_process] + + if not self.hook_data["issue"].get("pull_request", {}).get("merged_at"): + info_msg: str = f""" Cherry-pick requested for PR: `{pull_request.title}` by user `{reviewed_user}` -Adding label/s `{" ".join([_cp_label for _cp_label in cp_labels])}` for automatic cheery-pick once the PR is merged +Adding label/s `{" ".join(cp_labels)}` for automatic cherry-pick once the PR is merged """ - self.logger.info(f"{self.log_prefix} {info_msg}") - await asyncio.to_thread(pull_request.create_issue_comment, info_msg) - else: - for _exits_target_branch in _exits_target_branches: - await self.runner_handler.cherry_pick( - pull_request=pull_request, - target_branch=_exits_target_branch, - assign_to_pr_owner=self.github_webhook.cherry_pick_assign_to_pr_author, + self.logger.info(f"{self.log_prefix} {info_msg}") + await asyncio.to_thread(pull_request.create_issue_comment, info_msg) + else: + for _branch in _branches_to_process: + label_added = await self.labels_handler._add_label( + pull_request=pull_request, + label=f"{CHERRY_PICK_LABEL_PREFIX}{_branch}", + ) + if not label_added: + self.logger.info( + f"{self.log_prefix} Skipping cherry-pick for {_branch}," + " label already exists or could not be added" ) + continue + await self.runner_handler.cherry_pick( + pull_request=pull_request, + target_branch=_branch, + assign_to_pr_owner=self.github_webhook.cherry_pick_assign_to_pr_author, + ) + return - for _cp_label in cp_labels: - await self.labels_handler._add_label(pull_request=pull_request, label=_cp_label) + for _cp_label in cp_labels: + await self.labels_handler._add_label(pull_request=pull_request, label=_cp_label) async def process_retest_command( self, pull_request: PullRequest, command_args: str, reviewed_user: str, automerge: bool = False diff --git a/webhook_server/tests/test_issue_comment_handler.py b/webhook_server/tests/test_issue_comment_handler.py index e70c90fa..9e116a71 100644 --- a/webhook_server/tests/test_issue_comment_handler.py +++ b/webhook_server/tests/test_issue_comment_handler.py @@ -8,6 +8,7 @@ from webhook_server.utils.constants import ( APPROVE_STR, BUILD_AND_PUSH_CONTAINER_STR, + CHERRY_PICK_LABEL_PREFIX, COMMAND_ASSIGN_REVIEWER_STR, COMMAND_ASSIGN_REVIEWERS_STR, COMMAND_CHECK_CAN_MERGE_STR, @@ -794,6 +795,7 @@ async def test_process_cherry_pick_command_existing_branches( """Test processing cherry pick command with existing branches.""" mock_pull_request = Mock() mock_pull_request.title = "Test PR" + mock_pull_request.labels = [] # Patch is_merged as a method with patch.object(mock_pull_request, "is_merged", new=Mock(return_value=False)): with patch.object(issue_comment_handler.repository, "get_branch") as mock_get_branch: @@ -820,6 +822,7 @@ async def test_process_cherry_pick_command_non_existing_branches( ) -> None: """Test processing cherry pick command with non-existing branches.""" mock_pull_request = Mock() + mock_pull_request.labels = [] with patch.object(issue_comment_handler.repository, "get_branch", side_effect=Exception("Branch not found")): with patch.object(mock_pull_request, "create_issue_comment") as mock_comment: @@ -834,6 +837,7 @@ async def test_process_cherry_pick_command_non_existing_branches( async def test_process_cherry_pick_command_merged_pr(self, issue_comment_handler: IssueCommentHandler) -> None: """Test processing cherry pick command for merged PR.""" mock_pull_request = Mock() + mock_pull_request.labels = [] # Set merged_at in hook_data to simulate a merged PR at comment time issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2026-01-01T00:00:00Z"} with patch.object(issue_comment_handler.repository, "get_branch"): @@ -846,6 +850,7 @@ async def test_process_cherry_pick_command_merged_pr(self, issue_comment_handler issue_comment_handler.labels_handler, "_add_label", new_callable=AsyncMock, + return_value=True, ) as mock_add_label: await issue_comment_handler.process_cherry_pick_command( pull_request=mock_pull_request, @@ -859,7 +864,7 @@ async def test_process_cherry_pick_command_merged_pr(self, issue_comment_handler ) mock_add_label.assert_called_once_with( pull_request=mock_pull_request, - label="cherry-pick-branch1", + label=f"{CHERRY_PICK_LABEL_PREFIX}branch1", ) @pytest.mark.asyncio @@ -875,6 +880,7 @@ async def test_process_cherry_pick_command_merged_pr_multiple_branches( """ mock_pull_request = Mock() mock_pull_request.title = "Test PR" + mock_pull_request.labels = [] # Set merged_at in hook_data to simulate a merged PR at comment time issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2026-01-01T00:00:00Z"} @@ -888,6 +894,7 @@ async def test_process_cherry_pick_command_merged_pr_multiple_branches( issue_comment_handler.labels_handler, "_add_label", new_callable=AsyncMock, + return_value=True, ) as mock_add_label: # Execute cherry-pick command with multiple branches await issue_comment_handler.process_cherry_pick_command( @@ -916,9 +923,15 @@ async def test_process_cherry_pick_command_merged_pr_multiple_branches( # Verify labels were added exactly once for each branch (not duplicated) assert mock_add_label.call_count == 3 - mock_add_label.assert_any_call(pull_request=mock_pull_request, label="cherry-pick-branch1") - mock_add_label.assert_any_call(pull_request=mock_pull_request, label="cherry-pick-branch2") - mock_add_label.assert_any_call(pull_request=mock_pull_request, label="cherry-pick-branch3") + mock_add_label.assert_any_call( + pull_request=mock_pull_request, label=f"{CHERRY_PICK_LABEL_PREFIX}branch1" + ) + mock_add_label.assert_any_call( + pull_request=mock_pull_request, label=f"{CHERRY_PICK_LABEL_PREFIX}branch2" + ) + mock_add_label.assert_any_call( + pull_request=mock_pull_request, label=f"{CHERRY_PICK_LABEL_PREFIX}branch3" + ) @pytest.mark.asyncio async def test_process_cherry_pick_command_merged_pr_assign_disabled( @@ -927,6 +940,7 @@ async def test_process_cherry_pick_command_merged_pr_assign_disabled( """Test cherry-pick on merged PR passes assign_to_pr_owner=False when config disabled.""" issue_comment_handler.github_webhook.cherry_pick_assign_to_pr_author = False mock_pull_request = Mock() + mock_pull_request.labels = [] # Set merged_at in hook_data to simulate a merged PR at comment time issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2026-01-01T00:00:00Z"} with patch.object(issue_comment_handler.repository, "get_branch"): @@ -939,6 +953,7 @@ async def test_process_cherry_pick_command_merged_pr_assign_disabled( issue_comment_handler.labels_handler, "_add_label", new_callable=AsyncMock, + return_value=True, ): await issue_comment_handler.process_cherry_pick_command( pull_request=mock_pull_request, @@ -951,6 +966,181 @@ async def test_process_cherry_pick_command_merged_pr_assign_disabled( assign_to_pr_owner=False, ) + @pytest.mark.asyncio + async def test_process_cherry_pick_command_skips_already_cherry_picked( + self, issue_comment_handler: IssueCommentHandler + ) -> None: + """Test cherry-pick skips branches whose cherry-pick label already exists on the PR.""" + mock_pull_request = Mock() + mock_pull_request.title = "Test PR" + + # Simulate existing cherry-pick label for branch1 + existing_label = Mock() + existing_label.name = f"{CHERRY_PICK_LABEL_PREFIX}branch1" + mock_pull_request.labels = [existing_label] + + # Set merged_at in hook_data to simulate a merged PR at comment time + issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2026-01-01T00:00:00Z"} + with ( + patch.object(issue_comment_handler.repository, "get_branch"), + patch.object( + issue_comment_handler.runner_handler, + "cherry_pick", + new_callable=AsyncMock, + ) as mock_cherry_pick, + patch.object( + issue_comment_handler.labels_handler, + "_add_label", + new_callable=AsyncMock, + return_value=True, + ) as mock_add_label, + patch.object(mock_pull_request, "create_issue_comment") as mock_comment, + ): + await issue_comment_handler.process_cherry_pick_command( + pull_request=mock_pull_request, + command_args="branch1 branch2", + reviewed_user="test-user", + ) + + # branch1 should be skipped (label already exists), branch2 should proceed + mock_cherry_pick.assert_called_once_with( + pull_request=mock_pull_request, + target_branch="branch2", + assign_to_pr_owner=True, + ) + + # Only branch2 label should be added + mock_add_label.assert_called_once_with( + pull_request=mock_pull_request, + label=f"{CHERRY_PICK_LABEL_PREFIX}branch2", + ) + + # Verify "already present" comment was posted for branch1 + assert any( + "already present" in call.args[0] and "branch1" in call.args[0] for call in mock_comment.call_args_list + ), f"Expected 'already present' comment for branch1, got: {mock_comment.call_args_list}" + + @pytest.mark.asyncio + async def test_process_cherry_pick_command_all_already_cherry_picked( + self, issue_comment_handler: IssueCommentHandler + ) -> None: + """Test cherry-pick does nothing when all branches already have cherry-pick labels.""" + mock_pull_request = Mock() + mock_pull_request.title = "Test PR" + + # All branches already cherry-picked + label1 = Mock() + label1.name = f"{CHERRY_PICK_LABEL_PREFIX}branch1" + label2 = Mock() + label2.name = f"{CHERRY_PICK_LABEL_PREFIX}branch2" + mock_pull_request.labels = [label1, label2] + + issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2026-01-01T00:00:00Z"} + with ( + patch.object(issue_comment_handler.repository, "get_branch"), + patch.object( + issue_comment_handler.runner_handler, + "cherry_pick", + new_callable=AsyncMock, + ) as mock_cherry_pick, + patch.object( + issue_comment_handler.labels_handler, + "_add_label", + new_callable=AsyncMock, + ) as mock_add_label, + patch.object(mock_pull_request, "create_issue_comment") as mock_comment, + ): + await issue_comment_handler.process_cherry_pick_command( + pull_request=mock_pull_request, + command_args="branch1 branch2", + reviewed_user="test-user", + ) + + # No cherry-picks should be triggered + mock_cherry_pick.assert_not_called() + + # No labels should be added + mock_add_label.assert_not_called() + + # "already present" comment should be posted + assert any("already present" in call.args[0] for call in mock_comment.call_args_list), ( + f"Expected 'already present' comment, got: {mock_comment.call_args_list}" + ) + + @pytest.mark.asyncio + async def test_process_cherry_pick_command_skips_when_add_label_fails( + self, issue_comment_handler: IssueCommentHandler + ) -> None: + """Test cherry_pick is skipped when _add_label returns False (TOCTOU protection).""" + mock_pull_request = Mock() + mock_pull_request.title = "Test PR" + mock_pull_request.labels = [] # No existing labels in snapshot + + issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2024-01-01T00:00:00Z"} + + with ( + patch.object(issue_comment_handler.repository, "get_branch"), + # _add_label returns False (label was added by another webhook in the meantime) + patch.object( + issue_comment_handler.labels_handler, + "_add_label", + new_callable=AsyncMock, + return_value=False, + ), + patch.object( + issue_comment_handler.runner_handler, + "cherry_pick", + new_callable=AsyncMock, + ) as mock_cherry_pick, + ): + await issue_comment_handler.process_cherry_pick_command( + pull_request=mock_pull_request, + command_args="branch1", + reviewed_user="test-user", + ) + mock_cherry_pick.assert_not_called() + + @pytest.mark.asyncio + async def test_process_cherry_pick_command_skips_already_cherry_picked_unmerged( + self, issue_comment_handler: IssueCommentHandler + ) -> None: + """Test cherry-pick label check works for unmerged PRs too.""" + mock_pull_request = Mock() + mock_pull_request.title = "Test PR" + + # Simulate existing cherry-pick label for branch1 + existing_label = Mock() + existing_label.name = f"{CHERRY_PICK_LABEL_PREFIX}branch1" + mock_pull_request.labels = [existing_label] + + # Set merged_at to None to simulate an unmerged PR + issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": None} + with ( + patch.object(issue_comment_handler.repository, "get_branch"), + patch.object( + issue_comment_handler.labels_handler, + "_add_label", + new_callable=AsyncMock, + ) as mock_add_label, + patch.object(mock_pull_request, "create_issue_comment") as mock_comment, + ): + await issue_comment_handler.process_cherry_pick_command( + pull_request=mock_pull_request, + command_args="branch1 branch2", + reviewed_user="test-user", + ) + + # Verify "already present" comment was posted for branch1 + assert any( + "already present" in call.args[0] and "branch1" in call.args[0] for call in mock_comment.call_args_list + ), f"Expected 'already present' comment for branch1, got: {mock_comment.call_args_list}" + + # Only branch2 label should be added (not branch1) + mock_add_label.assert_called_once_with( + pull_request=mock_pull_request, + label=f"{CHERRY_PICK_LABEL_PREFIX}branch2", + ) + @pytest.mark.asyncio async def test_process_retest_command_no_target_tests(self, issue_comment_handler: IssueCommentHandler) -> None: """Test processing retest command with no target tests."""