From 4a90312716f66fd24967d2e9ac0df24038e88810 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 19 Feb 2026 15:11:47 +0200 Subject: [PATCH 01/11] feat(cherry-pick): add PR author attribution and optional assignee for cherry-pick PRs --- webhook_server/config/schema.yaml | 8 ++ webhook_server/libs/github_api.py | 3 + .../libs/handlers/pull_request_handler.py | 12 +- .../libs/handlers/runner_handler.py | 30 ++++- .../tests/test_pull_request_handler.py | 5 +- webhook_server/tests/test_runner_handler.py | 105 ++++++++++++++++++ 6 files changed, 155 insertions(+), 8 deletions(-) diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index e3825cc2..ce7213e2 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -79,6 +79,10 @@ properties: type: boolean description: Automatically verify cherry-picked PRs (default is true for backward compatibility) default: true + cherry-pick-assign-to-pr-author: + type: boolean + description: Assign cherry-picked PRs to the original PR author instead of the bot user + default: false create-issue-for-new-pr: type: boolean description: Create a tracking issue for new pull requests (global default) @@ -273,6 +277,10 @@ properties: type: boolean description: Automatically verify cherry-picked PRs for this repository (default is true) default: true + cherry-pick-assign-to-pr-author: + type: boolean + description: Assign cherry-picked PRs to the original PR author for this repository + default: false github-tokens: type: array items: diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index e070167d..2b579863 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -712,6 +712,9 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None: self.auto_verify_cherry_picked_prs: bool = self.config.get_value( value="auto-verify-cherry-picked-prs", return_on_none=True, extra_dict=repository_config ) + self.cherry_pick_assign_to_pr_author: bool = self.config.get_value( + value="cherry-pick-assign-to-pr-author", return_on_none=False, extra_dict=repository_config + ) _required_labels = self.config.get_value( value="can-be-merged-required-labels", return_on_none=[], extra_dict=repository_config ) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 3b1743bb..6df170de 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -126,11 +126,15 @@ 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): + cherry_pick_labels = [_label for _label in labels if _label.name.startswith(CHERRY_PICK_LABEL_PREFIX)] + if cherry_pick_labels: + pr_author = await asyncio.to_thread(lambda: pull_request.user.login) + for _label in cherry_pick_labels: 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=_label.name.replace(CHERRY_PICK_LABEL_PREFIX, ""), + reviewed_user=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..32669def 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -536,10 +536,34 @@ 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. + 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" + else: + requested_by = reviewed_user or "by target-branch label" self.logger.info(f"{self.log_prefix} Cherry-pick requested by user: {requested_by}") + if self.github_webhook.cherry_pick_assign_to_pr_author: + if reviewed_user: + pr_author = reviewed_user + else: + pr_author = await asyncio.to_thread(lambda: pull_request.user.login) + assignee_flag = f" -a {shlex.quote(pr_author)}" + else: + assignee_flag = "" + 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 +587,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..f1771f0b 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -286,7 +286,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="owner1", + by_label=True, ) mock_build.assert_called_once_with( push=True, diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 0ec92acc..614c165b 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -44,6 +44,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.container_command_args = [] mock_webhook.ctx = None mock_webhook.custom_check_runs = [] + mock_webhook.cherry_pick_assign_to_pr_author = False return mock_webhook @pytest.fixture @@ -918,6 +919,110 @@ async def test_cherry_pick_manual_needed(self, runner_handler, mock_pull_request mock_set_failure.assert_called_once() mock_comment.assert_called_once() + @pytest.mark.asyncio + async def test_cherry_pick_assigns_to_reviewed_user( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test cherry_pick uses reviewed_user for assignee when cherry_pick_assign_to_pr_author is True.""" + runner_handler.github_webhook.pypi = {"token": "dummy"} + runner_handler.github_webhook.cherry_pick_assign_to_pr_author = True + 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: + await runner_handler.cherry_pick( + mock_pull_request, "main", reviewed_user="cherry-requester" + ) + mock_set_progress.assert_called_once() + mock_set_success.assert_called_once() + mock_comment.assert_called_once() + # Verify the hub pull-request command includes -a with the reviewed_user + last_cmd = mock_run_cmd.call_args_list[-1] + hub_command = last_cmd.kwargs.get("command", last_cmd.args[0] if last_cmd.args else "") + assert "-a 'cherry-requester'" in hub_command or "-a cherry-requester" in hub_command + + @pytest.mark.asyncio + async def test_cherry_pick_assigns_to_pr_author_fallback( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test cherry_pick falls back to pull_request.user.login when reviewed_user is empty.""" + runner_handler.github_webhook.pypi = {"token": "dummy"} + runner_handler.github_webhook.cherry_pick_assign_to_pr_author = True + mock_pull_request.user = Mock() + mock_pull_request.user.login = "pr-author-login" + 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: + await runner_handler.cherry_pick(mock_pull_request, "main", reviewed_user="") + mock_set_progress.assert_called_once() + mock_set_success.assert_called_once() + mock_comment.assert_called_once() + # Verify asyncio.to_thread was called (for user.login fallback) + assert mock_to_thread.call_count >= 1 + # Verify the hub pull-request command includes -a with the pr author + last_cmd = mock_run_cmd.call_args_list[-1] + hub_command = last_cmd.kwargs.get( + "command", last_cmd.args[0] if last_cmd.args else "" + ) + assert "-a 'pr-author-login'" in hub_command or "-a pr-author-login" 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.""" + runner_handler.github_webhook.pypi = {"token": "dummy"} + runner_handler.github_webhook.cherry_pick_assign_to_pr_author = True + 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: + await runner_handler.cherry_pick( + mock_pull_request, "main", reviewed_user="label-user", by_label=True + ) + mock_set_progress.assert_called_once() + mock_set_success.assert_called_once() + mock_comment.assert_called_once() + # Verify the hub command's last -m contains the by_label format + last_cmd = mock_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-user with target-branch label" in hub_command + @pytest.mark.asyncio async def test_checkout_worktree_branch_already_checked_out( self, runner_handler: RunnerHandler, mock_pull_request: Mock From 9452a3894732f836e2be948beed1e9a6888fab61 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 19 Feb 2026 15:47:38 +0200 Subject: [PATCH 02/11] fix(cherry-pick): always use PR author for assignee, not command requester --- .../libs/handlers/runner_handler.py | 5 +- webhook_server/tests/test_runner_handler.py | 81 +++++++++++++++---- 2 files changed, 67 insertions(+), 19 deletions(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 32669def..b9a35ec4 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -556,10 +556,7 @@ async def cherry_pick( self.logger.info(f"{self.log_prefix} Cherry-pick requested by user: {requested_by}") if self.github_webhook.cherry_pick_assign_to_pr_author: - if reviewed_user: - pr_author = reviewed_user - else: - pr_author = await asyncio.to_thread(lambda: pull_request.user.login) + pr_author = await asyncio.to_thread(lambda: pull_request.user.login) assignee_flag = f" -a {shlex.quote(pr_author)}" else: assignee_flag = "" diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 614c165b..e7a7c8a3 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -920,12 +920,14 @@ async def test_cherry_pick_manual_needed(self, runner_handler, mock_pull_request mock_comment.assert_called_once() @pytest.mark.asyncio - async def test_cherry_pick_assigns_to_reviewed_user( + async def test_cherry_pick_assigns_to_pr_author( self, runner_handler: RunnerHandler, mock_pull_request: Mock ) -> None: - """Test cherry_pick uses reviewed_user for assignee when cherry_pick_assign_to_pr_author is True.""" + """Test cherry_pick assigns to the original PR author, not the requester.""" runner_handler.github_webhook.pypi = {"token": "dummy"} runner_handler.github_webhook.cherry_pick_assign_to_pr_author = True + mock_pull_request.user = Mock() + mock_pull_request.user.login = "original-pr-author" 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: @@ -940,16 +942,25 @@ async def test_cherry_pick_assigns_to_reviewed_user( new=AsyncMock(return_value=(True, "success", "")), ) as mock_run_cmd: with patch.object(mock_pull_request, "create_issue_comment", new=Mock()) as mock_comment: - await runner_handler.cherry_pick( - mock_pull_request, "main", reviewed_user="cherry-requester" - ) - mock_set_progress.assert_called_once() - mock_set_success.assert_called_once() - mock_comment.assert_called_once() - # Verify the hub pull-request command includes -a with the reviewed_user - last_cmd = mock_run_cmd.call_args_list[-1] - hub_command = last_cmd.kwargs.get("command", last_cmd.args[0] if last_cmd.args else "") - assert "-a 'cherry-requester'" in hub_command or "-a cherry-requester" in hub_command + with patch( + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), + ): + await runner_handler.cherry_pick( + mock_pull_request, "main", reviewed_user="cherry-requester" + ) + mock_set_progress.assert_called_once() + mock_set_success.assert_called_once() + mock_comment.assert_called_once() + # Verify the hub command assigns to the PR author, NOT the requester + last_cmd = mock_run_cmd.call_args_list[-1] + hub_command = last_cmd.kwargs.get( + "command", last_cmd.args[0] if last_cmd.args else "" + ) + assert ( + "-a 'original-pr-author'" in hub_command + or "-a original-pr-author" in hub_command + ) @pytest.mark.asyncio async def test_cherry_pick_assigns_to_pr_author_fallback( @@ -998,6 +1009,46 @@ async def test_cherry_pick_by_label_requested_by_format( """Test cherry_pick by_label produces correct requested-by format in hub command.""" runner_handler.github_webhook.pypi = {"token": "dummy"} runner_handler.github_webhook.cherry_pick_assign_to_pr_author = True + mock_pull_request.user = Mock() + mock_pull_request.user.login = "label-user" + 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()), + ): + await runner_handler.cherry_pick( + mock_pull_request, "main", reviewed_user="label-user", by_label=True + ) + mock_set_progress.assert_called_once() + mock_set_success.assert_called_once() + mock_comment.assert_called_once() + # Verify the hub command's last -m contains the by_label format + last_cmd = mock_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-user with target-branch label" in hub_command + + @pytest.mark.asyncio + async def test_cherry_pick_disabled_no_assignee( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test cherry_pick does not include -a flag when cherry_pick_assign_to_pr_author is False.""" + runner_handler.github_webhook.pypi = {"token": "dummy"} + runner_handler.github_webhook.cherry_pick_assign_to_pr_author = False 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: @@ -1013,15 +1064,15 @@ async def test_cherry_pick_by_label_requested_by_format( ) as mock_run_cmd: with patch.object(mock_pull_request, "create_issue_comment", new=Mock()) as mock_comment: await runner_handler.cherry_pick( - mock_pull_request, "main", reviewed_user="label-user", by_label=True + mock_pull_request, "main", reviewed_user="cherry-requester" ) mock_set_progress.assert_called_once() mock_set_success.assert_called_once() mock_comment.assert_called_once() - # Verify the hub command's last -m contains the by_label format + # Verify the hub pull-request command does NOT contain -a flag last_cmd = mock_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-user with target-branch label" in hub_command + assert " -a " not in hub_command @pytest.mark.asyncio async def test_checkout_worktree_branch_already_checked_out( From d609248ddbd4780beeb4105e552a41e02f9dd8b9 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 19 Feb 2026 16:21:50 +0200 Subject: [PATCH 03/11] fix(cherry-pick): handle empty reviewed_user in by_label path and fix test naming --- webhook_server/libs/handlers/runner_handler.py | 5 ++++- webhook_server/tests/test_runner_handler.py | 11 +++++++---- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index b9a35ec4..2ae9fce0 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -550,7 +550,10 @@ async def cherry_pick( 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: + requested_by = f"by {reviewed_user} with target-branch label" + else: + requested_by = "by target-branch label" else: requested_by = reviewed_user or "by target-branch label" self.logger.info(f"{self.log_prefix} Cherry-pick requested by user: {requested_by}") diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index e7a7c8a3..7282d5a9 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -963,10 +963,13 @@ async def test_cherry_pick_assigns_to_pr_author( ) @pytest.mark.asyncio - async def test_cherry_pick_assigns_to_pr_author_fallback( + async def test_cherry_pick_always_assigns_to_pr_author_when_flag_set( self, runner_handler: RunnerHandler, mock_pull_request: Mock ) -> None: - """Test cherry_pick falls back to pull_request.user.login when reviewed_user is empty.""" + """Test cherry_pick always uses pull_request.user.login as assignee. + + When cherry_pick_assign_to_pr_author is True, regardless of reviewed_user. + """ runner_handler.github_webhook.pypi = {"token": "dummy"} runner_handler.github_webhook.cherry_pick_assign_to_pr_author = True mock_pull_request.user = Mock() @@ -993,9 +996,9 @@ async def test_cherry_pick_assigns_to_pr_author_fallback( mock_set_progress.assert_called_once() mock_set_success.assert_called_once() mock_comment.assert_called_once() - # Verify asyncio.to_thread was called (for user.login fallback) + # Verify asyncio.to_thread was called (for user.login access) assert mock_to_thread.call_count >= 1 - # Verify the hub pull-request command includes -a with the pr author + # Verify assignee is always pull_request.user.login, not reviewed_user last_cmd = mock_run_cmd.call_args_list[-1] hub_command = last_cmd.kwargs.get( "command", last_cmd.args[0] if last_cmd.args else "" From c339f7eebdfb211f7d54bda2c97bed81d46865a9 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 19 Feb 2026 19:36:25 +0200 Subject: [PATCH 04/11] fix(cherry-pick): tighten test assertions and add edge case coverage --- webhook_server/tests/test_runner_handler.py | 47 ++++++++++++++++++++- 1 file changed, 45 insertions(+), 2 deletions(-) diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 7282d5a9..c8530b16 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -996,8 +996,8 @@ async def test_cherry_pick_always_assigns_to_pr_author_when_flag_set( mock_set_progress.assert_called_once() mock_set_success.assert_called_once() mock_comment.assert_called_once() - # Verify asyncio.to_thread was called (for user.login access) - assert mock_to_thread.call_count >= 1 + # Exactly 2 calls: user.login + create_issue_comment + assert mock_to_thread.call_count == 2 # Verify assignee is always pull_request.user.login, not reviewed_user last_cmd = mock_run_cmd.call_args_list[-1] hub_command = last_cmd.kwargs.get( @@ -1045,6 +1045,49 @@ async def test_cherry_pick_by_label_requested_by_format( ) assert "requested-by by label-user with target-branch label" in hub_command + @pytest.mark.asyncio + async def test_cherry_pick_by_label_empty_reviewed_user_requested_by_format( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test cherry_pick by_label with empty reviewed_user produces clean requested-by string.""" + runner_handler.github_webhook.pypi = {"token": "dummy"} + runner_handler.github_webhook.cherry_pick_assign_to_pr_author = False + mock_pull_request.user = Mock() + mock_pull_request.user.login = "label-user" + 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()), + ): + await runner_handler.cherry_pick( + mock_pull_request, "main", reviewed_user="", by_label=True + ) + mock_set_progress.assert_called_once() + mock_set_success.assert_called_once() + mock_comment.assert_called_once() + # Verify the hub command contains clean requested-by string + last_cmd = mock_run_cmd.call_args_list[-1] + hub_command = last_cmd.kwargs.get( + "command", last_cmd.args[0] if last_cmd.args else "" + ) + # Should contain "requested-by by target-branch label" (no double space) + assert "requested-by by target-branch label" in hub_command + # The double-space bug: "by with" should NOT appear + assert "by with" not in hub_command + @pytest.mark.asyncio async def test_cherry_pick_disabled_no_assignee( self, runner_handler: RunnerHandler, mock_pull_request: Mock From a9395b786aaff9c04578f85d087b0d94b6227214 Mon Sep 17 00:00:00 2001 From: rnetser Date: Thu, 19 Feb 2026 20:52:45 +0200 Subject: [PATCH 05/11] fix(cherry-pick): strengthen test assertions with distinct values and call count checks --- webhook_server/tests/test_runner_handler.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index c8530b16..bcfe5384 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -945,13 +945,15 @@ async def test_cherry_pick_assigns_to_pr_author( 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: await runner_handler.cherry_pick( mock_pull_request, "main", reviewed_user="cherry-requester" ) mock_set_progress.assert_called_once() mock_set_success.assert_called_once() mock_comment.assert_called_once() + # Exactly 2 calls: user.login + create_issue_comment + assert mock_to_thread.call_count == 2 # Verify the hub command assigns to the PR author, NOT the requester last_cmd = mock_run_cmd.call_args_list[-1] hub_command = last_cmd.kwargs.get( @@ -1013,7 +1015,7 @@ async def test_cherry_pick_by_label_requested_by_format( runner_handler.github_webhook.pypi = {"token": "dummy"} runner_handler.github_webhook.cherry_pick_assign_to_pr_author = True mock_pull_request.user = Mock() - mock_pull_request.user.login = "label-user" + mock_pull_request.user.login = "pr-author-login" 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: @@ -1033,7 +1035,7 @@ async def test_cherry_pick_by_label_requested_by_format( new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), ): await runner_handler.cherry_pick( - mock_pull_request, "main", reviewed_user="label-user", by_label=True + mock_pull_request, "main", reviewed_user="label-requester", by_label=True ) mock_set_progress.assert_called_once() mock_set_success.assert_called_once() @@ -1043,7 +1045,8 @@ async def test_cherry_pick_by_label_requested_by_format( hub_command = last_cmd.kwargs.get( "command", last_cmd.args[0] if last_cmd.args else "" ) - assert "requested-by by label-user with target-branch label" in hub_command + assert "requested-by by label-requester with target-branch label" in hub_command + assert "-a 'pr-author-login'" in hub_command or "-a pr-author-login" in hub_command @pytest.mark.asyncio async def test_cherry_pick_by_label_empty_reviewed_user_requested_by_format( From 9f51fd3463ea3a180b1856f2d2606ce5792b6adb Mon Sep 17 00:00:00 2001 From: rnetser Date: Mon, 23 Feb 2026 10:23:39 +0200 Subject: [PATCH 06/11] fix(cherry-pick): capture mock_to_thread and remove dead code in tests - Capture asyncio.to_thread mock in test_cherry_pick_by_label_requested_by_format and add call_count == 2 assertion for consistency with sibling tests - Remove dead-code mock_pull_request.user setup in test_cherry_pick_by_label_empty_reviewed_user_requested_by_format since cherry_pick_assign_to_pr_author=False means user.login is never accessed --- webhook_server/tests/test_runner_handler.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index bcfe5384..bc65cfef 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -1033,7 +1033,7 @@ async def test_cherry_pick_by_label_requested_by_format( 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: await runner_handler.cherry_pick( mock_pull_request, "main", reviewed_user="label-requester", by_label=True ) @@ -1047,6 +1047,7 @@ async def test_cherry_pick_by_label_requested_by_format( ) assert "requested-by by label-requester with target-branch label" in hub_command assert "-a 'pr-author-login'" in hub_command or "-a pr-author-login" in hub_command + assert mock_to_thread.call_count == 2 @pytest.mark.asyncio async def test_cherry_pick_by_label_empty_reviewed_user_requested_by_format( @@ -1055,8 +1056,6 @@ async def test_cherry_pick_by_label_empty_reviewed_user_requested_by_format( """Test cherry_pick by_label with empty reviewed_user produces clean requested-by string.""" runner_handler.github_webhook.pypi = {"token": "dummy"} runner_handler.github_webhook.cherry_pick_assign_to_pr_author = False - mock_pull_request.user = Mock() - mock_pull_request.user.login = "label-user" 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: From ea8c5414c68d8b63238c92ff74ce7419543f19ba Mon Sep 17 00:00:00 2001 From: rnetser Date: Mon, 23 Feb 2026 15:01:17 +0200 Subject: [PATCH 07/11] refactor(cherry-pick): optimize API call, fix fallback message, and reduce test nesting - Reuse reviewed_user as pr_author when by_label=True to avoid redundant asyncio.to_thread call for pull_request.user.login - Fix misleading fallback message: use "unknown requester" instead of "by target-branch label" for comment-triggered cherry-picks - Extract CherryPickMocks dataclass and cherry_pick_setup async context manager to reduce 7-8 levels of nesting to 1 in cherry-pick tests --- .../libs/handlers/runner_handler.py | 7 +- webhook_server/tests/test_runner_handler.py | 232 ++++++------------ 2 files changed, 86 insertions(+), 153 deletions(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 2ae9fce0..99826f1e 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -555,11 +555,14 @@ async def cherry_pick( else: requested_by = "by target-branch label" else: - requested_by = reviewed_user or "by target-branch label" + requested_by = reviewed_user or "unknown requester" self.logger.info(f"{self.log_prefix} Cherry-pick requested by user: {requested_by}") if self.github_webhook.cherry_pick_assign_to_pr_author: - pr_author = await asyncio.to_thread(lambda: pull_request.user.login) + if by_label and reviewed_user: + pr_author = reviewed_user + else: + pr_author = await asyncio.to_thread(lambda: pull_request.user.login) assignee_flag = f" -a {shlex.quote(pr_author)}" else: assignee_flag = "" diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index bc65cfef..cb0e7c7b 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.""" @@ -919,15 +930,17 @@ async def test_cherry_pick_manual_needed(self, runner_handler, mock_pull_request mock_set_failure.assert_called_once() mock_comment.assert_called_once() - @pytest.mark.asyncio - async def test_cherry_pick_assigns_to_pr_author( - self, runner_handler: RunnerHandler, mock_pull_request: Mock - ) -> None: - """Test cherry_pick assigns to the original PR author, not the requester.""" + @staticmethod + @asynccontextmanager + async def cherry_pick_setup( + runner_handler: RunnerHandler, + mock_pull_request: Mock, + *, + assign_to_author: bool = False, + ): + """Common setup for cherry-pick tests.""" runner_handler.github_webhook.pypi = {"token": "dummy"} - runner_handler.github_webhook.cherry_pick_assign_to_pr_author = True - mock_pull_request.user = Mock() - mock_pull_request.user.login = "original-pr-author" + runner_handler.github_webhook.cherry_pick_assign_to_pr_author = assign_to_author 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: @@ -946,24 +959,31 @@ async def test_cherry_pick_assigns_to_pr_author( "asyncio.to_thread", new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), ) as mock_to_thread: - await runner_handler.cherry_pick( - mock_pull_request, "main", reviewed_user="cherry-requester" - ) - mock_set_progress.assert_called_once() - mock_set_success.assert_called_once() - mock_comment.assert_called_once() - # Exactly 2 calls: user.login + create_issue_comment - assert mock_to_thread.call_count == 2 - # Verify the hub command assigns to the PR author, NOT the requester - last_cmd = mock_run_cmd.call_args_list[-1] - hub_command = last_cmd.kwargs.get( - "command", last_cmd.args[0] if last_cmd.args else "" - ) - assert ( - "-a 'original-pr-author'" in hub_command - or "-a original-pr-author" in hub_command + 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_to_pr_author( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test cherry_pick assigns to the original PR author, not the requester.""" + mock_pull_request.user = Mock() + mock_pull_request.user.login = "original-pr-author" + async with self.cherry_pick_setup(runner_handler, mock_pull_request, assign_to_author=True) 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 'original-pr-author'" in hub_command or "-a original-pr-author" in hub_command + @pytest.mark.asyncio async def test_cherry_pick_always_assigns_to_pr_author_when_flag_set( self, runner_handler: RunnerHandler, mock_pull_request: Mock @@ -972,155 +992,65 @@ async def test_cherry_pick_always_assigns_to_pr_author_when_flag_set( When cherry_pick_assign_to_pr_author is True, regardless of reviewed_user. """ - runner_handler.github_webhook.pypi = {"token": "dummy"} - runner_handler.github_webhook.cherry_pick_assign_to_pr_author = True mock_pull_request.user = Mock() mock_pull_request.user.login = "pr-author-login" - 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: - await runner_handler.cherry_pick(mock_pull_request, "main", reviewed_user="") - mock_set_progress.assert_called_once() - mock_set_success.assert_called_once() - mock_comment.assert_called_once() - # Exactly 2 calls: user.login + create_issue_comment - assert mock_to_thread.call_count == 2 - # Verify assignee is always pull_request.user.login, not reviewed_user - last_cmd = mock_run_cmd.call_args_list[-1] - hub_command = last_cmd.kwargs.get( - "command", last_cmd.args[0] if last_cmd.args else "" - ) - assert "-a 'pr-author-login'" in hub_command or "-a pr-author-login" in hub_command + async with self.cherry_pick_setup(runner_handler, mock_pull_request, assign_to_author=True) as mocks: + await runner_handler.cherry_pick(mock_pull_request, "main", reviewed_user="") + 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 'pr-author-login'" in hub_command or "-a pr-author-login" 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.""" - runner_handler.github_webhook.pypi = {"token": "dummy"} - runner_handler.github_webhook.cherry_pick_assign_to_pr_author = True mock_pull_request.user = Mock() mock_pull_request.user.login = "pr-author-login" - 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: - await runner_handler.cherry_pick( - mock_pull_request, "main", reviewed_user="label-requester", by_label=True - ) - mock_set_progress.assert_called_once() - mock_set_success.assert_called_once() - mock_comment.assert_called_once() - # Verify the hub command's last -m contains the by_label format - last_cmd = mock_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 'pr-author-login'" in hub_command or "-a pr-author-login" in hub_command - assert mock_to_thread.call_count == 2 + async with self.cherry_pick_setup(runner_handler, mock_pull_request, assign_to_author=True) 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 'label-requester'" in hub_command or "-a label-requester" in hub_command + # Only 1 call (create_issue_comment) - user.login skipped via by_label optimization + assert mocks.to_thread.call_count == 1 @pytest.mark.asyncio async def test_cherry_pick_by_label_empty_reviewed_user_requested_by_format( self, runner_handler: RunnerHandler, mock_pull_request: Mock ) -> None: """Test cherry_pick by_label with empty reviewed_user produces clean requested-by string.""" - runner_handler.github_webhook.pypi = {"token": "dummy"} - runner_handler.github_webhook.cherry_pick_assign_to_pr_author = False - 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()), - ): - await runner_handler.cherry_pick( - mock_pull_request, "main", reviewed_user="", by_label=True - ) - mock_set_progress.assert_called_once() - mock_set_success.assert_called_once() - mock_comment.assert_called_once() - # Verify the hub command contains clean requested-by string - last_cmd = mock_run_cmd.call_args_list[-1] - hub_command = last_cmd.kwargs.get( - "command", last_cmd.args[0] if last_cmd.args else "" - ) - # Should contain "requested-by by target-branch label" (no double space) - assert "requested-by by target-branch label" in hub_command - # The double-space bug: "by with" should NOT appear - assert "by with" not 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="", 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 target-branch label" in hub_command + assert "by with" not in hub_command @pytest.mark.asyncio async def test_cherry_pick_disabled_no_assignee( self, runner_handler: RunnerHandler, mock_pull_request: Mock ) -> None: """Test cherry_pick does not include -a flag when cherry_pick_assign_to_pr_author is False.""" - runner_handler.github_webhook.pypi = {"token": "dummy"} - runner_handler.github_webhook.cherry_pick_assign_to_pr_author = False - 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: - await runner_handler.cherry_pick( - mock_pull_request, "main", reviewed_user="cherry-requester" - ) - mock_set_progress.assert_called_once() - mock_set_success.assert_called_once() - mock_comment.assert_called_once() - # Verify the hub pull-request command does NOT contain -a flag - last_cmd = mock_run_cmd.call_args_list[-1] - hub_command = last_cmd.kwargs.get("command", last_cmd.args[0] if last_cmd.args else "") - assert " -a " not 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="cherry-requester") + 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 " -a " not in hub_command @pytest.mark.asyncio async def test_checkout_worktree_branch_already_checked_out( From 7023d15418f82aa2e24240fbeac4449652165b2e Mon Sep 17 00:00:00 2001 From: rnetser Date: Mon, 23 Feb 2026 15:18:24 +0200 Subject: [PATCH 08/11] test(schema): add cherry-pick-assign-to-pr-author to schema validation tests --- webhook_server/tests/test_config_schema.py | 2 ++ webhook_server/tests/test_schema_validator.py | 9 ++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/webhook_server/tests/test_config_schema.py b/webhook_server/tests/test_config_schema.py index 9e0e95b8..d7246826 100644 --- a/webhook_server/tests/test_config_schema.py +++ b/webhook_server/tests/test_config_schema.py @@ -41,6 +41,7 @@ def valid_full_config(self) -> dict[str, Any]: "docker": {"username": "dockeruser", "password": "dockerpass"}, # pragma: allowlist secret "default-status-checks": ["WIP", "build"], "auto-verified-and-merged-users": ["bot[bot]"], + "cherry-pick-assign-to-pr-author": True, "branch-protection": { "strict": True, "require_code_owner_reviews": True, @@ -75,6 +76,7 @@ def valid_full_config(self) -> dict[str, Any]: "github-tokens": ["repo-token"], "branch-protection": {"strict": False, "required_approving_review_count": 1}, "set-auto-merge-prs": ["main"], + "cherry-pick-assign-to-pr-author": True, "can-be-merged-required-labels": ["ready"], "allow-commands-on-draft-prs": ["wip", "hold"], "conventional-title": "feat,fix,docs", diff --git a/webhook_server/tests/test_schema_validator.py b/webhook_server/tests/test_schema_validator.py index eb5353a6..1784e9bf 100644 --- a/webhook_server/tests/test_schema_validator.py +++ b/webhook_server/tests/test_schema_validator.py @@ -73,6 +73,7 @@ def _validate_root_fields(self, config: dict[str, Any]) -> None: "disable-ssl-warnings", "mask-sensitive-data", "auto-verify-cherry-picked-prs", + "cherry-pick-assign-to-pr-author", ] for field in boolean_fields: if field in config and not isinstance(config[field], bool): @@ -166,7 +167,13 @@ 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", + "cherry-pick-assign-to-pr-author", + ] 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") From b10d527ecee0156dc09666cbcbf4aea923a20657 Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 24 Feb 2026 18:40:14 +0200 Subject: [PATCH 09/11] remove config for username in cherrypick --- webhook_server/config/schema.yaml | 8 -- webhook_server/libs/github_api.py | 3 - .../libs/handlers/pull_request_handler.py | 23 ++++- .../libs/handlers/runner_handler.py | 9 +- webhook_server/tests/test_config_schema.py | 2 - .../tests/test_pull_request_handler.py | 49 +++++++++- webhook_server/tests/test_runner_handler.py | 89 +++++-------------- webhook_server/tests/test_schema_validator.py | 2 - 8 files changed, 90 insertions(+), 95 deletions(-) diff --git a/webhook_server/config/schema.yaml b/webhook_server/config/schema.yaml index ce7213e2..e3825cc2 100644 --- a/webhook_server/config/schema.yaml +++ b/webhook_server/config/schema.yaml @@ -79,10 +79,6 @@ properties: type: boolean description: Automatically verify cherry-picked PRs (default is true for backward compatibility) default: true - cherry-pick-assign-to-pr-author: - type: boolean - description: Assign cherry-picked PRs to the original PR author instead of the bot user - default: false create-issue-for-new-pr: type: boolean description: Create a tracking issue for new pull requests (global default) @@ -277,10 +273,6 @@ properties: type: boolean description: Automatically verify cherry-picked PRs for this repository (default is true) default: true - cherry-pick-assign-to-pr-author: - type: boolean - description: Assign cherry-picked PRs to the original PR author for this repository - default: false github-tokens: type: array items: diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 2b579863..e070167d 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -712,9 +712,6 @@ def _repo_data_from_config(self, repository_config: dict[str, Any]) -> None: self.auto_verify_cherry_picked_prs: bool = self.config.get_value( value="auto-verify-cherry-picked-prs", return_on_none=True, extra_dict=repository_config ) - self.cherry_pick_assign_to_pr_author: bool = self.config.get_value( - value="cherry-pick-assign-to-pr-author", return_on_none=False, extra_dict=repository_config - ) _required_labels = self.config.get_value( value="can-be-merged-required-labels", return_on_none=[], extra_dict=repository_config ) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 6df170de..a2b2fd6a 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, @@ -128,12 +129,30 @@ async def process_pull_request_webhook_data(self, pull_request: PullRequest) -> labels = await asyncio.to_thread(lambda: list(pull_request.labels)) cherry_pick_labels = [_label for _label in labels if _label.name.startswith(CHERRY_PICK_LABEL_PREFIX)] if cherry_pick_labels: + 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, ""), - reviewed_user=pr_author, + target_branch=target_branch, + reviewed_user=requester or pr_author, by_label=True, ) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 99826f1e..3076131d 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -558,14 +558,7 @@ async def cherry_pick( requested_by = reviewed_user or "unknown requester" self.logger.info(f"{self.log_prefix} Cherry-pick requested by user: {requested_by}") - if self.github_webhook.cherry_pick_assign_to_pr_author: - if by_label and reviewed_user: - pr_author = reviewed_user - else: - pr_author = await asyncio.to_thread(lambda: pull_request.user.login) - assignee_flag = f" -a {shlex.quote(pr_author)}" - else: - assignee_flag = "" + assignee_flag = f" -a {shlex.quote(reviewed_user)}" new_branch_name = f"{CHERRY_PICKED_LABEL}-{pull_request.head.ref}-{shortuuid.uuid()[:5]}" if not await self.is_branch_exists(branch=target_branch): diff --git a/webhook_server/tests/test_config_schema.py b/webhook_server/tests/test_config_schema.py index d7246826..9e0e95b8 100644 --- a/webhook_server/tests/test_config_schema.py +++ b/webhook_server/tests/test_config_schema.py @@ -41,7 +41,6 @@ def valid_full_config(self) -> dict[str, Any]: "docker": {"username": "dockeruser", "password": "dockerpass"}, # pragma: allowlist secret "default-status-checks": ["WIP", "build"], "auto-verified-and-merged-users": ["bot[bot]"], - "cherry-pick-assign-to-pr-author": True, "branch-protection": { "strict": True, "require_code_owner_reviews": True, @@ -76,7 +75,6 @@ def valid_full_config(self) -> dict[str, Any]: "github-tokens": ["repo-token"], "branch-protection": {"strict": False, "required_approving_review_count": 1}, "set-auto-merge-prs": ["main"], - "cherry-pick-assign-to-pr-author": True, "can-be-merged-required-labels": ["ready"], "allow-commands-on-draft-prs": ["wip", "hold"], "conventional-title": "feat,fix,docs", diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index f1771f0b..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( @@ -288,7 +302,7 @@ async def test_process_pull_request_webhook_data_closed_action_merged( mock_cherry_pick.assert_called_once_with( pull_request=mock_pull_request, target_branch="branch1", - reviewed_user="owner1", + reviewed_user="cherry-pick-requester", by_label=True, ) mock_build.assert_called_once_with( @@ -299,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 cb0e7c7b..da236302 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -55,7 +55,6 @@ def mock_github_webhook(self) -> Mock: mock_webhook.container_command_args = [] mock_webhook.ctx = None mock_webhook.custom_check_runs = [] - mock_webhook.cherry_pick_assign_to_pr_author = False return mock_webhook @pytest.fixture @@ -80,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 @@ -659,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 @@ -675,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 @@ -696,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() @@ -718,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() @@ -925,7 +930,9 @@ 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() @@ -935,12 +942,9 @@ async def test_cherry_pick_manual_needed(self, runner_handler, mock_pull_request async def cherry_pick_setup( runner_handler: RunnerHandler, mock_pull_request: Mock, - *, - assign_to_author: bool = False, ): """Common setup for cherry-pick tests.""" runner_handler.github_webhook.pypi = {"token": "dummy"} - runner_handler.github_webhook.cherry_pick_assign_to_pr_author = assign_to_author 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: @@ -968,50 +972,26 @@ async def cherry_pick_setup( ) @pytest.mark.asyncio - async def test_cherry_pick_assigns_to_pr_author( + async def test_cherry_pick_assigns_reviewed_user( self, runner_handler: RunnerHandler, mock_pull_request: Mock ) -> None: - """Test cherry_pick assigns to the original PR author, not the requester.""" - mock_pull_request.user = Mock() - mock_pull_request.user.login = "original-pr-author" - async with self.cherry_pick_setup(runner_handler, mock_pull_request, assign_to_author=True) as mocks: + """Test cherry_pick assigns to reviewed_user (the requester or PR author passed by caller).""" + 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 'original-pr-author'" in hub_command or "-a original-pr-author" in hub_command - - @pytest.mark.asyncio - async def test_cherry_pick_always_assigns_to_pr_author_when_flag_set( - self, runner_handler: RunnerHandler, mock_pull_request: Mock - ) -> None: - """Test cherry_pick always uses pull_request.user.login as assignee. - - When cherry_pick_assign_to_pr_author is True, regardless of reviewed_user. - """ - mock_pull_request.user = Mock() - mock_pull_request.user.login = "pr-author-login" - async with self.cherry_pick_setup(runner_handler, mock_pull_request, assign_to_author=True) as mocks: - await runner_handler.cherry_pick(mock_pull_request, "main", reviewed_user="") - mocks.set_progress.assert_called_once() - mocks.set_success.assert_called_once() - mocks.comment.assert_called_once() - assert mocks.to_thread.call_count == 2 + assert mocks.to_thread.call_count == 1 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 'pr-author-login'" in hub_command or "-a pr-author-login" in hub_command + assert "-a 'cherry-requester'" in hub_command or "-a cherry-requester" 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.""" - mock_pull_request.user = Mock() - mock_pull_request.user.login = "pr-author-login" - async with self.cherry_pick_setup(runner_handler, mock_pull_request, assign_to_author=True) as mocks: + 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() @@ -1020,38 +1000,9 @@ async def test_cherry_pick_by_label_requested_by_format( 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 'label-requester'" in hub_command or "-a label-requester" in hub_command - # Only 1 call (create_issue_comment) - user.login skipped via by_label optimization + # Only 1 call (create_issue_comment) - no user.login API call needed assert mocks.to_thread.call_count == 1 - @pytest.mark.asyncio - async def test_cherry_pick_by_label_empty_reviewed_user_requested_by_format( - self, runner_handler: RunnerHandler, mock_pull_request: Mock - ) -> None: - """Test cherry_pick by_label with empty reviewed_user produces clean requested-by string.""" - async with self.cherry_pick_setup(runner_handler, mock_pull_request) as mocks: - await runner_handler.cherry_pick(mock_pull_request, "main", reviewed_user="", 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 target-branch label" in hub_command - assert "by with" not in hub_command - - @pytest.mark.asyncio - async def test_cherry_pick_disabled_no_assignee( - self, runner_handler: RunnerHandler, mock_pull_request: Mock - ) -> None: - """Test cherry_pick does not include -a flag when cherry_pick_assign_to_pr_author is False.""" - 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() - 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 " not in hub_command - @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 1784e9bf..c00b578a 100644 --- a/webhook_server/tests/test_schema_validator.py +++ b/webhook_server/tests/test_schema_validator.py @@ -73,7 +73,6 @@ def _validate_root_fields(self, config: dict[str, Any]) -> None: "disable-ssl-warnings", "mask-sensitive-data", "auto-verify-cherry-picked-prs", - "cherry-pick-assign-to-pr-author", ] for field in boolean_fields: if field in config and not isinstance(config[field], bool): @@ -172,7 +171,6 @@ def _validate_single_repository(self, repo_name: str, repo_config: Any) -> None: "pre-commit", "mask-sensitive-data", "auto-verify-cherry-picked-prs", - "cherry-pick-assign-to-pr-author", ] for field in boolean_fields: if field in repo_config and not isinstance(repo_config[field], bool): From 033f90e423f75654f7a55774ce436d09be4a59cf Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 24 Feb 2026 19:33:28 +0200 Subject: [PATCH 10/11] fix(cherry-pick): assign PR to original author instead of requester Always use pull_request.user.login as the cherry-pick PR assignee rather than the user who requested the cherry-pick. Wrap the user.login access with asyncio.to_thread to avoid blocking. Collapse redundant conditional in by_label branch. Update tests to assert PR author as assignee and correct to_thread call counts. --- webhook_server/libs/handlers/runner_handler.py | 11 +++++------ webhook_server/tests/test_runner_handler.py | 15 ++++++--------- 2 files changed, 11 insertions(+), 15 deletions(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 3076131d..9ea21658 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -545,20 +545,19 @@ async def cherry_pick( 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. + 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: - if reviewed_user: - requested_by = f"by {reviewed_user} with target-branch label" - else: - requested_by = "by target-branch 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}") - assignee_flag = f" -a {shlex.quote(reviewed_user)}" + 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): diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index da236302..4c5e7fc7 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -972,19 +972,17 @@ async def cherry_pick_setup( ) @pytest.mark.asyncio - async def test_cherry_pick_assigns_reviewed_user( - self, runner_handler: RunnerHandler, mock_pull_request: Mock - ) -> None: - """Test cherry_pick assigns to reviewed_user (the requester or PR author passed by caller).""" + 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 == 1 + 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 'cherry-requester'" in hub_command or "-a cherry-requester" in hub_command + 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( @@ -999,9 +997,8 @@ async def test_cherry_pick_by_label_requested_by_format( 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 'label-requester'" in hub_command or "-a label-requester" in hub_command - # Only 1 call (create_issue_comment) - no user.login API call needed - assert mocks.to_thread.call_count == 1 + 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( From 9533625d566cf3deb1f6d59fcd3357dcae55044f Mon Sep 17 00:00:00 2001 From: rnetser Date: Tue, 24 Feb 2026 19:39:55 +0200 Subject: [PATCH 11/11] refactor(cherry-pick): use walrus operator for cherry-pick label filtering Combine list comprehension assignment and truthiness check into a single walrus operator expression. --- webhook_server/libs/handlers/pull_request_handler.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index a2b2fd6a..dba1eb44 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -127,8 +127,9 @@ 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)) - cherry_pick_labels = [_label for _label in labels if _label.name.startswith(CHERRY_PICK_LABEL_PREFIX)] - if cherry_pick_labels: + 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)