diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 3b1743bb..dba1eb44 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -22,6 +22,7 @@ CHANGED_REQUESTED_BY_LABEL_PREFIX, CHERRY_PICK_LABEL_PREFIX, CHERRY_PICKED_LABEL, + COMMAND_CHERRY_PICK_STR, COMMENTED_BY_LABEL_PREFIX, CONVENTIONAL_TITLE_STR, FAILURE_STR, @@ -126,11 +127,34 @@ async def process_pull_request_webhook_data(self, pull_request: PullRequest) -> self.logger.info(f"{self.log_prefix} PR is merged") labels = await asyncio.to_thread(lambda: list(pull_request.labels)) - for _label in labels: - _label_name = _label.name - if _label_name.startswith(CHERRY_PICK_LABEL_PREFIX): + if cherry_pick_labels := [ + _label for _label in labels if _label.name.startswith(CHERRY_PICK_LABEL_PREFIX) + ]: + comments = await asyncio.to_thread(lambda: list(pull_request.get_issue_comments())) + pr_author = await asyncio.to_thread(lambda: pull_request.user.login) + + for _label in cherry_pick_labels: + target_branch = _label.name.replace(CHERRY_PICK_LABEL_PREFIX, "") + + # Search comments (newest first) for /cherry-pick command matching this target branch + command_prefix = f"/{COMMAND_CHERRY_PICK_STR}" + requester = None + for comment in reversed(comments): + for line in comment.body.strip().splitlines(): + stripped = line.strip() + if stripped.startswith(command_prefix): + args = stripped[len(command_prefix) :].strip() + if args != "cancel" and target_branch in args.split(): + requester = comment.user.login + break + if requester is not None: + break + await self.runner_handler.cherry_pick( - pull_request=pull_request, target_branch=_label_name.replace(CHERRY_PICK_LABEL_PREFIX, "") + pull_request=pull_request, + target_branch=target_branch, + reviewed_user=requester or pr_author, + by_label=True, ) await self.runner_handler.run_build_container( diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index b0adb1fe..9ea21658 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -536,10 +536,29 @@ async def run_custom_check( async def is_branch_exists(self, branch: str) -> Branch: return await asyncio.to_thread(self.repository.get_branch, branch) - async def cherry_pick(self, pull_request: PullRequest, target_branch: str, reviewed_user: str = "") -> None: - requested_by = reviewed_user or "by target-branch label" + async def cherry_pick( + self, pull_request: PullRequest, target_branch: str, reviewed_user: str = "", *, by_label: bool = False + ) -> None: + """Cherry-pick a merged PR to a target branch. + + Args: + pull_request: The merged pull request to cherry-pick. + target_branch: Branch to cherry-pick into. + reviewed_user: The user who requested the cherry-pick via comment command, + or the PR author's login when triggered by label. Used for attribution only. + by_label: True when cherry-pick was triggered by a target-branch label + on PR merge, False when triggered by a comment command. + """ + if by_label: + requested_by = f"by {reviewed_user} with target-branch label" if reviewed_user else "by target-branch label" + else: + requested_by = reviewed_user or "unknown requester" self.logger.info(f"{self.log_prefix} Cherry-pick requested by user: {requested_by}") + pr_author = await asyncio.to_thread(lambda: pull_request.user.login) + assignee_flag = f" -a {shlex.quote(pr_author)}" + self.logger.debug(f"{self.log_prefix} Cherry-pick PR assignee: {pr_author}") + new_branch_name = f"{CHERRY_PICKED_LABEL}-{pull_request.head.ref}-{shortuuid.uuid()[:5]}" if not await self.is_branch_exists(branch=target_branch): err_msg = f"cherry-pick failed: {target_branch} does not exists" @@ -563,7 +582,7 @@ async def cherry_pick(self, pull_request: PullRequest, target_branch: str, revie f"{git_cmd} cherry-pick {commit_hash}", f"{git_cmd} push origin {new_branch_name}", f'bash -c "{hub_cmd} pull-request -b {target_branch} ' - f"-h {new_branch_name} -l {CHERRY_PICKED_LABEL} " + f"-h {new_branch_name} -l {CHERRY_PICKED_LABEL}{assignee_flag} " f"-m '{CHERRY_PICKED_LABEL}: [{target_branch}] " f"{commit_msg_striped}' -m 'cherry-pick {pull_request_url} " f"into {target_branch}' -m 'requested-by {requested_by}'\"", diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 89984976..0a2c902d 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -269,6 +269,20 @@ async def test_process_pull_request_webhook_data_closed_action_merged( mock_label.name = f"{CHERRY_PICK_LABEL_PREFIX}branch1" mock_pull_request.labels = [mock_label] + # Mock a comment from a different user requesting the cherry-pick + mock_request_comment = Mock() + mock_request_comment.body = "/cherry-pick branch1" + mock_request_comment.user = Mock() + mock_request_comment.user.login = "cherry-pick-requester" + + # Cancel command (should be skipped - this is newer, found first in reversed order) + mock_cancel_comment = Mock() + mock_cancel_comment.body = "/cherry-pick cancel" + mock_cancel_comment.user = Mock() + mock_cancel_comment.user.login = "cancel-user" + + mock_pull_request.get_issue_comments.return_value = [mock_request_comment, mock_cancel_comment] + with patch.object(pull_request_handler, "close_issue_for_merged_or_closed_pr") as mock_close_issue: with patch.object(pull_request_handler, "delete_remote_tag_for_merged_or_closed_pr") as mock_delete_tag: with patch.object( @@ -286,7 +300,10 @@ async def test_process_pull_request_webhook_data_closed_action_merged( ) mock_delete_tag.assert_called_once_with(pull_request=mock_pull_request) mock_cherry_pick.assert_called_once_with( - pull_request=mock_pull_request, target_branch="branch1" + pull_request=mock_pull_request, + target_branch="branch1", + reviewed_user="cherry-pick-requester", + by_label=True, ) mock_build.assert_called_once_with( push=True, @@ -296,6 +313,39 @@ async def test_process_pull_request_webhook_data_closed_action_merged( ) mock_label_all.assert_called_once() + @pytest.mark.asyncio + async def test_process_pull_request_cherry_pick_label_no_comment_falls_back_to_pr_author( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test cherry-pick by label falls back to PR author when no /cherry-pick comment found.""" + pull_request_handler.hook_data["action"] = "closed" + pull_request_handler.hook_data["pull_request"]["merged"] = True + + mock_label = Mock() + mock_label.name = f"{CHERRY_PICK_LABEL_PREFIX}branch1" + mock_pull_request.labels = [mock_label] + # No matching cherry-pick comment — label was added manually + mock_pull_request.get_issue_comments.return_value = [] + + with patch.object(pull_request_handler, "close_issue_for_merged_or_closed_pr"): + with patch.object(pull_request_handler, "delete_remote_tag_for_merged_or_closed_pr"): + with patch.object( + pull_request_handler.runner_handler, "cherry_pick", new_callable=AsyncMock + ) as mock_cherry_pick: + with patch.object( + pull_request_handler.runner_handler, "run_build_container", new_callable=AsyncMock + ): + with patch.object( + pull_request_handler, "label_all_opened_pull_requests_merge_state_after_merged" + ): + await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) + mock_cherry_pick.assert_called_once_with( + pull_request=mock_pull_request, + target_branch="branch1", + reviewed_user="owner1", + by_label=True, + ) + @pytest.mark.asyncio async def test_process_pull_request_webhook_data_labeled_action( self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 0ec92acc..4c5e7fc7 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -1,4 +1,6 @@ from collections.abc import Generator +from contextlib import asynccontextmanager +from dataclasses import dataclass from unittest.mock import AsyncMock, Mock, patch import pytest @@ -13,6 +15,15 @@ ) +@dataclass +class CherryPickMocks: + set_progress: Mock + set_success: Mock + run_cmd: Mock + comment: Mock + to_thread: Mock + + class TestRunnerHandler: """Test suite for RunnerHandler class.""" @@ -68,6 +79,8 @@ def mock_pull_request(self) -> Mock: mock_pr.head.ref = "feature-branch" mock_pr.merge_commit_sha = "abc123" mock_pr.html_url = "https://github.com/test/repo/pull/123" + mock_pr.user = Mock() + mock_pr.user.login = "test-pr-author" mock_pr.create_issue_comment = Mock() return mock_pr @@ -647,7 +660,9 @@ async def test_cherry_pick_branch_not_exists(self, runner_handler: RunnerHandler """Test cherry_pick when target branch doesn't exist.""" with patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=None)): with patch.object(mock_pull_request, "create_issue_comment", new=Mock()) as mock_comment: - await runner_handler.cherry_pick(mock_pull_request, "non-existent-branch") + await runner_handler.cherry_pick( + mock_pull_request, "non-existent-branch", reviewed_user="test-requester" + ) mock_comment.assert_called_once() @pytest.mark.asyncio @@ -663,7 +678,7 @@ async def test_cherry_pick_prepare_failure(self, runner_handler: RunnerHandler, return_value=(False, "/tmp/worktree-path", "out", "err") ) mock_checkout.return_value.__aexit__ = AsyncMock(return_value=None) - await runner_handler.cherry_pick(mock_pull_request, "main") + await runner_handler.cherry_pick(mock_pull_request, "main", reviewed_user="test-requester") mock_set_progress.assert_called_once() assert mock_set_failure.call_count >= 1 @@ -684,7 +699,7 @@ async def test_cherry_pick_command_failure(self, runner_handler: RunnerHandler, "webhook_server.utils.helpers.run_command", new=AsyncMock(return_value=(False, "output", "error")), ): - await runner_handler.cherry_pick(mock_pull_request, "main") + await runner_handler.cherry_pick(mock_pull_request, "main", reviewed_user="test-requester") mock_set_progress.assert_called_once() mock_set_failure.assert_called_once() @@ -706,7 +721,9 @@ async def test_cherry_pick_success(self, runner_handler: RunnerHandler, mock_pul new=AsyncMock(return_value=(True, "success", "")), ): with patch.object(mock_pull_request, "create_issue_comment", new=Mock()) as mock_comment: - await runner_handler.cherry_pick(mock_pull_request, "main") + await runner_handler.cherry_pick( + mock_pull_request, "main", reviewed_user="test-requester" + ) mock_set_progress.assert_called_once() mock_set_success.assert_called_once() mock_comment.assert_called_once() @@ -913,11 +930,76 @@ async def test_cherry_pick_manual_needed(self, runner_handler, mock_pull_request # First command fails, triggers manual cherry-pick with patch("webhook_server.utils.helpers.run_command", side_effect=[(False, "fail", "err")]): with patch.object(mock_pull_request, "create_issue_comment", new=Mock()) as mock_comment: - await runner_handler.cherry_pick(mock_pull_request, "main") + await runner_handler.cherry_pick( + mock_pull_request, "main", reviewed_user="test-requester" + ) mock_set_progress.assert_called_once() mock_set_failure.assert_called_once() mock_comment.assert_called_once() + @staticmethod + @asynccontextmanager + async def cherry_pick_setup( + runner_handler: RunnerHandler, + mock_pull_request: Mock, + ): + """Common setup for cherry-pick tests.""" + runner_handler.github_webhook.pypi = {"token": "dummy"} + with patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())): + with patch.object(runner_handler.check_run_handler, "set_check_in_progress") as mock_set_progress: + with patch.object(runner_handler.check_run_handler, "set_check_success") as mock_set_success: + with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: + mock_checkout.return_value = AsyncMock() + mock_checkout.return_value.__aenter__ = AsyncMock( + return_value=(True, "/tmp/worktree-path", "", "") + ) + mock_checkout.return_value.__aexit__ = AsyncMock(return_value=None) + with patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(return_value=(True, "success", "")), + ) as mock_run_cmd: + with patch.object(mock_pull_request, "create_issue_comment", new=Mock()) as mock_comment: + with patch( + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), + ) as mock_to_thread: + yield CherryPickMocks( + set_progress=mock_set_progress, + set_success=mock_set_success, + run_cmd=mock_run_cmd, + comment=mock_comment, + to_thread=mock_to_thread, + ) + + @pytest.mark.asyncio + async def test_cherry_pick_assigns_pr_author(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: + """Test cherry_pick assigns to PR author, not the cherry-pick requester.""" + async with self.cherry_pick_setup(runner_handler, mock_pull_request) as mocks: + await runner_handler.cherry_pick(mock_pull_request, "main", reviewed_user="cherry-requester") + mocks.set_progress.assert_called_once() + mocks.set_success.assert_called_once() + mocks.comment.assert_called_once() + assert mocks.to_thread.call_count == 2 + last_cmd = mocks.run_cmd.call_args_list[-1] + hub_command = last_cmd.kwargs.get("command", last_cmd.args[0] if last_cmd.args else "") + assert "-a 'test-pr-author'" in hub_command or "-a test-pr-author" in hub_command + + @pytest.mark.asyncio + async def test_cherry_pick_by_label_requested_by_format( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test cherry_pick by_label produces correct requested-by format in hub command.""" + async with self.cherry_pick_setup(runner_handler, mock_pull_request) as mocks: + await runner_handler.cherry_pick(mock_pull_request, "main", reviewed_user="label-requester", by_label=True) + mocks.set_progress.assert_called_once() + mocks.set_success.assert_called_once() + mocks.comment.assert_called_once() + last_cmd = mocks.run_cmd.call_args_list[-1] + hub_command = last_cmd.kwargs.get("command", last_cmd.args[0] if last_cmd.args else "") + assert "requested-by by label-requester with target-branch label" in hub_command + assert "-a 'test-pr-author'" in hub_command or "-a test-pr-author" in hub_command + assert mocks.to_thread.call_count == 2 + @pytest.mark.asyncio async def test_checkout_worktree_branch_already_checked_out( self, runner_handler: RunnerHandler, mock_pull_request: Mock diff --git a/webhook_server/tests/test_schema_validator.py b/webhook_server/tests/test_schema_validator.py index eb5353a6..c00b578a 100644 --- a/webhook_server/tests/test_schema_validator.py +++ b/webhook_server/tests/test_schema_validator.py @@ -166,7 +166,12 @@ def _validate_single_repository(self, repo_name: str, repo_config: Any) -> None: self.errors.append(f"Repository '{repo_name}' field '{field}' must be a string") # Boolean fields - boolean_fields = ["verified-job", "pre-commit", "mask-sensitive-data", "auto-verify-cherry-picked-prs"] + boolean_fields = [ + "verified-job", + "pre-commit", + "mask-sensitive-data", + "auto-verify-cherry-picked-prs", + ] for field in boolean_fields: if field in repo_config and not isinstance(repo_config[field], bool): self.errors.append(f"Repository '{repo_name}' field '{field}' must be a boolean")