From 67aecff833c1d6510ce7a1d4d12949c6c2dcb2af Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 11 Nov 2025 12:07:07 +0200 Subject: [PATCH] fix(cherry-pick): prevent label duplication on merged PRs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When cherry-picking to multiple branches on a merged PR, labels were being added N times (once per branch) inside the cherry-pick loop, resulting in N×N total label additions. Fixed by moving label addition outside the cherry-pick loop, ensuring labels are added exactly once per target branch. Also added: - Comprehensive test for multiple branches scenario - Docstring explaining why labels are added for both unmerged and merged PRs - Labels serve as audit trail for completed cherry-picks on merged PRs Test coverage: - test_process_cherry_pick_command_merged_pr_multiple_branches validates fix - All 5 cherry-pick tests passing --- .../libs/handlers/issue_comment_handler.py | 43 ++++++++++-- .../tests/test_issue_comment_handler.py | 65 +++++++++++++++++-- 2 files changed, 97 insertions(+), 11 deletions(-) diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index ee048e1b..e0b77dea 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -329,6 +329,36 @@ async def _add_reviewer_by_user_comment(self, pull_request: PullRequest, reviewe async def process_cherry_pick_command( self, pull_request: PullRequest, command_args: str, reviewed_user: str ) -> None: + """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: + + **Unmerged PRs:** + - Labels indicate which branches need cherry-picking after the PR is merged + - Labels act as a TODO list for automatic cherry-picking + - When the PR is merged, the PR handler detects these labels and triggers cherry-picks + + **Merged PRs:** + - Cherry-picks are executed immediately for all target branches + - Labels are added to track which branches have been cherry-picked to + - Labels serve as a historical record of completed cherry-pick operations + - Helps with auditing and tracking which releases include this change + + Args: + pull_request: The pull request to cherry-pick + command_args: Space-separated list of target branches (e.g., "v1.0 v2.0") + reviewed_user: User who requested the cherry-pick + + Example: + # Unmerged PR: /cherry-pick v1.0 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 + # - Executes cherry-pick to v1.0 and v2.0 immediately + # - Adds labels: cherry-pick/v1.0, cherry-pick/v2.0 to track completion + """ _target_branches: list[str] = command_args.split() _exits_target_branches: set[str] = set() _non_exits_target_branches_msg: str = "" @@ -349,19 +379,19 @@ 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 _exits_target_branches: if not await asyncio.to_thread(pull_request.is_merged): - cp_labels: list[str] = [ - f"{CHERRY_PICK_LABEL_PREFIX}{_target_branch}" for _target_branch in _exits_target_branches - ] 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 """ + self.logger.info(f"{self.log_prefix} {info_msg}") await asyncio.to_thread(pull_request.create_issue_comment, info_msg) - for _cp_label in cp_labels: - await self.labels_handler._add_label(pull_request=pull_request, label=_cp_label) else: for _exits_target_branch in _exits_target_branches: await self.runner_handler.cherry_pick( @@ -370,6 +400,9 @@ async def process_cherry_pick_command( reviewed_user=reviewed_user, ) + 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 ) -> None: diff --git a/webhook_server/tests/test_issue_comment_handler.py b/webhook_server/tests/test_issue_comment_handler.py index d6291b80..183cdd1f 100644 --- a/webhook_server/tests/test_issue_comment_handler.py +++ b/webhook_server/tests/test_issue_comment_handler.py @@ -656,12 +656,65 @@ async def test_process_cherry_pick_command_merged_pr(self, issue_comment_handler with 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_called_once_with( - pull_request=mock_pull_request, target_branch="branch1", reviewed_user="test-user" - ) + with patch.object( + issue_comment_handler.labels_handler, "_add_label", new_callable=AsyncMock + ) as mock_add_label: + await issue_comment_handler.process_cherry_pick_command( + pull_request=mock_pull_request, command_args="branch1", reviewed_user="test-user" + ) + mock_cherry_pick.assert_called_once_with( + pull_request=mock_pull_request, target_branch="branch1", reviewed_user="test-user" + ) + mock_add_label.assert_called_once_with( + pull_request=mock_pull_request, label="cherry-pick-branch1" + ) + + @pytest.mark.asyncio + async def test_process_cherry_pick_command_merged_pr_multiple_branches( + self, issue_comment_handler: IssueCommentHandler + ) -> None: + """Test processing cherry pick command for merged PR with multiple branches. + + This test verifies that when cherry-picking to multiple branches on a merged PR: + 1. cherry_pick is called for each target branch + 2. Labels are added exactly once for each branch (not duplicated) + """ + mock_pull_request = Mock() + mock_pull_request.title = "Test PR" + + # Patch is_merged to return True (merged PR) + with patch.object(mock_pull_request, "is_merged", new=Mock(return_value=True)): + with patch.object(issue_comment_handler.repository, "get_branch"): + with patch.object( + issue_comment_handler.runner_handler, "cherry_pick", new_callable=AsyncMock + ) as mock_cherry_pick: + with patch.object( + issue_comment_handler.labels_handler, "_add_label", new_callable=AsyncMock + ) as mock_add_label: + # Execute cherry-pick command with multiple branches + await issue_comment_handler.process_cherry_pick_command( + pull_request=mock_pull_request, + command_args="branch1 branch2 branch3", + reviewed_user="test-user", + ) + + # Verify cherry_pick was called for each branch + assert mock_cherry_pick.call_count == 3 + mock_cherry_pick.assert_any_call( + pull_request=mock_pull_request, target_branch="branch1", reviewed_user="test-user" + ) + mock_cherry_pick.assert_any_call( + pull_request=mock_pull_request, target_branch="branch2", reviewed_user="test-user" + ) + mock_cherry_pick.assert_any_call( + pull_request=mock_pull_request, target_branch="branch3", reviewed_user="test-user" + ) + + # 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") @pytest.mark.asyncio async def test_process_retest_command_no_target_tests(self, issue_comment_handler: IssueCommentHandler) -> None: