From 6d6cec221fbeca07f36456cf3565d0cc2dbb78f9 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 9 Jun 2026 12:40:31 +0300 Subject: [PATCH 01/17] feat: cherry-pick retry/rebase commands + pre-commit auto-fix + security hardening - Add /cherry-pick-retry command for retrying failed cherry-picks (#1103) - Add /rebase command for rebasing PRs onto base branch (#1103) - Add pre-commit auto-fix after cherry-pick before push (#1089) - Add is_user_valid_to_run_commands guard to all unguarded commands - Verify bot ownership before closing cherry-pick PR on retry - Update welcome message with new commands Closes #1103 Closes #1089 --- .../libs/handlers/issue_comment_handler.py | 153 ++++++ .../libs/handlers/pull_request_handler.py | 6 +- .../libs/handlers/runner_handler.py | 200 +++++++ .../tests/test_issue_comment_handler.py | 510 ++++++++++++++++++ webhook_server/tests/test_runner_handler.py | 418 ++++++++++++++ webhook_server/utils/constants.py | 2 + 6 files changed, 1288 insertions(+), 1 deletion(-) diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index 8796aefa2..e704faf9d 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -24,7 +24,9 @@ COMMAND_ASSIGN_REVIEWER_STR, COMMAND_ASSIGN_REVIEWERS_STR, COMMAND_CHECK_CAN_MERGE_STR, + COMMAND_CHERRY_PICK_RETRY_STR, COMMAND_CHERRY_PICK_STR, + COMMAND_REBASE_STR, COMMAND_REGENERATE_WELCOME_STR, COMMAND_REPROCESS_STR, COMMAND_RETEST_STR, @@ -160,6 +162,8 @@ async def user_commands( COMMAND_RETEST_STR, COMMAND_REPROCESS_STR, COMMAND_CHERRY_PICK_STR, + COMMAND_CHERRY_PICK_RETRY_STR, + COMMAND_REBASE_STR, COMMAND_ASSIGN_REVIEWERS_STR, COMMAND_CHECK_CAN_MERGE_STR, BUILD_AND_PUSH_CONTAINER_STR, @@ -219,6 +223,7 @@ async def user_commands( COMMAND_RETEST_STR, COMMAND_ASSIGN_REVIEWER_STR, COMMAND_ADD_ALLOWED_USER_STR, + COMMAND_CHERRY_PICK_RETRY_STR, ) and not _args ): @@ -253,6 +258,10 @@ async def user_commands( self.logger.debug(f"{self.log_prefix} Added reaction to comment.") if _command == COMMAND_ASSIGN_REVIEWER_STR: + if not await self.owners_file_handler.is_user_valid_to_run_commands( + pull_request=pull_request, reviewed_user=reviewed_user + ): + return await self._add_reviewer_by_user_comment(pull_request=pull_request, reviewer=_args) elif _command == COMMAND_ADD_ALLOWED_USER_STR: @@ -264,16 +273,44 @@ async def user_commands( ) elif _command == COMMAND_ASSIGN_REVIEWERS_STR: + if not await self.owners_file_handler.is_user_valid_to_run_commands( + pull_request=pull_request, reviewed_user=reviewed_user + ): + return await self.owners_file_handler.assign_reviewers(pull_request=pull_request) elif _command == COMMAND_CHECK_CAN_MERGE_STR: + if not await self.owners_file_handler.is_user_valid_to_run_commands( + pull_request=pull_request, reviewed_user=reviewed_user + ): + return await self.pull_request_handler.check_if_can_be_merged(pull_request=pull_request) elif _command == COMMAND_CHERRY_PICK_STR: + if not await self.owners_file_handler.is_user_valid_to_run_commands( + pull_request=pull_request, reviewed_user=reviewed_user + ): + return await self.process_cherry_pick_command( pull_request=pull_request, command_args=_args, reviewed_user=reviewed_user ) + elif _command == COMMAND_CHERRY_PICK_RETRY_STR: + if not await self.owners_file_handler.is_user_valid_to_run_commands( + pull_request=pull_request, reviewed_user=reviewed_user + ): + return + await self.process_cherry_pick_retry_command( + pull_request=pull_request, command_args=_args, reviewed_user=reviewed_user + ) + + elif _command == COMMAND_REBASE_STR: + if not await self.owners_file_handler.is_user_valid_to_run_commands( + pull_request=pull_request, reviewed_user=reviewed_user + ): + return + await self.runner_handler.rebase_pr(pull_request=pull_request, reviewed_user=reviewed_user) + elif _command == COMMAND_RETEST_STR: await self.process_retest_command( pull_request=pull_request, command_args=_args, reviewed_user=reviewed_user @@ -295,6 +332,10 @@ async def user_commands( await self.pull_request_handler.regenerate_welcome_message(pull_request=pull_request) elif _command == COMMAND_TEST_ORACLE_STR: + if not await self.owners_file_handler.is_user_valid_to_run_commands( + pull_request=pull_request, reviewed_user=reviewed_user + ): + return task = asyncio.create_task( call_test_oracle( github_webhook=self.github_webhook, @@ -322,6 +363,10 @@ async def user_commands( ) elif _command == WIP_STR: + if not await self.owners_file_handler.is_user_valid_to_run_commands( + pull_request=pull_request, reviewed_user=reviewed_user + ): + return wip_for_title: str = f"{WIP_STR.upper()}:" if remove: label_changed = await self.labels_handler._remove_label(pull_request=pull_request, label=WIP_STR) @@ -374,6 +419,10 @@ async def user_commands( await self.labels_handler._add_label(pull_request=pull_request, label=HOLD_LABEL_STR) elif _command == VERIFIED_LABEL_STR: + if not await self.owners_file_handler.is_user_valid_to_run_commands( + pull_request=pull_request, reviewed_user=reviewed_user + ): + return if remove: await self.labels_handler._remove_label(pull_request=pull_request, label=VERIFIED_LABEL_STR) await self.check_run_handler.set_check_queued(name=VERIFIED_LABEL_STR) @@ -558,6 +607,110 @@ async def process_cherry_pick_command( for _cp_label in cp_labels: await self.labels_handler._add_label(pull_request=pull_request, label=_cp_label) + async def process_cherry_pick_retry_command( + self, pull_request: PullRequest, command_args: str, reviewed_user: str + ) -> None: + """Process cherry-pick-retry command on a merged PR. + + Retries a cherry-pick for a specific branch when the original cherry-pick + failed or the cherry-pick PR has issues. Only works on merged PRs where + the cherry-pick label already exists. + + Steps: + 1. Validate the PR is merged + 2. Validate the cherry-pick label exists (this is a retry, not a new cherry-pick) + 3. Close any existing failed cherry-pick PR for that branch (created by bot) + 4. Re-run cherry_pick() for the target branch + + Args: + pull_request: The original merged pull request + command_args: Target branch name to retry cherry-pick for + reviewed_user: User who requested the retry + """ + target_branch = command_args.strip() + self.logger.info(f"{self.log_prefix} Processing cherry-pick retry for branch {target_branch}") + + # Validate PR is merged + if not self.hook_data["issue"].get("pull_request", {}).get("merged_at"): + msg = "Cherry-pick retry can only be used on merged PRs" + self.logger.debug(f"{self.log_prefix} {msg}") + await github_api_call( + pull_request.create_issue_comment, msg, logger=self.logger, log_prefix=self.log_prefix + ) + return + + # Validate the cherry-pick label exists (this is a retry, not a new cherry-pick) + cherry_pick_label = f"{CHERRY_PICK_LABEL_PREFIX}{target_branch}" + existing_labels = { + label.name + for label in await github_api_call( + lambda: list(pull_request.labels), logger=self.logger, log_prefix=self.log_prefix + ) + } + if cherry_pick_label not in existing_labels: + msg = ( + f"Cherry-pick label `{cherry_pick_label}` not found on this PR.\n" + f"Use `/{COMMAND_CHERRY_PICK_STR} {target_branch}` to create a new cherry-pick." + ) + self.logger.debug(f"{self.log_prefix} {msg}") + await github_api_call( + pull_request.create_issue_comment, msg, logger=self.logger, log_prefix=self.log_prefix + ) + return + + # Find and close existing failed cherry-pick PR for this branch (created by bot) + pr_title_prefix = f"CherryPicked: [{target_branch}]" + open_pulls = await github_api_call( + lambda: list(self.repository.get_pulls(state="open")), + logger=self.logger, + log_prefix=self.log_prefix, + ) + for open_pr in open_pulls: + pr_title = await github_api_call( + lambda _pr=open_pr: _pr.title, logger=self.logger, log_prefix=self.log_prefix + ) + if pr_title.startswith(pr_title_prefix): + # Verify the PR was created by a bot (not a human-created PR) + pr_author = await github_api_call( + lambda _pr=open_pr: _pr.user.login, logger=self.logger, log_prefix=self.log_prefix + ) + if pr_author not in self.github_webhook.auto_verified_and_merged_users: + continue # Skip non-bot PRs + + # Check if the PR body references the original PR + pr_body = await github_api_call( + lambda _pr=open_pr: _pr.body or "", logger=self.logger, log_prefix=self.log_prefix + ) + if pull_request.html_url in pr_body: + self.logger.info(f"{self.log_prefix} Closing existing cherry-pick PR #{open_pr.number} for retry") + await github_api_call( + open_pr.edit, + state="closed", + logger=self.logger, + log_prefix=self.log_prefix, + ) + await github_api_call( + open_pr.create_issue_comment, + f"Closed by cherry-pick retry requested by @{reviewed_user} on {pull_request.html_url}", + logger=self.logger, + log_prefix=self.log_prefix, + ) + break + + # Re-run cherry-pick for the target branch + self.logger.info(f"{self.log_prefix} Retrying cherry-pick to {target_branch}") + await github_api_call( + pull_request.create_issue_comment, + f"Retrying cherry-pick to `{target_branch}` requested by @{reviewed_user}", + logger=self.logger, + log_prefix=self.log_prefix, + ) + await self.runner_handler.cherry_pick( + pull_request=pull_request, + target_branch=target_branch, + assign_to_pr_owner=self.github_webhook.cherry_pick_assign_to_pr_author, + ) + async def process_retest_command( self, pull_request: PullRequest, command_args: str, reviewed_user: str, automerge: bool = False ) -> None: diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 0f7002336..b7c77a4d6 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -795,8 +795,12 @@ def _prepare_cherry_pick_section(self) -> str: return """#### Cherry-pick Operations * `/cherry-pick ` - Schedule cherry-pick to target branch when PR is merged * Multiple branches: `/cherry-pick branch1 branch2 branch3` +* `/cherry-pick-retry ` - Retry a failed cherry-pick (merged PRs only) + +#### Branch Management +* `/rebase` - Rebase this PR branch onto its base branch """ - return "" + return "\n#### Branch Management\n* `/rebase` - Rebase this PR branch onto its base branch\n" async def label_all_opened_pull_requests_merge_state_after_merged(self) -> None: """ diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 6ad743796..6dfb9f1c3 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -1261,6 +1261,54 @@ async def cherry_pick( github_token=github_token, ) + # Run pre-commit auto-fix if enabled for the repo + if self.github_webhook.pre_commit: + self.logger.info(f"{self.log_prefix} Running pre-commit on cherry-pick worktree") + pre_commit_cmd = f"uvx --directory {worktree_path} {PREK_STR} run --all-files" + rc_pc, _out_pc, _err_pc = await run_command( + command=pre_commit_cmd, + log_prefix=self.log_prefix, + redact_secrets=[github_token], + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc_pc: + # Pre-commit returned non-zero — check if any files were + # actually modified before committing. + rc_diff, out_diff, _ = await run_command( + command=f"{git_cmd} diff --name-only", + log_prefix=self.log_prefix, + redact_secrets=[github_token], + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if rc_diff and out_diff.strip(): + self.logger.info(f"{self.log_prefix} Pre-commit modified files, committing fixes") + rc_add, _, err_add = await run_command( + command=f"{git_cmd} add -A", + log_prefix=self.log_prefix, + redact_secrets=[github_token], + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc_add: + self.logger.warning(f"{self.log_prefix} git add failed after pre-commit fix: {err_add}") + rc_commit, _, err_commit = await run_command( + command=( + f"{git_cmd} commit -m" + f" {shlex.quote('pre-commit auto-fix for cherry-pick')}" + " --no-verify" + ), + log_prefix=self.log_prefix, + redact_secrets=[github_token], + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc_commit: + self.logger.warning( + f"{self.log_prefix} git commit failed after pre-commit fix: {err_commit}" + ) + else: + self.logger.debug(f"{self.log_prefix} Pre-commit failed but no files modified") + else: + self.logger.debug(f"{self.log_prefix} Pre-commit passed without modifications") + # Push the branch rc, out, err = await run_command( command=push_command, @@ -1484,6 +1532,158 @@ async def cherry_pick( log_prefix=self.log_prefix, ) + async def rebase_pr(self, pull_request: PullRequest, reviewed_user: str) -> None: + """Rebase a PR branch onto its base branch. + + Fetches the base branch and rebases the PR head branch onto it, + then force-pushes the result. For bot-owned PRs (e.g., cherry-pick PRs), + validates the user is the cherry-pick initiator (PR assignee) or a maintainer. + + Args: + pull_request: The pull request to rebase + reviewed_user: User who requested the rebase + """ + pr_state = await github_api_call(lambda: pull_request.state, logger=self.logger, log_prefix=self.log_prefix) + if pr_state != "open": + msg = "Rebase can only be used on open PRs" + self.logger.debug(f"{self.log_prefix} {msg}") + await github_api_call( + pull_request.create_issue_comment, msg, logger=self.logger, log_prefix=self.log_prefix + ) + return + + pr_user_login = await github_api_call( + lambda: pull_request.user.login, logger=self.logger, log_prefix=self.log_prefix + ) + + # Check if PR is bot-owned (e.g., cherry-pick PRs created by the app) + is_bot_pr = pr_user_login in self.github_webhook.auto_verified_and_merged_users + + if is_bot_pr: + # For bot-owned PRs, validate the user is the PR assignee or a maintainer + assignees = await github_api_call( + lambda: [a.login for a in pull_request.assignees], + logger=self.logger, + log_prefix=self.log_prefix, + ) + maintainers = await self.owners_file_handler.get_all_repository_maintainers() + if reviewed_user not in assignees and reviewed_user not in maintainers: + msg = ( + f"@{reviewed_user} is not authorized to rebase this bot-owned PR.\n" + "Only the PR assignee (cherry-pick initiator) or maintainers can rebase." + ) + self.logger.debug(f"{self.log_prefix} {msg}") + await github_api_call( + pull_request.create_issue_comment, msg, logger=self.logger, log_prefix=self.log_prefix + ) + return + + base_ref = await github_api_call(lambda: pull_request.base.ref, logger=self.logger, log_prefix=self.log_prefix) + head_ref = await github_api_call(lambda: pull_request.head.ref, logger=self.logger, log_prefix=self.log_prefix) + github_token = self.github_webhook.token + + self.logger.info(f"{self.log_prefix} Rebasing {head_ref} onto {base_ref}") + + async with self._checkout_worktree(pull_request=pull_request, skip_merge=True) as ( + success, + worktree_path, + out, + err, + ): + if not success: + msg = "Failed to prepare worktree for rebase" + self.logger.error(f"{self.log_prefix} {msg}: {out} --- {err}") + await github_api_call( + pull_request.create_issue_comment, msg, logger=self.logger, log_prefix=self.log_prefix + ) + return + + git_cmd = f"git --work-tree={worktree_path} --git-dir={worktree_path}/.git" + + # Checkout the PR head branch + rc, out, err = await run_command( + command=f"{git_cmd} checkout {head_ref}", + log_prefix=self.log_prefix, + redact_secrets=[github_token], + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc: + redacted_err = _redact_secrets(err, [github_token], mask_sensitive=self.github_webhook.mask_sensitive) + msg = f"Failed to checkout branch `{head_ref}`: {redacted_err}" + self.logger.error(f"{self.log_prefix} {msg}") + await github_api_call( + pull_request.create_issue_comment, msg, logger=self.logger, log_prefix=self.log_prefix + ) + return + + # Fetch the base branch + rc, out, err = await run_command( + command=f"{git_cmd} fetch origin {base_ref}", + log_prefix=self.log_prefix, + redact_secrets=[github_token], + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc: + redacted_err = _redact_secrets(err, [github_token], mask_sensitive=self.github_webhook.mask_sensitive) + msg = f"Failed to fetch base branch `{base_ref}`: {redacted_err}" + self.logger.error(f"{self.log_prefix} {msg}") + await github_api_call( + pull_request.create_issue_comment, msg, logger=self.logger, log_prefix=self.log_prefix + ) + return + + # Rebase onto base branch + rc, out, err = await run_command( + command=f"{git_cmd} rebase origin/{base_ref}", + log_prefix=self.log_prefix, + redact_secrets=[github_token], + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc: + # Abort the rebase to clean up + await run_command( + command=f"{git_cmd} rebase --abort", + log_prefix=self.log_prefix, + redact_secrets=[github_token], + mask_sensitive=self.github_webhook.mask_sensitive, + ) + redacted_err = _redact_secrets(err, [github_token], mask_sensitive=self.github_webhook.mask_sensitive) + redacted_out = _redact_secrets(out, [github_token], mask_sensitive=self.github_webhook.mask_sensitive) + msg = ( + f"**Rebase failed** for `{head_ref}` onto `{base_ref}`:\n" + f"```\n{redacted_out}\n{redacted_err}\n```\n" + "Please resolve conflicts manually." + ) + self.logger.error(f"{self.log_prefix} Rebase failed: {redacted_out} --- {redacted_err}") + await github_api_call( + pull_request.create_issue_comment, msg, logger=self.logger, log_prefix=self.log_prefix + ) + return + + # Force push the rebased branch + rc, out, err = await run_command( + command=f"{git_cmd} push --force-with-lease origin {head_ref}", + log_prefix=self.log_prefix, + redact_secrets=[github_token], + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc: + redacted_err = _redact_secrets(err, [github_token], mask_sensitive=self.github_webhook.mask_sensitive) + msg = f"Rebase succeeded but push failed for `{head_ref}`: {redacted_err}" + self.logger.error(f"{self.log_prefix} {msg}") + await github_api_call( + pull_request.create_issue_comment, msg, logger=self.logger, log_prefix=self.log_prefix + ) + return + + self.logger.info(f"{self.log_prefix} Successfully rebased {head_ref} onto {base_ref}") + await github_api_call( + pull_request.create_issue_comment, + f"Successfully rebased `{head_ref}` onto `{base_ref}` ✅", + logger=self.logger, + log_prefix=self.log_prefix, + ) + async def run_retests(self, supported_retests: list[str], pull_request: PullRequest) -> None: """Run the specified retests for a pull request. diff --git a/webhook_server/tests/test_issue_comment_handler.py b/webhook_server/tests/test_issue_comment_handler.py index 739168b98..2dac3bd3c 100644 --- a/webhook_server/tests/test_issue_comment_handler.py +++ b/webhook_server/tests/test_issue_comment_handler.py @@ -12,7 +12,9 @@ COMMAND_ASSIGN_REVIEWER_STR, COMMAND_ASSIGN_REVIEWERS_STR, COMMAND_CHECK_CAN_MERGE_STR, + COMMAND_CHERRY_PICK_RETRY_STR, COMMAND_CHERRY_PICK_STR, + COMMAND_REBASE_STR, COMMAND_REGENERATE_WELCOME_STR, COMMAND_REPROCESS_STR, COMMAND_RETEST_STR, @@ -1733,3 +1735,511 @@ async def test_approve_cancel_does_not_call_test_oracle(self, issue_comment_hand ) mock_oracle.assert_not_called() mock_create_task.assert_not_called() + + # --- Permission guard tests for newly guarded commands --- + + @pytest.mark.asyncio + async def test_cherry_pick_command_unauthorized_user(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test /cherry-pick command is blocked for unauthorized users.""" + mock_pull_request = Mock() + mock_is_valid = AsyncMock(return_value=False) + + with ( + patch.object( + issue_comment_handler.owners_file_handler, + "is_user_valid_to_run_commands", + new=mock_is_valid, + ), + patch.object( + issue_comment_handler, + "process_cherry_pick_command", + new=AsyncMock(), + ) as mock_cherry_pick, + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=f"{COMMAND_CHERRY_PICK_STR} branch1", + reviewed_user="unauthorized-user", + issue_comment_id=123, + is_draft=False, + ) + mock_is_valid.assert_awaited_once_with( + pull_request=mock_pull_request, + reviewed_user="unauthorized-user", + ) + mock_cherry_pick.assert_not_awaited() + + @pytest.mark.asyncio + async def test_assign_reviewer_command_unauthorized_user(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test /assign-reviewer command is blocked for unauthorized users.""" + mock_pull_request = Mock() + mock_is_valid = AsyncMock(return_value=False) + + with ( + patch.object( + issue_comment_handler.owners_file_handler, + "is_user_valid_to_run_commands", + new=mock_is_valid, + ), + patch.object( + issue_comment_handler, + "_add_reviewer_by_user_comment", + new=AsyncMock(), + ) as mock_add_reviewer, + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=f"{COMMAND_ASSIGN_REVIEWER_STR} @someuser", + reviewed_user="unauthorized-user", + issue_comment_id=123, + is_draft=False, + ) + mock_is_valid.assert_awaited_once() + mock_add_reviewer.assert_not_awaited() + + @pytest.mark.asyncio + async def test_assign_reviewers_command_unauthorized_user(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test /assign-reviewers command is blocked for unauthorized users.""" + mock_pull_request = Mock() + mock_is_valid = AsyncMock(return_value=False) + + with ( + patch.object( + issue_comment_handler.owners_file_handler, + "is_user_valid_to_run_commands", + new=mock_is_valid, + ), + patch.object( + issue_comment_handler.owners_file_handler, + "assign_reviewers", + new=AsyncMock(), + ) as mock_assign, + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=COMMAND_ASSIGN_REVIEWERS_STR, + reviewed_user="unauthorized-user", + issue_comment_id=123, + is_draft=False, + ) + mock_is_valid.assert_awaited_once() + mock_assign.assert_not_awaited() + + @pytest.mark.asyncio + async def test_check_can_merge_command_unauthorized_user(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test /check-can-merge command is blocked for unauthorized users.""" + mock_pull_request = Mock() + mock_is_valid = AsyncMock(return_value=False) + + with ( + patch.object( + issue_comment_handler.owners_file_handler, + "is_user_valid_to_run_commands", + new=mock_is_valid, + ), + patch.object( + issue_comment_handler.pull_request_handler, + "check_if_can_be_merged", + new=AsyncMock(), + ) as mock_check, + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=COMMAND_CHECK_CAN_MERGE_STR, + reviewed_user="unauthorized-user", + issue_comment_id=123, + is_draft=False, + ) + mock_is_valid.assert_awaited_once() + mock_check.assert_not_awaited() + + @pytest.mark.asyncio + async def test_verified_command_unauthorized_user(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test /verified command is blocked for unauthorized users.""" + mock_pull_request = Mock() + mock_is_valid = AsyncMock(return_value=False) + + with ( + patch.object( + issue_comment_handler.owners_file_handler, + "is_user_valid_to_run_commands", + new=mock_is_valid, + ), + patch.object( + issue_comment_handler.labels_handler, + "_add_label", + new=AsyncMock(), + ) as mock_add_label, + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=VERIFIED_LABEL_STR, + reviewed_user="unauthorized-user", + issue_comment_id=123, + is_draft=False, + ) + mock_is_valid.assert_awaited_once() + mock_add_label.assert_not_awaited() + + @pytest.mark.asyncio + async def test_wip_command_unauthorized_user(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test /wip command is blocked for unauthorized users.""" + mock_pull_request = Mock() + mock_is_valid = AsyncMock(return_value=False) + + with ( + patch.object( + issue_comment_handler.owners_file_handler, + "is_user_valid_to_run_commands", + new=mock_is_valid, + ), + patch.object( + issue_comment_handler.labels_handler, + "_add_label", + new=AsyncMock(), + ) as mock_add_label, + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=WIP_STR, + reviewed_user="unauthorized-user", + issue_comment_id=123, + is_draft=False, + ) + mock_is_valid.assert_awaited_once() + mock_add_label.assert_not_awaited() + + @pytest.mark.asyncio + async def test_test_oracle_command_unauthorized_user(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test /test-oracle command is blocked for unauthorized users.""" + mock_pull_request = Mock() + mock_is_valid = AsyncMock(return_value=False) + + with ( + patch.object( + issue_comment_handler.owners_file_handler, + "is_user_valid_to_run_commands", + new=mock_is_valid, + ), + patch( + "webhook_server.libs.handlers.issue_comment_handler.call_test_oracle", + new_callable=AsyncMock, + ) as mock_oracle, + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=COMMAND_TEST_ORACLE_STR, + reviewed_user="unauthorized-user", + issue_comment_id=456, + is_draft=False, + ) + mock_is_valid.assert_awaited_once() + mock_oracle.assert_not_called() + + # --- Cherry-pick retry command tests --- + + @pytest.mark.asyncio + async def test_cherry_pick_retry_command_dispatched(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test /cherry-pick-retry command dispatches to process_cherry_pick_retry_command.""" + mock_pull_request = Mock() + + with ( + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + patch.object( + issue_comment_handler, + "process_cherry_pick_retry_command", + new_callable=AsyncMock, + ) as mock_retry, + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=f"{COMMAND_CHERRY_PICK_RETRY_STR} release-1.0", + reviewed_user="test-user", + issue_comment_id=123, + is_draft=False, + ) + mock_retry.assert_awaited_once_with( + pull_request=mock_pull_request, + command_args="release-1.0", + reviewed_user="test-user", + ) + + @pytest.mark.asyncio + async def test_cherry_pick_retry_missing_args(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test /cherry-pick-retry without args posts error comment.""" + mock_pull_request = Mock() + + with ( + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + patch("asyncio.to_thread", new_callable=AsyncMock, side_effect=lambda f, *a, **k: f(*a, **k)), + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=COMMAND_CHERRY_PICK_RETRY_STR, + reviewed_user="test-user", + issue_comment_id=123, + is_draft=False, + ) + mock_pull_request.create_issue_comment.assert_called_once() + call_body = mock_pull_request.create_issue_comment.call_args + assert "requires an argument" in str(call_body) + + @pytest.mark.asyncio + async def test_cherry_pick_retry_unauthorized_user(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test /cherry-pick-retry command is blocked for unauthorized users.""" + mock_pull_request = Mock() + mock_is_valid = AsyncMock(return_value=False) + + with ( + patch.object( + issue_comment_handler.owners_file_handler, + "is_user_valid_to_run_commands", + new=mock_is_valid, + ), + patch.object( + issue_comment_handler, + "process_cherry_pick_retry_command", + new=AsyncMock(), + ) as mock_retry, + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=f"{COMMAND_CHERRY_PICK_RETRY_STR} branch1", + reviewed_user="unauthorized-user", + issue_comment_id=123, + is_draft=False, + ) + mock_is_valid.assert_awaited_once() + mock_retry.assert_not_awaited() + + @pytest.mark.asyncio + async def test_process_cherry_pick_retry_not_merged(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test cherry-pick-retry rejects if PR is not merged.""" + mock_pull_request = Mock() + # PR not merged + issue_comment_handler.hook_data["issue"] = {"number": 123, "pull_request": {}} + + with patch("asyncio.to_thread", new_callable=AsyncMock, side_effect=lambda f, *a, **k: f(*a, **k)): + await issue_comment_handler.process_cherry_pick_retry_command( + pull_request=mock_pull_request, + command_args="release-1.0", + reviewed_user="test-user", + ) + mock_pull_request.create_issue_comment.assert_called_once() + call_body = str(mock_pull_request.create_issue_comment.call_args) + assert "merged" in call_body.lower() + + @pytest.mark.asyncio + async def test_process_cherry_pick_retry_label_missing(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test cherry-pick-retry rejects if the cherry-pick label is missing.""" + mock_pull_request = Mock() + mock_pull_request.labels = [] # No labels + # PR is merged + issue_comment_handler.hook_data["issue"] = { + "number": 123, + "pull_request": {"merged_at": "2024-01-01T00:00:00Z"}, + } + + with patch("asyncio.to_thread", new_callable=AsyncMock, side_effect=lambda f, *a, **k: f(*a, **k)): + await issue_comment_handler.process_cherry_pick_retry_command( + pull_request=mock_pull_request, + command_args="release-1.0", + reviewed_user="test-user", + ) + assert mock_pull_request.create_issue_comment.call_count == 1 + call_body = str(mock_pull_request.create_issue_comment.call_args) + assert f"{CHERRY_PICK_LABEL_PREFIX}release-1.0" in call_body + + @pytest.mark.asyncio + async def test_process_cherry_pick_retry_success(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test cherry-pick-retry closes old PR and re-runs cherry-pick.""" + mock_pull_request = Mock() + mock_pull_request.html_url = "https://github.com/test/repo/pull/123" + # Has the cherry-pick label + mock_label = Mock() + mock_label.name = f"{CHERRY_PICK_LABEL_PREFIX}release-1.0" + mock_pull_request.labels = [mock_label] + + # PR is merged + issue_comment_handler.hook_data["issue"] = { + "number": 123, + "pull_request": {"merged_at": "2024-01-01T00:00:00Z"}, + } + + # Mock existing cherry-pick PR to close (created by bot) + mock_existing_cp_pr = Mock() + mock_existing_cp_pr.number = 456 + mock_existing_cp_pr.title = "CherryPicked: [release-1.0] Some commit" + mock_existing_cp_pr.body = "Cherry-pick from main, original PR: https://github.com/test/repo/pull/123" + mock_existing_cp_pr.user = Mock() + mock_existing_cp_pr.user.login = "bot-user[bot]" + mock_existing_cp_pr.edit = Mock() + mock_existing_cp_pr.create_issue_comment = Mock() + + issue_comment_handler.github_webhook.auto_verified_and_merged_users = ["bot-user[bot]"] + issue_comment_handler.repository.get_pulls = Mock(return_value=[mock_existing_cp_pr]) + + with ( + patch("asyncio.to_thread", new_callable=AsyncMock, side_effect=lambda f, *a, **k: f(*a, **k)), + patch.object( + issue_comment_handler.runner_handler, + "cherry_pick", + new_callable=AsyncMock, + ) as mock_cherry_pick, + ): + await issue_comment_handler.process_cherry_pick_retry_command( + pull_request=mock_pull_request, + command_args="release-1.0", + reviewed_user="test-user", + ) + # Old PR should be closed + mock_existing_cp_pr.edit.assert_called_once_with(state="closed") + # Cherry-pick should be re-run + mock_cherry_pick.assert_awaited_once_with( + pull_request=mock_pull_request, + target_branch="release-1.0", + assign_to_pr_owner=issue_comment_handler.github_webhook.cherry_pick_assign_to_pr_author, + ) + + @pytest.mark.asyncio + async def test_process_cherry_pick_retry_no_existing_pr(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test cherry-pick-retry succeeds even when there's no existing cherry-pick PR to close.""" + mock_pull_request = Mock() + mock_pull_request.html_url = "https://github.com/test/repo/pull/123" + mock_label = Mock() + mock_label.name = f"{CHERRY_PICK_LABEL_PREFIX}release-1.0" + mock_pull_request.labels = [mock_label] + + issue_comment_handler.hook_data["issue"] = { + "number": 123, + "pull_request": {"merged_at": "2024-01-01T00:00:00Z"}, + } + + # No open PRs matching + issue_comment_handler.repository.get_pulls = Mock(return_value=[]) + + with ( + patch("asyncio.to_thread", new_callable=AsyncMock, side_effect=lambda f, *a, **k: f(*a, **k)), + patch.object( + issue_comment_handler.runner_handler, + "cherry_pick", + new_callable=AsyncMock, + ) as mock_cherry_pick, + ): + await issue_comment_handler.process_cherry_pick_retry_command( + pull_request=mock_pull_request, + command_args="release-1.0", + reviewed_user="test-user", + ) + # Cherry-pick should still be re-run + mock_cherry_pick.assert_awaited_once() + + @pytest.mark.asyncio + async def test_process_cherry_pick_retry_skips_human_created_pr( + self, issue_comment_handler: IssueCommentHandler + ) -> None: + """Test cherry-pick-retry does NOT close a human-created PR matching the title.""" + mock_pull_request = Mock() + mock_pull_request.html_url = "https://github.com/test/repo/pull/123" + mock_label = Mock() + mock_label.name = f"{CHERRY_PICK_LABEL_PREFIX}release-1.0" + mock_pull_request.labels = [mock_label] + + issue_comment_handler.hook_data["issue"] = { + "number": 123, + "pull_request": {"merged_at": "2024-01-01T00:00:00Z"}, + } + + # Human-created PR with matching title (NOT a bot) + mock_human_pr = Mock() + mock_human_pr.number = 789 + mock_human_pr.title = "CherryPicked: [release-1.0] Manual cherry-pick" + mock_human_pr.body = "original PR: https://github.com/test/repo/pull/123" + mock_human_pr.user = Mock() + mock_human_pr.user.login = "human-user" # Not a bot + mock_human_pr.edit = Mock() + + issue_comment_handler.github_webhook.auto_verified_and_merged_users = ["bot-user[bot]"] + issue_comment_handler.repository.get_pulls = Mock(return_value=[mock_human_pr]) + + with ( + patch("asyncio.to_thread", new_callable=AsyncMock, side_effect=lambda f, *a, **k: f(*a, **k)), + patch.object( + issue_comment_handler.runner_handler, + "cherry_pick", + new_callable=AsyncMock, + ) as mock_cherry_pick, + ): + await issue_comment_handler.process_cherry_pick_retry_command( + pull_request=mock_pull_request, + command_args="release-1.0", + reviewed_user="test-user", + ) + # Human PR should NOT be closed + mock_human_pr.edit.assert_not_called() + # Cherry-pick should still be re-run + mock_cherry_pick.assert_awaited_once() + + # --- Rebase command tests --- + + @pytest.mark.asyncio + async def test_rebase_command_dispatched(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test /rebase command dispatches to runner_handler.rebase_pr.""" + mock_pull_request = Mock() + + with ( + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + patch.object( + issue_comment_handler.runner_handler, + "rebase_pr", + new_callable=AsyncMock, + ) as mock_rebase, + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=COMMAND_REBASE_STR, + reviewed_user="test-user", + issue_comment_id=123, + is_draft=False, + ) + mock_rebase.assert_awaited_once_with( + pull_request=mock_pull_request, + reviewed_user="test-user", + ) + + @pytest.mark.asyncio + async def test_rebase_command_unauthorized_user(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test /rebase command is blocked for unauthorized users.""" + mock_pull_request = Mock() + mock_is_valid = AsyncMock(return_value=False) + + with ( + patch.object( + issue_comment_handler.owners_file_handler, + "is_user_valid_to_run_commands", + new=mock_is_valid, + ), + patch.object( + issue_comment_handler.runner_handler, + "rebase_pr", + new_callable=AsyncMock, + ) as mock_rebase, + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=COMMAND_REBASE_STR, + reviewed_user="unauthorized-user", + issue_comment_id=123, + is_draft=False, + ) + mock_is_valid.assert_awaited_once() + mock_rebase.assert_not_awaited() diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 07e62542f..8c25be9a8 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -2883,3 +2883,421 @@ async def test_build_container_includes_oci_annotations( cmd_arg = mock_cmd.call_args[1].get("command", "") if mock_cmd.call_args[1] else mock_cmd.call_args[0][0] assert "--annotation" in cmd_arg, f"Expected --annotation in podman build command: {cmd_arg}" assert "org.opencontainers.image.source=https://github.com/test-org/test-repo" in cmd_arg + + +class TestRebasePr: + """Test suite for rebase_pr method.""" + + @pytest.fixture + def mock_github_webhook(self) -> Mock: + """Create a mock GithubWebhook instance.""" + mock_webhook = Mock() + mock_webhook.hook_data = {"action": "opened"} + mock_webhook.logger = Mock() + mock_webhook.log_prefix = "[TEST]" + mock_webhook.repository = Mock() + mock_webhook.repository.clone_url = "https://github.com/test/repo.git" + mock_webhook.token = "test-token" # pragma: allowlist secret + mock_webhook.clone_repo_dir = "/tmp/test-repo" + mock_webhook.mask_sensitive = False + mock_webhook.auto_verified_and_merged_users = ["bot-user[bot]"] + mock_webhook.ctx = None + mock_webhook.config = Mock() + mock_webhook.config.get_value = Mock(return_value=None) + mock_webhook.pre_commit = False + mock_webhook.tox = {} + mock_webhook.build_and_push_container = False + mock_webhook.pypi = {} + mock_webhook.conventional_title = "" + mock_webhook.custom_check_runs = [] + return mock_webhook + + @pytest.fixture + def mock_owners_file_handler(self) -> Mock: + mock_handler = Mock() + mock_handler.is_user_valid_to_run_commands = AsyncMock(return_value=True) + mock_handler.get_all_repository_maintainers = AsyncMock(return_value=["maintainer1"]) + return mock_handler + + @pytest.fixture + def runner_handler(self, mock_github_webhook: Mock, mock_owners_file_handler: Mock) -> RunnerHandler: + return RunnerHandler(mock_github_webhook, mock_owners_file_handler) + + @pytest.fixture + def mock_pull_request(self) -> Mock: + mock_pr = Mock() + mock_pr.number = 123 + mock_pr.state = "open" + mock_pr.base.ref = "main" + mock_pr.head.ref = "feature-branch" + mock_pr.user = Mock() + mock_pr.user.login = "test-user" + mock_pr.assignees = [] + mock_pr.create_issue_comment = Mock() + return mock_pr + + @pytest.mark.asyncio + async def test_rebase_pr_not_open(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: + """Test rebase_pr rejects closed PRs.""" + mock_pull_request.state = "closed" + 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.rebase_pr(mock_pull_request, reviewed_user="test-user") + mock_pull_request.create_issue_comment.assert_called_once() + call_body = str(mock_pull_request.create_issue_comment.call_args) + assert "open" in call_body.lower() + + @pytest.mark.asyncio + async def test_rebase_pr_bot_pr_unauthorized(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: + """Test rebase_pr rejects unauthorized users on bot-owned PRs.""" + mock_pull_request.user.login = "bot-user[bot]" + mock_pull_request.assignees = [Mock(login="original-author")] + + 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.rebase_pr(mock_pull_request, reviewed_user="random-user") + mock_pull_request.create_issue_comment.assert_called_once() + call_body = str(mock_pull_request.create_issue_comment.call_args) + assert "not authorized" in call_body.lower() + + @pytest.mark.asyncio + async def test_rebase_pr_bot_pr_assignee_allowed( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test rebase_pr allows PR assignee on bot-owned PRs.""" + mock_pull_request.user.login = "bot-user[bot]" + mock_pull_request.assignees = [Mock(login="test-user")] + + with ( + patch( + "asyncio.to_thread", new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()) + ), + patch.object(runner_handler, "_checkout_worktree") as mock_checkout, + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(return_value=(True, "success", "")), + ), + ): + 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) + + await runner_handler.rebase_pr(mock_pull_request, reviewed_user="test-user") + # Should succeed - verify success comment + assert any( + "successfully" in str(call).lower() for call in mock_pull_request.create_issue_comment.call_args_list + ) + + @pytest.mark.asyncio + async def test_rebase_pr_bot_pr_maintainer_allowed( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test rebase_pr allows maintainers on bot-owned PRs.""" + mock_pull_request.user.login = "bot-user[bot]" + mock_pull_request.assignees = [Mock(login="original-author")] + + with ( + patch( + "asyncio.to_thread", new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()) + ), + patch.object(runner_handler, "_checkout_worktree") as mock_checkout, + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(return_value=(True, "success", "")), + ), + ): + 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) + + await runner_handler.rebase_pr(mock_pull_request, reviewed_user="maintainer1") + assert any( + "successfully" in str(call).lower() for call in mock_pull_request.create_issue_comment.call_args_list + ) + + @pytest.mark.asyncio + async def test_rebase_pr_success(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: + """Test successful rebase operation.""" + with ( + patch( + "asyncio.to_thread", new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()) + ), + patch.object(runner_handler, "_checkout_worktree") as mock_checkout, + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(return_value=(True, "success", "")), + ), + ): + 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) + + await runner_handler.rebase_pr(mock_pull_request, reviewed_user="test-user") + assert any( + "successfully" in str(call).lower() for call in mock_pull_request.create_issue_comment.call_args_list + ) + + @pytest.mark.asyncio + async def test_rebase_pr_conflict_failure(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: + """Test rebase_pr handles rebase conflicts.""" + call_count = 0 + + async def side_effect_rebase(*args: Any, **kwargs: Any) -> tuple[bool, str, str]: + nonlocal call_count + call_count += 1 + cmd = kwargs.get("command", args[0] if args else "") + if "rebase origin/" in cmd: + return (False, "", "CONFLICT (content): Merge conflict in file.py") + if "rebase --abort" in cmd: + return (True, "", "") + return (True, "success", "") + + with ( + patch( + "asyncio.to_thread", new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()) + ), + patch.object(runner_handler, "_checkout_worktree") as mock_checkout, + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(side_effect=side_effect_rebase), + ), + ): + 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) + + await runner_handler.rebase_pr(mock_pull_request, reviewed_user="test-user") + assert any( + "rebase failed" in str(call).lower() for call in mock_pull_request.create_issue_comment.call_args_list + ) + + @pytest.mark.asyncio + async def test_rebase_pr_worktree_failure(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: + """Test rebase_pr handles worktree failure.""" + with ( + patch( + "asyncio.to_thread", new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()) + ), + patch.object(runner_handler, "_checkout_worktree") as mock_checkout, + ): + mock_checkout.return_value = AsyncMock() + mock_checkout.return_value.__aenter__ = AsyncMock(return_value=(False, "", "worktree error", "stderr")) + mock_checkout.return_value.__aexit__ = AsyncMock(return_value=None) + + await runner_handler.rebase_pr(mock_pull_request, reviewed_user="test-user") + mock_pull_request.create_issue_comment.assert_called_once() + call_body = str(mock_pull_request.create_issue_comment.call_args) + assert "worktree" in call_body.lower() or "failed" in call_body.lower() + + +class TestCherryPickPreCommitAutoFix: + """Test suite for pre-commit auto-fix in cherry_pick method.""" + + @pytest.fixture + def mock_github_webhook(self) -> Mock: + mock_webhook = Mock() + mock_webhook.hook_data = {"action": "opened"} + mock_webhook.logger = Mock() + mock_webhook.log_prefix = "[TEST]" + mock_webhook.repository = Mock() + mock_webhook.repository.clone_url = "https://github.com/test/repo.git" + mock_webhook.repository.owner.login = "test-owner" + mock_webhook.repository.owner.email = "test@example.com" + mock_webhook.token = "test-token" # pragma: allowlist secret + mock_webhook.clone_repo_dir = "/tmp/test-repo" + mock_webhook.mask_sensitive = False + mock_webhook.pre_commit = True # Enable pre-commit + mock_webhook.tox = {} + mock_webhook.build_and_push_container = False + mock_webhook.pypi = {} + mock_webhook.conventional_title = "" + mock_webhook.custom_check_runs = [] + mock_webhook.cherry_pick_assign_to_pr_author = True + mock_webhook.repository_full_name = "test/repo" + mock_webhook.repository_name = "repo" + mock_webhook.ctx = None + mock_webhook.ai_features = None + mock_webhook.config = Mock() + mock_webhook.config.get_value = Mock(return_value=None) + return mock_webhook + + @pytest.fixture + def mock_owners_file_handler(self) -> Mock: + mock_handler = Mock() + mock_handler.root_approvers = ["maintainer1"] + return mock_handler + + @pytest.fixture + def runner_handler(self, mock_github_webhook: Mock, mock_owners_file_handler: Mock) -> RunnerHandler: + return RunnerHandler(mock_github_webhook, mock_owners_file_handler) + + @pytest.fixture + def mock_pull_request(self) -> Mock: + mock_pr = Mock() + mock_pr.number = 123 + mock_pr.title = "feat: Test PR" + mock_pr.base.ref = "main" + 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 + + @pytest.fixture(autouse=True) + def patch_check_run_text(self) -> Generator[None]: + with patch( + "webhook_server.libs.handlers.check_run_handler.CheckRunHandler.get_check_run_text", + return_value="dummy output", + ): + yield + + @pytest.fixture(autouse=True) + def patch_shutil_rmtree(self) -> Generator[None]: + with patch("shutil.rmtree"): + yield + + @pytest.mark.asyncio + async def test_cherry_pick_runs_pre_commit_when_enabled( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test that cherry_pick runs pre-commit when pre_commit is enabled.""" + commands_executed: list[str] = [] + + async def mock_run_command(*args: Any, **kwargs: Any) -> tuple[bool, str, str]: + cmd = kwargs.get("command", args[0] if args else "") + commands_executed.append(cmd) + return (True, "success", "") + + with ( + patch.object( + runner_handler, + "_restore_original_author_for_cherry_pick", + new=AsyncMock(return_value=False), + ), + patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())), + patch.object(runner_handler.check_run_handler, "set_check_in_progress"), + patch.object(runner_handler.check_run_handler, "set_check_success"), + patch.object(runner_handler, "_checkout_worktree") as mock_checkout, + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(side_effect=mock_run_command), + ), + patch( + "webhook_server.libs.handlers.runner_handler.get_repository_github_app_token", + return_value=None, + ), + patch( + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), + ), + ): + 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) + + await runner_handler.cherry_pick(mock_pull_request, "main") + + # Verify pre-commit was executed + pre_commit_cmds = [c for c in commands_executed if "prek" in c and "run" in c] + assert len(pre_commit_cmds) >= 1, f"Expected pre-commit command, got: {commands_executed}" + + @pytest.mark.asyncio + async def test_cherry_pick_pre_commit_fixes_committed( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test that pre-commit file fixes are committed.""" + commands_executed: list[str] = [] + + async def mock_run_command(*args: Any, **kwargs: Any) -> tuple[bool, str, str]: + cmd = kwargs.get("command", args[0] if args else "") + commands_executed.append(cmd) + if "prek" in cmd and "run" in cmd: + return (False, "", "Fixing files...") # pre-commit modified files + if "diff --name-only" in cmd: + return (True, "file1.py\nfile2.py", "") # Files modified + return (True, "success", "") + + with ( + patch.object( + runner_handler, + "_restore_original_author_for_cherry_pick", + new=AsyncMock(return_value=False), + ), + patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())), + patch.object(runner_handler.check_run_handler, "set_check_in_progress"), + patch.object(runner_handler.check_run_handler, "set_check_success"), + patch.object(runner_handler, "_checkout_worktree") as mock_checkout, + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(side_effect=mock_run_command), + ), + patch( + "webhook_server.libs.handlers.runner_handler.get_repository_github_app_token", + return_value=None, + ), + patch( + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), + ), + ): + 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) + + await runner_handler.cherry_pick(mock_pull_request, "main") + + # Verify git add and commit were run after pre-commit fix + add_cmds = [c for c in commands_executed if "add -A" in c] + commit_cmds = [c for c in commands_executed if "pre-commit auto-fix" in c] + assert len(add_cmds) >= 1, f"Expected git add, got: {commands_executed}" + assert len(commit_cmds) >= 1, f"Expected commit, got: {commands_executed}" + + @pytest.mark.asyncio + async def test_cherry_pick_skips_pre_commit_when_disabled( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test that cherry_pick does NOT run pre-commit when disabled.""" + runner_handler.github_webhook.pre_commit = False + commands_executed: list[str] = [] + + async def mock_run_command(*args: Any, **kwargs: Any) -> tuple[bool, str, str]: + cmd = kwargs.get("command", args[0] if args else "") + commands_executed.append(cmd) + return (True, "success", "") + + with ( + patch.object( + runner_handler, + "_restore_original_author_for_cherry_pick", + new=AsyncMock(return_value=False), + ), + patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())), + patch.object(runner_handler.check_run_handler, "set_check_in_progress"), + patch.object(runner_handler.check_run_handler, "set_check_success"), + patch.object(runner_handler, "_checkout_worktree") as mock_checkout, + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(side_effect=mock_run_command), + ), + patch( + "webhook_server.libs.handlers.runner_handler.get_repository_github_app_token", + return_value=None, + ), + patch( + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), + ), + ): + 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) + + await runner_handler.cherry_pick(mock_pull_request, "main") + + # Verify pre-commit was NOT executed + pre_commit_cmds = [c for c in commands_executed if "prek" in c and "run" in c] + assert len(pre_commit_cmds) == 0, f"Pre-commit should not run when disabled: {commands_executed}" diff --git a/webhook_server/utils/constants.py b/webhook_server/utils/constants.py index 8c2911e8c..765665de4 100644 --- a/webhook_server/utils/constants.py +++ b/webhook_server/utils/constants.py @@ -43,6 +43,8 @@ COMMAND_AUTOMERGE_STR: str = "automerge" COMMAND_REGENERATE_WELCOME_STR: str = "regenerate-welcome" COMMAND_TEST_ORACLE_STR: str = "test-oracle" +COMMAND_CHERRY_PICK_RETRY_STR: str = "cherry-pick-retry" +COMMAND_REBASE_STR: str = "rebase" AUTOMERGE_LABEL_STR: str = "automerge" ROOT_APPROVERS_KEY: str = "root-approvers" From fbeb1354ff199a1dc7659012a6041474f1040773 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 9 Jun 2026 13:45:55 +0300 Subject: [PATCH 02/17] =?UTF-8?q?fix:=20address=20Qodo=20review=20findings?= =?UTF-8?q?=20=E2=80=94=20fork=20PR=20safety,=20rebase=20auth,=20pre-commi?= =?UTF-8?q?t=20abort,=20DCO=20signoff?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Reject rebase on fork PRs (head repo != base repo) - Restrict non-bot PR rebase to PR owner or maintainers only - Abort cherry-pick when pre-commit fails with unfixable errors - Abort cherry-pick when git add/commit fails after pre-commit fix - Add --signoff to pre-commit auto-fix commit for DCO compliance - Validate single branch name in /cherry-pick-retry --- .../libs/handlers/issue_comment_handler.py | 10 ++ .../libs/handlers/runner_handler.py | 65 ++++++- .../tests/test_issue_comment_handler.py | 17 ++ webhook_server/tests/test_runner_handler.py | 164 ++++++++++++++++++ 4 files changed, 252 insertions(+), 4 deletions(-) diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index e704faf9d..f5ae6159e 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -628,6 +628,16 @@ async def process_cherry_pick_retry_command( reviewed_user: User who requested the retry """ target_branch = command_args.strip() + + # Validate exactly one branch name + if " " in target_branch: + msg = "cherry-pick-retry accepts exactly one branch name" + self.logger.debug(f"{self.log_prefix} {msg}") + await github_api_call( + pull_request.create_issue_comment, msg, logger=self.logger, log_prefix=self.log_prefix + ) + return + self.logger.info(f"{self.log_prefix} Processing cherry-pick retry for branch {target_branch}") # Validate PR is merged diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 6dfb9f1c3..22ca336fe 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -1289,23 +1289,54 @@ async def cherry_pick( mask_sensitive=self.github_webhook.mask_sensitive, ) if not rc_add: - self.logger.warning(f"{self.log_prefix} git add failed after pre-commit fix: {err_add}") + self.logger.error(f"{self.log_prefix} git add failed after pre-commit fix: {err_add}") + output["text"] = self.check_run_handler.get_check_run_text(err=err_add, out="") + await self.check_run_handler.set_check_failure(name=CHERRY_PICKED_LABEL, output=output) + await github_api_call( + pull_request.create_issue_comment, + "Cherry-pick pre-commit auto-fix failed during `git add`. " + "Manual intervention needed.", + logger=self.logger, + log_prefix=self.log_prefix, + ) + return rc_commit, _, err_commit = await run_command( command=( f"{git_cmd} commit -m" f" {shlex.quote('pre-commit auto-fix for cherry-pick')}" - " --no-verify" + " --signoff --no-verify" ), log_prefix=self.log_prefix, redact_secrets=[github_token], mask_sensitive=self.github_webhook.mask_sensitive, ) if not rc_commit: - self.logger.warning( + self.logger.error( f"{self.log_prefix} git commit failed after pre-commit fix: {err_commit}" ) + output["text"] = self.check_run_handler.get_check_run_text(err=err_commit, out="") + await self.check_run_handler.set_check_failure(name=CHERRY_PICKED_LABEL, output=output) + await github_api_call( + pull_request.create_issue_comment, + "Cherry-pick pre-commit auto-fix failed during `git commit`. " + "Manual intervention needed.", + logger=self.logger, + log_prefix=self.log_prefix, + ) + return else: - self.logger.debug(f"{self.log_prefix} Pre-commit failed but no files modified") + # Pre-commit failed with no fixable changes — abort + self.logger.error(f"{self.log_prefix} Pre-commit failed with unfixable errors") + output["text"] = self.check_run_handler.get_check_run_text(err=_err_pc, out=_out_pc) + await self.check_run_handler.set_check_failure(name=CHERRY_PICKED_LABEL, output=output) + await github_api_call( + pull_request.create_issue_comment, + "Cherry-pick pre-commit check failed with unfixable errors. " + "Manual intervention needed.", + logger=self.logger, + log_prefix=self.log_prefix, + ) + return else: self.logger.debug(f"{self.log_prefix} Pre-commit passed without modifications") @@ -1552,6 +1583,18 @@ async def rebase_pr(self, pull_request: PullRequest, reviewed_user: str) -> None ) return + # Reject fork PRs — force-push would target the base repo + head_repo_full_name = await github_api_call( + lambda: pull_request.head.repo.full_name, logger=self.logger, log_prefix=self.log_prefix + ) + if head_repo_full_name != self.github_webhook.repository_full_name: + msg = "Rebase is not supported for fork PRs — the head branch is in a different repository." + self.logger.debug(f"{self.log_prefix} {msg}") + await github_api_call( + pull_request.create_issue_comment, msg, logger=self.logger, log_prefix=self.log_prefix + ) + return + pr_user_login = await github_api_call( lambda: pull_request.user.login, logger=self.logger, log_prefix=self.log_prefix ) @@ -1577,6 +1620,20 @@ async def rebase_pr(self, pull_request: PullRequest, reviewed_user: str) -> None pull_request.create_issue_comment, msg, logger=self.logger, log_prefix=self.log_prefix ) return + else: + # For user-owned PRs, only the PR owner or maintainers can rebase + if reviewed_user != pr_user_login: + maintainers = await self.owners_file_handler.get_all_repository_maintainers() + if reviewed_user not in maintainers: + msg = ( + f"@{reviewed_user} is not authorized to rebase this PR.\n" + "Only the PR owner or maintainers can rebase." + ) + self.logger.debug(f"{self.log_prefix} {msg}") + await github_api_call( + pull_request.create_issue_comment, msg, logger=self.logger, log_prefix=self.log_prefix + ) + return base_ref = await github_api_call(lambda: pull_request.base.ref, logger=self.logger, log_prefix=self.log_prefix) head_ref = await github_api_call(lambda: pull_request.head.ref, logger=self.logger, log_prefix=self.log_prefix) diff --git a/webhook_server/tests/test_issue_comment_handler.py b/webhook_server/tests/test_issue_comment_handler.py index 2dac3bd3c..b4131d8e8 100644 --- a/webhook_server/tests/test_issue_comment_handler.py +++ b/webhook_server/tests/test_issue_comment_handler.py @@ -2020,6 +2020,23 @@ async def test_cherry_pick_retry_unauthorized_user(self, issue_comment_handler: mock_is_valid.assert_awaited_once() mock_retry.assert_not_awaited() + @pytest.mark.asyncio + async def test_process_cherry_pick_retry_multiple_branches_rejected( + self, issue_comment_handler: IssueCommentHandler + ) -> None: + """Test cherry-pick-retry rejects multiple branch names.""" + mock_pull_request = Mock() + + with patch("asyncio.to_thread", new_callable=AsyncMock, side_effect=lambda f, *a, **k: f(*a, **k)): + await issue_comment_handler.process_cherry_pick_retry_command( + pull_request=mock_pull_request, + command_args="branch1 branch2", + reviewed_user="test-user", + ) + mock_pull_request.create_issue_comment.assert_called_once() + call_body = str(mock_pull_request.create_issue_comment.call_args) + assert "exactly one" in call_body.lower() + @pytest.mark.asyncio async def test_process_cherry_pick_retry_not_merged(self, issue_comment_handler: IssueCommentHandler) -> None: """Test cherry-pick-retry rejects if PR is not merged.""" diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 8c25be9a8..f097ee969 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -2901,6 +2901,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.clone_repo_dir = "/tmp/test-repo" mock_webhook.mask_sensitive = False mock_webhook.auto_verified_and_merged_users = ["bot-user[bot]"] + mock_webhook.repository_full_name = "test/repo" mock_webhook.ctx = None mock_webhook.config = Mock() mock_webhook.config.get_value = Mock(return_value=None) @@ -2930,6 +2931,8 @@ def mock_pull_request(self) -> Mock: mock_pr.state = "open" mock_pr.base.ref = "main" mock_pr.head.ref = "feature-branch" + mock_pr.head.repo = Mock() + mock_pr.head.repo.full_name = "test/repo" # Same repo (not a fork) mock_pr.user = Mock() mock_pr.user.login = "test-user" mock_pr.assignees = [] @@ -3091,6 +3094,61 @@ async def test_rebase_pr_worktree_failure(self, runner_handler: RunnerHandler, m call_body = str(mock_pull_request.create_issue_comment.call_args) assert "worktree" in call_body.lower() or "failed" in call_body.lower() + @pytest.mark.asyncio + async def test_rebase_pr_fork_pr_rejected(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: + """Test rebase_pr rejects fork PRs.""" + mock_pull_request.head.repo.full_name = "fork-owner/repo" # Different from test/repo + + 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.rebase_pr(mock_pull_request, reviewed_user="test-user") + mock_pull_request.create_issue_comment.assert_called_once() + call_body = str(mock_pull_request.create_issue_comment.call_args) + assert "fork" in call_body.lower() + + @pytest.mark.asyncio + async def test_rebase_pr_non_owner_non_maintainer_rejected( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test rebase_pr rejects non-owner non-maintainer users on user-owned PRs.""" + mock_pull_request.user.login = "pr-owner" + + 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.rebase_pr(mock_pull_request, reviewed_user="random-user") + mock_pull_request.create_issue_comment.assert_called_once() + call_body = str(mock_pull_request.create_issue_comment.call_args) + assert "not authorized" in call_body.lower() + + @pytest.mark.asyncio + async def test_rebase_pr_owner_allowed(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: + """Test rebase_pr allows the PR owner.""" + mock_pull_request.user.login = "test-user" + + with ( + patch( + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), + ), + patch.object(runner_handler, "_checkout_worktree") as mock_checkout, + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(return_value=(True, "success", "")), + ), + ): + 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) + + await runner_handler.rebase_pr(mock_pull_request, reviewed_user="test-user") + assert any( + "successfully" in str(call).lower() for call in mock_pull_request.create_issue_comment.call_args_list + ) + class TestCherryPickPreCommitAutoFix: """Test suite for pre-commit auto-fix in cherry_pick method.""" @@ -3255,6 +3313,10 @@ async def mock_run_command(*args: Any, **kwargs: Any) -> tuple[bool, str, str]: commit_cmds = [c for c in commands_executed if "pre-commit auto-fix" in c] assert len(add_cmds) >= 1, f"Expected git add, got: {commands_executed}" assert len(commit_cmds) >= 1, f"Expected commit, got: {commands_executed}" + # Verify --signoff is included for DCO compliance + assert any("--signoff" in c for c in commit_cmds), ( + f"Expected --signoff in commit command, got: {commit_cmds}" + ) @pytest.mark.asyncio async def test_cherry_pick_skips_pre_commit_when_disabled( @@ -3301,3 +3363,105 @@ async def mock_run_command(*args: Any, **kwargs: Any) -> tuple[bool, str, str]: # Verify pre-commit was NOT executed pre_commit_cmds = [c for c in commands_executed if "prek" in c and "run" in c] assert len(pre_commit_cmds) == 0, f"Pre-commit should not run when disabled: {commands_executed}" + + @pytest.mark.asyncio + async def test_cherry_pick_pre_commit_unfixable_aborts( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test that cherry_pick aborts when pre-commit fails with no file modifications.""" + + async def mock_run_command(*args: Any, **kwargs: Any) -> tuple[bool, str, str]: + cmd = kwargs.get("command", args[0] if args else "") + if "prek" in cmd and "run" in cmd: + return (False, "lint error output", "unfixable error") # pre-commit failed + if "diff --name-only" in cmd: + return (True, "", "") # No files modified + return (True, "success", "") + + with ( + patch.object( + runner_handler, + "_restore_original_author_for_cherry_pick", + new=AsyncMock(return_value=False), + ), + patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())), + patch.object(runner_handler.check_run_handler, "set_check_in_progress"), + patch.object(runner_handler.check_run_handler, "set_check_failure") as mock_set_failure, + patch.object(runner_handler.check_run_handler, "set_check_success") as mock_set_success, + patch.object(runner_handler, "_checkout_worktree") as mock_checkout, + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(side_effect=mock_run_command), + ), + patch( + "webhook_server.libs.handlers.runner_handler.get_repository_github_app_token", + return_value=None, + ), + patch( + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), + ), + ): + 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) + + await runner_handler.cherry_pick(mock_pull_request, "main") + + # Should abort — set_check_failure called, set_check_success NOT called + mock_set_failure.assert_called() + mock_set_success.assert_not_called() + # Should post comment about unfixable errors + assert any( + "unfixable" in str(call).lower() for call in mock_pull_request.create_issue_comment.call_args_list + ) + + @pytest.mark.asyncio + async def test_cherry_pick_pre_commit_git_add_failure_aborts( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Test that cherry_pick aborts when git add fails after pre-commit fix.""" + + async def mock_run_command(*args: Any, **kwargs: Any) -> tuple[bool, str, str]: + cmd = kwargs.get("command", args[0] if args else "") + if "prek" in cmd and "run" in cmd: + return (False, "", "Fixing files...") # pre-commit modified files + if "diff --name-only" in cmd: + return (True, "file1.py", "") # Files modified + if "add -A" in cmd: + return (False, "", "git add failed") # git add fails + return (True, "success", "") + + with ( + patch.object( + runner_handler, + "_restore_original_author_for_cherry_pick", + new=AsyncMock(return_value=False), + ), + patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())), + patch.object(runner_handler.check_run_handler, "set_check_in_progress"), + patch.object(runner_handler.check_run_handler, "set_check_failure") as mock_set_failure, + patch.object(runner_handler.check_run_handler, "set_check_success") as mock_set_success, + patch.object(runner_handler, "_checkout_worktree") as mock_checkout, + patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(side_effect=mock_run_command), + ), + patch( + "webhook_server.libs.handlers.runner_handler.get_repository_github_app_token", + return_value=None, + ), + patch( + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), + ), + ): + 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) + + await runner_handler.cherry_pick(mock_pull_request, "main") + + mock_set_failure.assert_called() + mock_set_success.assert_not_called() + assert any("git add" in str(call).lower() for call in mock_pull_request.create_issue_comment.call_args_list) From ab46b6f84d792e6020398cb4c47440d240891c64 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 9 Jun 2026 13:59:37 +0300 Subject: [PATCH 03/17] fix: handle git diff failure separately - When git diff --name-only fails after pre-commit, report as worktree error - Previously fell into unfixable pre-commit errors branch, misattributing cause --- webhook_server/libs/handlers/runner_handler.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 22ca336fe..050f53704 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -1274,13 +1274,25 @@ async def cherry_pick( if not rc_pc: # Pre-commit returned non-zero — check if any files were # actually modified before committing. - rc_diff, out_diff, _ = await run_command( + rc_diff, out_diff, err_diff = await run_command( command=f"{git_cmd} diff --name-only", log_prefix=self.log_prefix, redact_secrets=[github_token], mask_sensitive=self.github_webhook.mask_sensitive, ) - if rc_diff and out_diff.strip(): + if not rc_diff: + # git diff itself failed — report as worktree error, not pre-commit + self.logger.error(f"{self.log_prefix} git diff failed after pre-commit: {err_diff}") + output["text"] = self.check_run_handler.get_check_run_text(err=err_diff, out=out_diff) + await self.check_run_handler.set_check_failure(name=CHERRY_PICKED_LABEL, output=output) + await github_api_call( + pull_request.create_issue_comment, + "Cherry-pick failed: git diff error after pre-commit run. Check worktree state.", + logger=self.logger, + log_prefix=self.log_prefix, + ) + return + elif out_diff.strip(): self.logger.info(f"{self.log_prefix} Pre-commit modified files, committing fixes") rc_add, _, err_add = await run_command( command=f"{git_cmd} add -A", From d7bfda95fe58853f737e3c01634b5dd2870b5c49 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 9 Jun 2026 14:10:23 +0300 Subject: [PATCH 04/17] security: redact secrets in git diff error logging Redact secrets in git diff error logging after pre-commit. Reformat long lines for readability. --- webhook_server/libs/handlers/runner_handler.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 050f53704..319ae0ed3 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -1282,8 +1282,18 @@ async def cherry_pick( ) if not rc_diff: # git diff itself failed — report as worktree error, not pre-commit - self.logger.error(f"{self.log_prefix} git diff failed after pre-commit: {err_diff}") - output["text"] = self.check_run_handler.get_check_run_text(err=err_diff, out=out_diff) + redacted_err_diff = _redact_secrets( + err_diff, [github_token], mask_sensitive=self.github_webhook.mask_sensitive + ) + redacted_out_diff = _redact_secrets( + out_diff, [github_token], mask_sensitive=self.github_webhook.mask_sensitive + ) + self.logger.error( + f"{self.log_prefix} git diff failed after pre-commit: {redacted_err_diff}" + ) + output["text"] = self.check_run_handler.get_check_run_text( + err=redacted_err_diff, out=redacted_out_diff + ) await self.check_run_handler.set_check_failure(name=CHERRY_PICKED_LABEL, output=output) await github_api_call( pull_request.create_issue_comment, From b7c27e8dff1e9a47f56e412456e6cb775b542744 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 9 Jun 2026 14:51:29 +0300 Subject: [PATCH 05/17] fix: use user.type == Bot for cherry-pick PR detection - Cherry-pick PRs are created by the GitHub App bot, not in auto_verified_and_merged_users - Check user.type == Bot to detect any bot-created PR - Fixes cherry-pick-retry not closing old cherry-pick PRs - Also fixes rebase bot-PR ownership detection --- webhook_server/libs/handlers/issue_comment_handler.py | 6 +++--- webhook_server/libs/handlers/runner_handler.py | 5 ++++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index f5ae6159e..085575dbe 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -681,10 +681,10 @@ async def process_cherry_pick_retry_command( ) if pr_title.startswith(pr_title_prefix): # Verify the PR was created by a bot (not a human-created PR) - pr_author = await github_api_call( - lambda _pr=open_pr: _pr.user.login, logger=self.logger, log_prefix=self.log_prefix + pr_user_type = await github_api_call( + lambda _pr=open_pr: _pr.user.type, logger=self.logger, log_prefix=self.log_prefix ) - if pr_author not in self.github_webhook.auto_verified_and_merged_users: + if pr_user_type != "Bot": continue # Skip non-bot PRs # Check if the PR body references the original PR diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 319ae0ed3..dc99781ba 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -1617,12 +1617,15 @@ async def rebase_pr(self, pull_request: PullRequest, reviewed_user: str) -> None ) return + pr_user_type = await github_api_call( + lambda: pull_request.user.type, logger=self.logger, log_prefix=self.log_prefix + ) pr_user_login = await github_api_call( lambda: pull_request.user.login, logger=self.logger, log_prefix=self.log_prefix ) # Check if PR is bot-owned (e.g., cherry-pick PRs created by the app) - is_bot_pr = pr_user_login in self.github_webhook.auto_verified_and_merged_users + is_bot_pr = pr_user_type == "Bot" if is_bot_pr: # For bot-owned PRs, validate the user is the PR assignee or a maintainer From 71904959722fe17bee51dab90a8bef672ba1d6bf Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 9 Jun 2026 15:07:12 +0300 Subject: [PATCH 06/17] fix: narrow bot-PR detection to app-managed cherry-pick PRs only - user.type == Bot is too broad (catches Renovate, Dependabot, etc.) - Now also checks for cherry-pick labels to confirm it is our app PR - Prevents unauthorized force-push rebase on non-cherry-pick bot PRs --- .../libs/handlers/issue_comment_handler.py | 13 ++++++++++++- webhook_server/libs/handlers/runner_handler.py | 9 +++++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index 085575dbe..e881b959f 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -20,6 +20,7 @@ AUTOMERGE_LABEL_STR, BUILD_AND_PUSH_CONTAINER_STR, CHERRY_PICK_LABEL_PREFIX, + CHERRY_PICKED_LABEL, COMMAND_ADD_ALLOWED_USER_STR, COMMAND_ASSIGN_REVIEWER_STR, COMMAND_ASSIGN_REVIEWERS_STR, @@ -680,13 +681,23 @@ async def process_cherry_pick_retry_command( lambda _pr=open_pr: _pr.title, logger=self.logger, log_prefix=self.log_prefix ) if pr_title.startswith(pr_title_prefix): - # Verify the PR was created by a bot (not a human-created PR) + # Verify the PR was created by our app (bot with cherry-pick labels), not any bot pr_user_type = await github_api_call( lambda _pr=open_pr: _pr.user.type, logger=self.logger, log_prefix=self.log_prefix ) if pr_user_type != "Bot": continue # Skip non-bot PRs + pr_labels = await github_api_call( + lambda _pr=open_pr: [label.name for label in _pr.labels], + logger=self.logger, + log_prefix=self.log_prefix, + ) + if not any( + label.startswith(CHERRY_PICKED_LABEL) or label.startswith("cherry-pick-") for label in pr_labels + ): + continue # Skip bot PRs not managed by our app + # Check if the PR body references the original PR pr_body = await github_api_call( lambda _pr=open_pr: _pr.body or "", logger=self.logger, log_prefix=self.log_prefix diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index dc99781ba..821772c6e 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -1624,8 +1624,13 @@ async def rebase_pr(self, pull_request: PullRequest, reviewed_user: str) -> None lambda: pull_request.user.login, logger=self.logger, log_prefix=self.log_prefix ) - # Check if PR is bot-owned (e.g., cherry-pick PRs created by the app) - is_bot_pr = pr_user_type == "Bot" + # Check if PR is managed by our app (has cherry-pick labels), not any bot PR (e.g. Renovate, Dependabot) + pr_labels = await github_api_call( + lambda: [label.name for label in pull_request.labels], logger=self.logger, log_prefix=self.log_prefix + ) + is_bot_pr = pr_user_type == "Bot" and any( + label.startswith(CHERRY_PICKED_LABEL) or label.startswith("cherry-pick-") for label in pr_labels + ) if is_bot_pr: # For bot-owned PRs, validate the user is the PR assignee or a maintainer From 6355e4c1fc3f9eb5c988a7906ea29313f636e1f0 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 9 Jun 2026 15:29:08 +0300 Subject: [PATCH 07/17] =?UTF-8?q?fix:=20cherry-pick-retry=20close=20logic?= =?UTF-8?q?=20=E2=80=94=20add=20debug=20logging,=20fix=20body=20URL=20matc?= =?UTF-8?q?h,=20simplify=20bot=20check?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Wrap pull_request.html_url in github_api_call to avoid blocking/stale data - Remove over-restrictive bot-ownership and label filters that prevented matches - Simplify close logic to title prefix + body URL match (sufficient for correctness) - Add debug logging at every step: PR count, title match/skip, body URL match/skip, close/no-match - Update rebase bot-PR detection to use user.type == "Bot" with cherry-pick label check - Update tests to match simplified close logic and bot detection changes --- .../libs/handlers/issue_comment_handler.py | 79 +++++++++++-------- .../tests/test_issue_comment_handler.py | 30 +++---- webhook_server/tests/test_runner_handler.py | 14 ++++ 3 files changed, 70 insertions(+), 53 deletions(-) diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index e881b959f..facc95025 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -20,7 +20,6 @@ AUTOMERGE_LABEL_STR, BUILD_AND_PUSH_CONTAINER_STR, CHERRY_PICK_LABEL_PREFIX, - CHERRY_PICKED_LABEL, COMMAND_ADD_ALLOWED_USER_STR, COMMAND_ASSIGN_REVIEWER_STR, COMMAND_ASSIGN_REVIEWERS_STR, @@ -671,52 +670,62 @@ async def process_cherry_pick_retry_command( # Find and close existing failed cherry-pick PR for this branch (created by bot) pr_title_prefix = f"CherryPicked: [{target_branch}]" + original_pr_url = await github_api_call( + lambda: pull_request.html_url, logger=self.logger, log_prefix=self.log_prefix + ) + self.logger.debug( + f"{self.log_prefix} Cherry-pick retry: looking for open PRs with title prefix " + f"'{pr_title_prefix}' referencing {original_pr_url}" + ) open_pulls = await github_api_call( lambda: list(self.repository.get_pulls(state="open")), logger=self.logger, log_prefix=self.log_prefix, ) + self.logger.debug(f"{self.log_prefix} Cherry-pick retry: found {len(open_pulls)} open PRs to scan") + closed_old_pr = False for open_pr in open_pulls: pr_title = await github_api_call( lambda _pr=open_pr: _pr.title, logger=self.logger, log_prefix=self.log_prefix ) - if pr_title.startswith(pr_title_prefix): - # Verify the PR was created by our app (bot with cherry-pick labels), not any bot - pr_user_type = await github_api_call( - lambda _pr=open_pr: _pr.user.type, logger=self.logger, log_prefix=self.log_prefix + if not pr_title.startswith(pr_title_prefix): + self.logger.debug( + f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} title " + f"'{pr_title}' does not match prefix '{pr_title_prefix}', skipping" ) - if pr_user_type != "Bot": - continue # Skip non-bot PRs + continue - pr_labels = await github_api_call( - lambda _pr=open_pr: [label.name for label in _pr.labels], - logger=self.logger, - log_prefix=self.log_prefix, - ) - if not any( - label.startswith(CHERRY_PICKED_LABEL) or label.startswith("cherry-pick-") for label in pr_labels - ): - continue # Skip bot PRs not managed by our app - - # Check if the PR body references the original PR - pr_body = await github_api_call( - lambda _pr=open_pr: _pr.body or "", logger=self.logger, log_prefix=self.log_prefix + self.logger.debug(f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} title matches prefix") + + # Check if the PR body references the original PR + pr_body = await github_api_call( + lambda _pr=open_pr: _pr.body or "", logger=self.logger, log_prefix=self.log_prefix + ) + if original_pr_url not in pr_body: + self.logger.debug( + f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} body does not " + f"contain original PR URL '{original_pr_url}', skipping" ) - if pull_request.html_url in pr_body: - self.logger.info(f"{self.log_prefix} Closing existing cherry-pick PR #{open_pr.number} for retry") - await github_api_call( - open_pr.edit, - state="closed", - logger=self.logger, - log_prefix=self.log_prefix, - ) - await github_api_call( - open_pr.create_issue_comment, - f"Closed by cherry-pick retry requested by @{reviewed_user} on {pull_request.html_url}", - logger=self.logger, - log_prefix=self.log_prefix, - ) - break + continue + + self.logger.info(f"{self.log_prefix} Closing existing cherry-pick PR #{open_pr.number} for retry") + await github_api_call( + open_pr.edit, + state="closed", + logger=self.logger, + log_prefix=self.log_prefix, + ) + await github_api_call( + open_pr.create_issue_comment, + f"Closed by cherry-pick retry requested by @{reviewed_user} on {original_pr_url}", + logger=self.logger, + log_prefix=self.log_prefix, + ) + closed_old_pr = True + break + + if not closed_old_pr: + self.logger.debug(f"{self.log_prefix} Cherry-pick retry: no existing cherry-pick PR found to close") # Re-run cherry-pick for the target branch self.logger.info(f"{self.log_prefix} Retrying cherry-pick to {target_branch}") diff --git a/webhook_server/tests/test_issue_comment_handler.py b/webhook_server/tests/test_issue_comment_handler.py index b4131d8e8..d7ba0aacd 100644 --- a/webhook_server/tests/test_issue_comment_handler.py +++ b/webhook_server/tests/test_issue_comment_handler.py @@ -2091,17 +2091,14 @@ async def test_process_cherry_pick_retry_success(self, issue_comment_handler: Is "pull_request": {"merged_at": "2024-01-01T00:00:00Z"}, } - # Mock existing cherry-pick PR to close (created by bot) + # Mock existing cherry-pick PR to close mock_existing_cp_pr = Mock() mock_existing_cp_pr.number = 456 mock_existing_cp_pr.title = "CherryPicked: [release-1.0] Some commit" mock_existing_cp_pr.body = "Cherry-pick from main, original PR: https://github.com/test/repo/pull/123" - mock_existing_cp_pr.user = Mock() - mock_existing_cp_pr.user.login = "bot-user[bot]" mock_existing_cp_pr.edit = Mock() mock_existing_cp_pr.create_issue_comment = Mock() - issue_comment_handler.github_webhook.auto_verified_and_merged_users = ["bot-user[bot]"] issue_comment_handler.repository.get_pulls = Mock(return_value=[mock_existing_cp_pr]) with ( @@ -2160,10 +2157,10 @@ async def test_process_cherry_pick_retry_no_existing_pr(self, issue_comment_hand mock_cherry_pick.assert_awaited_once() @pytest.mark.asyncio - async def test_process_cherry_pick_retry_skips_human_created_pr( + async def test_process_cherry_pick_retry_skips_pr_with_different_body_url( self, issue_comment_handler: IssueCommentHandler ) -> None: - """Test cherry-pick-retry does NOT close a human-created PR matching the title.""" + """Test cherry-pick-retry does NOT close a PR whose body references a different original PR.""" mock_pull_request = Mock() mock_pull_request.html_url = "https://github.com/test/repo/pull/123" mock_label = Mock() @@ -2175,17 +2172,14 @@ async def test_process_cherry_pick_retry_skips_human_created_pr( "pull_request": {"merged_at": "2024-01-01T00:00:00Z"}, } - # Human-created PR with matching title (NOT a bot) - mock_human_pr = Mock() - mock_human_pr.number = 789 - mock_human_pr.title = "CherryPicked: [release-1.0] Manual cherry-pick" - mock_human_pr.body = "original PR: https://github.com/test/repo/pull/123" - mock_human_pr.user = Mock() - mock_human_pr.user.login = "human-user" # Not a bot - mock_human_pr.edit = Mock() + # PR with matching title but body references a different original PR + mock_other_pr = Mock() + mock_other_pr.number = 789 + mock_other_pr.title = "CherryPicked: [release-1.0] Other commit" + mock_other_pr.body = "Cherry-pick from main, original PR: https://github.com/test/repo/pull/999" + mock_other_pr.edit = Mock() - issue_comment_handler.github_webhook.auto_verified_and_merged_users = ["bot-user[bot]"] - issue_comment_handler.repository.get_pulls = Mock(return_value=[mock_human_pr]) + issue_comment_handler.repository.get_pulls = Mock(return_value=[mock_other_pr]) with ( patch("asyncio.to_thread", new_callable=AsyncMock, side_effect=lambda f, *a, **k: f(*a, **k)), @@ -2200,8 +2194,8 @@ async def test_process_cherry_pick_retry_skips_human_created_pr( command_args="release-1.0", reviewed_user="test-user", ) - # Human PR should NOT be closed - mock_human_pr.edit.assert_not_called() + # PR with different body URL should NOT be closed + mock_other_pr.edit.assert_not_called() # Cherry-pick should still be re-run mock_cherry_pick.assert_awaited_once() diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index f097ee969..da102db0e 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -2935,7 +2935,9 @@ def mock_pull_request(self) -> Mock: mock_pr.head.repo.full_name = "test/repo" # Same repo (not a fork) mock_pr.user = Mock() mock_pr.user.login = "test-user" + mock_pr.user.type = "User" mock_pr.assignees = [] + mock_pr.labels = [] # No labels by default mock_pr.create_issue_comment = Mock() return mock_pr @@ -2955,6 +2957,10 @@ async def test_rebase_pr_not_open(self, runner_handler: RunnerHandler, mock_pull async def test_rebase_pr_bot_pr_unauthorized(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: """Test rebase_pr rejects unauthorized users on bot-owned PRs.""" mock_pull_request.user.login = "bot-user[bot]" + mock_pull_request.user.type = "Bot" + mock_cherry_pick_label = Mock() + mock_cherry_pick_label.name = "CherryPicked-from-main" + mock_pull_request.labels = [mock_cherry_pick_label] mock_pull_request.assignees = [Mock(login="original-author")] with patch( @@ -2971,6 +2977,10 @@ async def test_rebase_pr_bot_pr_assignee_allowed( ) -> None: """Test rebase_pr allows PR assignee on bot-owned PRs.""" mock_pull_request.user.login = "bot-user[bot]" + mock_pull_request.user.type = "Bot" + mock_cherry_pick_label = Mock() + mock_cherry_pick_label.name = "CherryPicked-from-main" + mock_pull_request.labels = [mock_cherry_pick_label] mock_pull_request.assignees = [Mock(login="test-user")] with ( @@ -2999,6 +3009,10 @@ async def test_rebase_pr_bot_pr_maintainer_allowed( ) -> None: """Test rebase_pr allows maintainers on bot-owned PRs.""" mock_pull_request.user.login = "bot-user[bot]" + mock_pull_request.user.type = "Bot" + mock_cherry_pick_label = Mock() + mock_cherry_pick_label.name = "CherryPicked-from-main" + mock_pull_request.labels = [mock_cherry_pick_label] mock_pull_request.assignees = [Mock(login="original-author")] with ( From 273ad40ffbf460c308987eb15136bc43e9c1a3b3 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 9 Jun 2026 15:35:05 +0300 Subject: [PATCH 08/17] fix: add back user.type == "Bot" check in cherry-pick-retry close logic - Restore Bot type verification before closing cherry-pick PRs on retry - Prevents closing human-created PRs that happen to match title/body pattern - Keep debug logging at every step (title, user type, body URL) - Add test for human-created PR skip (user.type == "User") --- .../libs/handlers/issue_comment_handler.py | 13 +++++ .../tests/test_issue_comment_handler.py | 49 ++++++++++++++++++- 2 files changed, 61 insertions(+), 1 deletion(-) diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index facc95025..a9773c722 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -697,6 +697,19 @@ async def process_cherry_pick_retry_command( self.logger.debug(f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} title matches prefix") + # Verify the PR was created by a bot (not a human) + pr_user_type = await github_api_call( + lambda _pr=open_pr: _pr.user.type, logger=self.logger, log_prefix=self.log_prefix + ) + if pr_user_type != "Bot": + self.logger.debug( + f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author type " + f"is '{pr_user_type}', not 'Bot', skipping" + ) + continue + + self.logger.debug(f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author is Bot") + # Check if the PR body references the original PR pr_body = await github_api_call( lambda _pr=open_pr: _pr.body or "", logger=self.logger, log_prefix=self.log_prefix diff --git a/webhook_server/tests/test_issue_comment_handler.py b/webhook_server/tests/test_issue_comment_handler.py index d7ba0aacd..26ddb981a 100644 --- a/webhook_server/tests/test_issue_comment_handler.py +++ b/webhook_server/tests/test_issue_comment_handler.py @@ -2091,11 +2091,13 @@ async def test_process_cherry_pick_retry_success(self, issue_comment_handler: Is "pull_request": {"merged_at": "2024-01-01T00:00:00Z"}, } - # Mock existing cherry-pick PR to close + # Mock existing cherry-pick PR to close (created by bot) mock_existing_cp_pr = Mock() mock_existing_cp_pr.number = 456 mock_existing_cp_pr.title = "CherryPicked: [release-1.0] Some commit" mock_existing_cp_pr.body = "Cherry-pick from main, original PR: https://github.com/test/repo/pull/123" + mock_existing_cp_pr.user = Mock() + mock_existing_cp_pr.user.type = "Bot" mock_existing_cp_pr.edit = Mock() mock_existing_cp_pr.create_issue_comment = Mock() @@ -2199,6 +2201,51 @@ async def test_process_cherry_pick_retry_skips_pr_with_different_body_url( # Cherry-pick should still be re-run mock_cherry_pick.assert_awaited_once() + @pytest.mark.asyncio + async def test_process_cherry_pick_retry_skips_human_created_pr( + self, issue_comment_handler: IssueCommentHandler + ) -> None: + """Test cherry-pick-retry does NOT close a human-created PR matching the title and body.""" + mock_pull_request = Mock() + mock_pull_request.html_url = "https://github.com/test/repo/pull/123" + mock_label = Mock() + mock_label.name = f"{CHERRY_PICK_LABEL_PREFIX}release-1.0" + mock_pull_request.labels = [mock_label] + + issue_comment_handler.hook_data["issue"] = { + "number": 123, + "pull_request": {"merged_at": "2024-01-01T00:00:00Z"}, + } + + # Human-created PR with matching title and body URL + mock_human_pr = Mock() + mock_human_pr.number = 789 + mock_human_pr.title = "CherryPicked: [release-1.0] Manual cherry-pick" + mock_human_pr.body = "original PR: https://github.com/test/repo/pull/123" + mock_human_pr.user = Mock() + mock_human_pr.user.type = "User" # Not a Bot + mock_human_pr.edit = Mock() + + issue_comment_handler.repository.get_pulls = Mock(return_value=[mock_human_pr]) + + with ( + patch("asyncio.to_thread", new_callable=AsyncMock, side_effect=lambda f, *a, **k: f(*a, **k)), + patch.object( + issue_comment_handler.runner_handler, + "cherry_pick", + new_callable=AsyncMock, + ) as mock_cherry_pick, + ): + await issue_comment_handler.process_cherry_pick_retry_command( + pull_request=mock_pull_request, + command_args="release-1.0", + reviewed_user="test-user", + ) + # Human PR should NOT be closed + mock_human_pr.edit.assert_not_called() + # Cherry-pick should still be re-run + mock_cherry_pick.assert_awaited_once() + # --- Rebase command tests --- @pytest.mark.asyncio From d920be98d695ff7eafd5efec8888e46dc328d4eb Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 9 Jun 2026 15:39:18 +0300 Subject: [PATCH 09/17] fix: use app_bot_login for precise bot-PR identification in retry and rebase - Store GitHub App bot login on GithubWebhook during init (github_api.py) - Cherry-pick retry: check PR author login matches app_bot_login instead of user.type - Rebase: check PR author login matches app_bot_login instead of user.type + label heuristic - Prevents false matches on Renovate, Dependabot, or other bot PRs --- webhook_server/libs/github_api.py | 6 ++++++ .../libs/handlers/issue_comment_handler.py | 15 ++++++++------- .../libs/handlers/runner_handler.py | 12 ++---------- .../tests/test_issue_comment_handler.py | 8 ++++++-- webhook_server/tests/test_runner_handler.py | 19 ++++--------------- 5 files changed, 26 insertions(+), 34 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 7bd17d8c3..32077532a 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -195,6 +195,12 @@ def __init__(self, hook_data: dict[Any, Any], headers: Headers, logger: logging. github_app_api=github_app_api, repository=self.repository_full_name ) + # Store the app's bot login for identifying PRs created by our app + try: + self.app_bot_login: str = github_app_api.get_user().login + except Exception: + self.app_bot_login = "" + if not (self.repository or self.repository_by_github_app): self.logger.error(f"{self.log_prefix} Failed to get repository.") return diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index a9773c722..2e2cebd10 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -697,18 +697,19 @@ async def process_cherry_pick_retry_command( self.logger.debug(f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} title matches prefix") - # Verify the PR was created by a bot (not a human) - pr_user_type = await github_api_call( - lambda _pr=open_pr: _pr.user.type, logger=self.logger, log_prefix=self.log_prefix + # Verify the PR was created by our app's bot + pr_author_login = await github_api_call( + lambda _pr=open_pr: _pr.user.login, logger=self.logger, log_prefix=self.log_prefix ) - if pr_user_type != "Bot": + if pr_author_login != self.github_webhook.app_bot_login: self.logger.debug( - f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author type " - f"is '{pr_user_type}', not 'Bot', skipping" + f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author " + f"'{pr_author_login}' is not our app bot " + f"'{self.github_webhook.app_bot_login}', skipping" ) continue - self.logger.debug(f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author is Bot") + self.logger.debug(f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author matches our app bot") # Check if the PR body references the original PR pr_body = await github_api_call( diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 821772c6e..828da8de3 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -1617,20 +1617,12 @@ async def rebase_pr(self, pull_request: PullRequest, reviewed_user: str) -> None ) return - pr_user_type = await github_api_call( - lambda: pull_request.user.type, logger=self.logger, log_prefix=self.log_prefix - ) pr_user_login = await github_api_call( lambda: pull_request.user.login, logger=self.logger, log_prefix=self.log_prefix ) - # Check if PR is managed by our app (has cherry-pick labels), not any bot PR (e.g. Renovate, Dependabot) - pr_labels = await github_api_call( - lambda: [label.name for label in pull_request.labels], logger=self.logger, log_prefix=self.log_prefix - ) - is_bot_pr = pr_user_type == "Bot" and any( - label.startswith(CHERRY_PICKED_LABEL) or label.startswith("cherry-pick-") for label in pr_labels - ) + # Check if PR was created by our app's bot + is_bot_pr = pr_user_login == self.github_webhook.app_bot_login if is_bot_pr: # For bot-owned PRs, validate the user is the PR assignee or a maintainer diff --git a/webhook_server/tests/test_issue_comment_handler.py b/webhook_server/tests/test_issue_comment_handler.py index 26ddb981a..15af4d144 100644 --- a/webhook_server/tests/test_issue_comment_handler.py +++ b/webhook_server/tests/test_issue_comment_handler.py @@ -2097,7 +2097,9 @@ async def test_process_cherry_pick_retry_success(self, issue_comment_handler: Is mock_existing_cp_pr.title = "CherryPicked: [release-1.0] Some commit" mock_existing_cp_pr.body = "Cherry-pick from main, original PR: https://github.com/test/repo/pull/123" mock_existing_cp_pr.user = Mock() - mock_existing_cp_pr.user.type = "Bot" + mock_existing_cp_pr.user.login = "manage-repositories-app[bot]" + + issue_comment_handler.github_webhook.app_bot_login = "manage-repositories-app[bot]" mock_existing_cp_pr.edit = Mock() mock_existing_cp_pr.create_issue_comment = Mock() @@ -2223,7 +2225,9 @@ async def test_process_cherry_pick_retry_skips_human_created_pr( mock_human_pr.title = "CherryPicked: [release-1.0] Manual cherry-pick" mock_human_pr.body = "original PR: https://github.com/test/repo/pull/123" mock_human_pr.user = Mock() - mock_human_pr.user.type = "User" # Not a Bot + mock_human_pr.user.login = "human-user" # Not our app bot + + issue_comment_handler.github_webhook.app_bot_login = "manage-repositories-app[bot]" mock_human_pr.edit = Mock() issue_comment_handler.repository.get_pulls = Mock(return_value=[mock_human_pr]) diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index da102db0e..1b8f693fa 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -2901,6 +2901,7 @@ def mock_github_webhook(self) -> Mock: mock_webhook.clone_repo_dir = "/tmp/test-repo" mock_webhook.mask_sensitive = False mock_webhook.auto_verified_and_merged_users = ["bot-user[bot]"] + mock_webhook.app_bot_login = "manage-repositories-app[bot]" mock_webhook.repository_full_name = "test/repo" mock_webhook.ctx = None mock_webhook.config = Mock() @@ -2956,11 +2957,7 @@ async def test_rebase_pr_not_open(self, runner_handler: RunnerHandler, mock_pull @pytest.mark.asyncio async def test_rebase_pr_bot_pr_unauthorized(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: """Test rebase_pr rejects unauthorized users on bot-owned PRs.""" - mock_pull_request.user.login = "bot-user[bot]" - mock_pull_request.user.type = "Bot" - mock_cherry_pick_label = Mock() - mock_cherry_pick_label.name = "CherryPicked-from-main" - mock_pull_request.labels = [mock_cherry_pick_label] + mock_pull_request.user.login = "manage-repositories-app[bot]" mock_pull_request.assignees = [Mock(login="original-author")] with patch( @@ -2976,11 +2973,7 @@ async def test_rebase_pr_bot_pr_assignee_allowed( self, runner_handler: RunnerHandler, mock_pull_request: Mock ) -> None: """Test rebase_pr allows PR assignee on bot-owned PRs.""" - mock_pull_request.user.login = "bot-user[bot]" - mock_pull_request.user.type = "Bot" - mock_cherry_pick_label = Mock() - mock_cherry_pick_label.name = "CherryPicked-from-main" - mock_pull_request.labels = [mock_cherry_pick_label] + mock_pull_request.user.login = "manage-repositories-app[bot]" mock_pull_request.assignees = [Mock(login="test-user")] with ( @@ -3008,11 +3001,7 @@ async def test_rebase_pr_bot_pr_maintainer_allowed( self, runner_handler: RunnerHandler, mock_pull_request: Mock ) -> None: """Test rebase_pr allows maintainers on bot-owned PRs.""" - mock_pull_request.user.login = "bot-user[bot]" - mock_pull_request.user.type = "Bot" - mock_cherry_pick_label = Mock() - mock_cherry_pick_label.name = "CherryPicked-from-main" - mock_pull_request.labels = [mock_cherry_pick_label] + mock_pull_request.user.login = "manage-repositories-app[bot]" mock_pull_request.assignees = [Mock(login="original-author")] with ( From d311c4d69b99237616908b9b658954939be6a2de Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 9 Jun 2026 18:11:17 +0300 Subject: [PATCH 10/17] fix: move app_bot_login init from blocking __init__ to async process() - Replace blocking github_app_api.get_user().login in __init__ with async github_api_call in process() - Log warning on failure instead of silent empty fallback - Guard with "if not self.app_bot_login" to avoid redundant API calls --- webhook_server/libs/github_api.py | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 32077532a..10cb0b30b 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -195,11 +195,7 @@ def __init__(self, hook_data: dict[Any, Any], headers: Headers, logger: logging. github_app_api=github_app_api, repository=self.repository_full_name ) - # Store the app's bot login for identifying PRs created by our app - try: - self.app_bot_login: str = github_app_api.get_user().login - except Exception: - self.app_bot_login = "" + self.app_bot_login: str = "" # Initialized async in process() if not (self.repository or self.repository_by_github_app): self.logger.error(f"{self.log_prefix} Failed to get repository.") @@ -543,6 +539,23 @@ async def process(self) -> Any: # Initialize auto-verified users from API users (async operation) await self.add_api_users_to_auto_verified_and_merged_users() + # Initialize app bot login for bot-PR identification (async) + if not self.app_bot_login: + _github_app_api = get_repository_github_app_api( + config_=self.config, repository_name=self.repository_full_name + ) + if _github_app_api: + try: + self.app_bot_login = await github_api_call( + lambda: _github_app_api.get_user().login, + logger=self.logger, + log_prefix=self.log_prefix, + ) + except Exception: + self.logger.warning( + f"{self.log_prefix} Failed to get app bot login — bot-PR detection may not work" + ) + event_log: str = f"Event type: {self.github_event}. event ID: {self.x_github_delivery}" # Start webhook routing context step From bb191a155c536663f54857db75a24b69481db644 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 9 Jun 2026 18:18:32 +0300 Subject: [PATCH 11/17] =?UTF-8?q?fix:=20app=5Fbot=5Flogin=20error=20handli?= =?UTF-8?q?ng=20=E2=80=94=20use=20logger.exception,=20re-raise=20Cancelled?= =?UTF-8?q?Error,=20log=20missing=20API?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Use logger.exception instead of logger.warning to preserve traceback - Add except asyncio.CancelledError: raise before broad except - Log debug message when GitHub App API is not available --- webhook_server/libs/github_api.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 10cb0b30b..3cb1b075b 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -551,10 +551,14 @@ async def process(self) -> Any: logger=self.logger, log_prefix=self.log_prefix, ) + except asyncio.CancelledError: + raise except Exception: - self.logger.warning( + self.logger.exception( f"{self.log_prefix} Failed to get app bot login — bot-PR detection may not work" ) + else: + self.logger.debug(f"{self.log_prefix} No GitHub App API available — app_bot_login not set") event_log: str = f"Event type: {self.github_event}. event ID: {self.x_github_delivery}" From 0f884d0eb7460a6854074f64182b7144129012c0 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Wed, 10 Jun 2026 18:54:07 +0300 Subject: [PATCH 12/17] fix: wrap get_repository_github_app_api in github_api_call and guard author check on empty app_bot_login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - github_api.py: get_repository_github_app_api() does file I/O (reads private key) and GitHub App setup — was called synchronously in async process(), blocking the event loop. Now wrapped with github_api_call() for non-blocking + retry. - issue_comment_handler.py: when app_bot_login is empty (init failed or fallback token used), the author check 'pr_author_login != app_bot_login' would skip ALL PRs since no login matches empty string. Now guards the author check behind 'if self.github_webhook.app_bot_login', falling back to title+body match only. --- webhook_server/libs/github_api.py | 8 ++++-- .../libs/handlers/issue_comment_handler.py | 27 +++++++++++-------- 2 files changed, 22 insertions(+), 13 deletions(-) diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 3cb1b075b..b7e6d94e0 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -541,8 +541,12 @@ async def process(self) -> Any: # Initialize app bot login for bot-PR identification (async) if not self.app_bot_login: - _github_app_api = get_repository_github_app_api( - config_=self.config, repository_name=self.repository_full_name + _github_app_api = await github_api_call( + get_repository_github_app_api, + config_=self.config, + repository_name=self.repository_full_name, + logger=self.logger, + log_prefix=self.log_prefix, ) if _github_app_api: try: diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index 2e2cebd10..7c316158f 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -697,19 +697,24 @@ async def process_cherry_pick_retry_command( self.logger.debug(f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} title matches prefix") - # Verify the PR was created by our app's bot - pr_author_login = await github_api_call( - lambda _pr=open_pr: _pr.user.login, logger=self.logger, log_prefix=self.log_prefix - ) - if pr_author_login != self.github_webhook.app_bot_login: - self.logger.debug( - f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author " - f"'{pr_author_login}' is not our app bot " - f"'{self.github_webhook.app_bot_login}', skipping" + # Verify the PR was created by our app's bot (if app_bot_login is available) + if self.github_webhook.app_bot_login: + pr_author_login = await github_api_call( + lambda _pr=open_pr: _pr.user.login, logger=self.logger, log_prefix=self.log_prefix ) - continue + if pr_author_login != self.github_webhook.app_bot_login: + self.logger.debug( + f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author " + f"'{pr_author_login}' is not our app bot " + f"'{self.github_webhook.app_bot_login}', skipping" + ) + continue - self.logger.debug(f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author matches our app bot") + self.logger.debug( + f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author matches our app bot" + ) + else: + self.logger.debug(f"{self.log_prefix} Cherry-pick retry: app_bot_login not set, skipping author check") # Check if the PR body references the original PR pr_body = await github_api_call( From caddc3c8c2130077e0fe621cecfcb69cd9f7f3a0 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Wed, 10 Jun 2026 18:58:01 +0300 Subject: [PATCH 13/17] fix: abort cherry-pick retry and rebase when app_bot_login is not set MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - issue_comment_handler.py: add early-exit guard before the get_pulls loop in cherry-pick retry. If app_bot_login is empty, log ERROR and post a PR comment explaining the failure, then return. Reverts the previous silent fallback that skipped the author check. - runner_handler.py: add early-exit guard in rebase_pr() before the is_bot_pr check. If app_bot_login is empty, log ERROR and post a PR comment, then return. Without this, bot PRs would be treated as regular PRs (anyone could rebase) since empty string never matches any login. No silent fallbacks — if we cannot verify PR ownership, we stop. --- .../libs/handlers/issue_comment_handler.py | 42 ++++++++++++------- .../libs/handlers/runner_handler.py | 15 +++++++ 2 files changed, 41 insertions(+), 16 deletions(-) diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index 7c316158f..5314e8a62 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -669,6 +669,21 @@ async def process_cherry_pick_retry_command( return # Find and close existing failed cherry-pick PR for this branch (created by bot) + # Abort early if app_bot_login is not set — we cannot verify PR ownership + if not self.github_webhook.app_bot_login: + self.logger.error( + f"{self.log_prefix} Cannot identify app bot — app_bot_login not set. " + "Cherry-pick retry cannot verify PR ownership. Aborting close step." + ) + await github_api_call( + pull_request.create_issue_comment, + "Cherry-pick retry failed: cannot identify app bot for PR ownership verification. " + "Please check GitHub App configuration.", + logger=self.logger, + log_prefix=self.log_prefix, + ) + return + pr_title_prefix = f"CherryPicked: [{target_branch}]" original_pr_url = await github_api_call( lambda: pull_request.html_url, logger=self.logger, log_prefix=self.log_prefix @@ -697,24 +712,19 @@ async def process_cherry_pick_retry_command( self.logger.debug(f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} title matches prefix") - # Verify the PR was created by our app's bot (if app_bot_login is available) - if self.github_webhook.app_bot_login: - pr_author_login = await github_api_call( - lambda _pr=open_pr: _pr.user.login, logger=self.logger, log_prefix=self.log_prefix - ) - if pr_author_login != self.github_webhook.app_bot_login: - self.logger.debug( - f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author " - f"'{pr_author_login}' is not our app bot " - f"'{self.github_webhook.app_bot_login}', skipping" - ) - continue - + # Verify the PR was created by our app's bot + pr_author_login = await github_api_call( + lambda _pr=open_pr: _pr.user.login, logger=self.logger, log_prefix=self.log_prefix + ) + if pr_author_login != self.github_webhook.app_bot_login: self.logger.debug( - f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author matches our app bot" + f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author " + f"'{pr_author_login}' is not our app bot " + f"'{self.github_webhook.app_bot_login}', skipping" ) - else: - self.logger.debug(f"{self.log_prefix} Cherry-pick retry: app_bot_login not set, skipping author check") + continue + + self.logger.debug(f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author matches our app bot") # Check if the PR body references the original PR pr_body = await github_api_call( diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 828da8de3..66ec3bbea 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -1622,6 +1622,21 @@ async def rebase_pr(self, pull_request: PullRequest, reviewed_user: str) -> None ) # Check if PR was created by our app's bot + # If app_bot_login is not set, we cannot verify bot ownership — abort for safety + if not self.github_webhook.app_bot_login: + self.logger.error( + f"{self.log_prefix} Cannot identify app bot — app_bot_login not set. " + "Rebase cannot verify bot-PR ownership. Aborting." + ) + await github_api_call( + pull_request.create_issue_comment, + "Rebase failed: cannot identify app bot for PR ownership verification. " + "Please check GitHub App configuration.", + logger=self.logger, + log_prefix=self.log_prefix, + ) + return + is_bot_pr = pr_user_login == self.github_webhook.app_bot_login if is_bot_pr: From 5139ee8e6674e014dc6c74c52ab96fb01e534465 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Wed, 10 Jun 2026 19:25:02 +0300 Subject: [PATCH 14/17] =?UTF-8?q?fix:=20refine=20app=5Fbot=5Flogin=20guard?= =?UTF-8?q?s=20=E2=80=94=20don't=20block=20normal=20user=20operations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - runner_handler.py rebase_pr(): remove top-level abort that blocked ALL rebases when app_bot_login was empty (including normal user PRs). Now: if app_bot_login is set, classify bot PRs normally; if not set, treat PR as user-owned with a warning log. Safe because is_user_valid_to_run_commands already gates access to the /rebase command. - issue_comment_handler.py cherry-pick retry: remove top-level abort that blocked the entire retry flow. Move the guard inside the loop after title match — if app_bot_login is empty, break out of the close loop with an error log (don't close any PR we can't verify ownership of), but still proceed to re-run the cherry-pick. --- .../libs/handlers/issue_comment_handler.py | 23 +++++++------------ .../libs/handlers/runner_handler.py | 22 ++++++------------ 2 files changed, 15 insertions(+), 30 deletions(-) diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index 5314e8a62..e3f2b7b6a 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -669,21 +669,6 @@ async def process_cherry_pick_retry_command( return # Find and close existing failed cherry-pick PR for this branch (created by bot) - # Abort early if app_bot_login is not set — we cannot verify PR ownership - if not self.github_webhook.app_bot_login: - self.logger.error( - f"{self.log_prefix} Cannot identify app bot — app_bot_login not set. " - "Cherry-pick retry cannot verify PR ownership. Aborting close step." - ) - await github_api_call( - pull_request.create_issue_comment, - "Cherry-pick retry failed: cannot identify app bot for PR ownership verification. " - "Please check GitHub App configuration.", - logger=self.logger, - log_prefix=self.log_prefix, - ) - return - pr_title_prefix = f"CherryPicked: [{target_branch}]" original_pr_url = await github_api_call( lambda: pull_request.html_url, logger=self.logger, log_prefix=self.log_prefix @@ -713,6 +698,14 @@ async def process_cherry_pick_retry_command( self.logger.debug(f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} title matches prefix") # Verify the PR was created by our app's bot + # If app_bot_login is not set, skip the close step entirely — can't verify ownership + if not self.github_webhook.app_bot_login: + self.logger.error( + f"{self.log_prefix} Cherry-pick retry: app_bot_login not set — " + "cannot verify PR ownership, skipping close step" + ) + break + pr_author_login = await github_api_call( lambda _pr=open_pr: _pr.user.login, logger=self.logger, log_prefix=self.log_prefix ) diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 66ec3bbea..3ce44105f 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -1622,22 +1622,14 @@ async def rebase_pr(self, pull_request: PullRequest, reviewed_user: str) -> None ) # Check if PR was created by our app's bot - # If app_bot_login is not set, we cannot verify bot ownership — abort for safety - if not self.github_webhook.app_bot_login: - self.logger.error( - f"{self.log_prefix} Cannot identify app bot — app_bot_login not set. " - "Rebase cannot verify bot-PR ownership. Aborting." - ) - await github_api_call( - pull_request.create_issue_comment, - "Rebase failed: cannot identify app bot for PR ownership verification. " - "Please check GitHub App configuration.", - logger=self.logger, - log_prefix=self.log_prefix, + # If app_bot_login is not set, we can't detect bot PRs — treat as user-owned + if self.github_webhook.app_bot_login: + is_bot_pr = pr_user_login == self.github_webhook.app_bot_login + else: + is_bot_pr = False + self.logger.warning( + f"{self.log_prefix} app_bot_login not set — cannot detect bot PRs, treating as user-owned" ) - return - - is_bot_pr = pr_user_login == self.github_webhook.app_bot_login if is_bot_pr: # For bot-owned PRs, validate the user is the PR assignee or a maintainer From 1ac10fd9e90f1da5cc6e879a2f46ef02a82a9cab Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Wed, 10 Jun 2026 19:59:11 +0300 Subject: [PATCH 15/17] fix: replace defensive merged_at check with pull_request.merged, quote git refs, lazy PR iteration - issue_comment_handler.py: replace hook_data["issue"]["pull_request"]["merged_at"] with `await github_api_call(lambda: pull_request.merged, ...)` at both cherry-pick (line 578) and cherry-pick-retry (line 644) merge checks. Direct PR object access instead of defensive .get() chaining on webhook payload. - runner_handler.py rebase_pr(): quote all interpolated git refs (head_ref, base_ref) and worktree_path with shlex.quote() to prevent shlex.split() breakage on branch names with spaces or special chars. Also quote worktree_path in cherry_pick() pre-commit command. - issue_comment_handler.py cherry-pick retry: remove list() wrapper from get_pulls(state="open") to iterate lazily via PyGithub's PaginatedList, fetching pages on demand and breaking on first match. - test_issue_comment_handler.py: update all tests to set mock_pull_request.merged instead of manipulating hook_data["issue"]["pull_request"]["merged_at"]. --- .../libs/handlers/issue_comment_handler.py | 10 +- .../libs/handlers/runner_handler.py | 12 +-- .../tests/test_issue_comment_handler.py | 91 ++++++------------- 3 files changed, 42 insertions(+), 71 deletions(-) diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index e3f2b7b6a..5cf46c0c4 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -575,7 +575,8 @@ async def process_cherry_pick_command( cp_labels: list[str] = [f"{CHERRY_PICK_LABEL_PREFIX}{_branch}" for _branch in _branches_to_process] - if not self.hook_data["issue"].get("pull_request", {}).get("merged_at"): + is_merged = await github_api_call(lambda: pull_request.merged, logger=self.logger, log_prefix=self.log_prefix) + if not is_merged: info_msg: str = f""" Cherry-pick requested for PR: `{pull_request.title}` by user `{reviewed_user}` Adding label/s `{" ".join(cp_labels)}` for automatic cherry-pick once the PR is merged @@ -641,7 +642,8 @@ async def process_cherry_pick_retry_command( self.logger.info(f"{self.log_prefix} Processing cherry-pick retry for branch {target_branch}") # Validate PR is merged - if not self.hook_data["issue"].get("pull_request", {}).get("merged_at"): + is_merged = await github_api_call(lambda: pull_request.merged, logger=self.logger, log_prefix=self.log_prefix) + if not is_merged: msg = "Cherry-pick retry can only be used on merged PRs" self.logger.debug(f"{self.log_prefix} {msg}") await github_api_call( @@ -678,12 +680,12 @@ async def process_cherry_pick_retry_command( f"'{pr_title_prefix}' referencing {original_pr_url}" ) open_pulls = await github_api_call( - lambda: list(self.repository.get_pulls(state="open")), + lambda: self.repository.get_pulls(state="open"), logger=self.logger, log_prefix=self.log_prefix, ) - self.logger.debug(f"{self.log_prefix} Cherry-pick retry: found {len(open_pulls)} open PRs to scan") closed_old_pr = False + # Iterate lazily — PyGithub's PaginatedList fetches pages on demand for open_pr in open_pulls: pr_title = await github_api_call( lambda _pr=open_pr: _pr.title, logger=self.logger, log_prefix=self.log_prefix diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 3ce44105f..9f22f2453 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -1264,7 +1264,7 @@ async def cherry_pick( # Run pre-commit auto-fix if enabled for the repo if self.github_webhook.pre_commit: self.logger.info(f"{self.log_prefix} Running pre-commit on cherry-pick worktree") - pre_commit_cmd = f"uvx --directory {worktree_path} {PREK_STR} run --all-files" + pre_commit_cmd = f"uvx --directory {shlex.quote(worktree_path)} {PREK_STR} run --all-files" rc_pc, _out_pc, _err_pc = await run_command( command=pre_commit_cmd, log_prefix=self.log_prefix, @@ -1684,11 +1684,11 @@ async def rebase_pr(self, pull_request: PullRequest, reviewed_user: str) -> None ) return - git_cmd = f"git --work-tree={worktree_path} --git-dir={worktree_path}/.git" + git_cmd = f"git --work-tree={shlex.quote(worktree_path)} --git-dir={shlex.quote(worktree_path + '/.git')}" # Checkout the PR head branch rc, out, err = await run_command( - command=f"{git_cmd} checkout {head_ref}", + command=f"{git_cmd} checkout {shlex.quote(head_ref)}", log_prefix=self.log_prefix, redact_secrets=[github_token], mask_sensitive=self.github_webhook.mask_sensitive, @@ -1704,7 +1704,7 @@ async def rebase_pr(self, pull_request: PullRequest, reviewed_user: str) -> None # Fetch the base branch rc, out, err = await run_command( - command=f"{git_cmd} fetch origin {base_ref}", + command=f"{git_cmd} fetch origin {shlex.quote(base_ref)}", log_prefix=self.log_prefix, redact_secrets=[github_token], mask_sensitive=self.github_webhook.mask_sensitive, @@ -1720,7 +1720,7 @@ async def rebase_pr(self, pull_request: PullRequest, reviewed_user: str) -> None # Rebase onto base branch rc, out, err = await run_command( - command=f"{git_cmd} rebase origin/{base_ref}", + command=f"{git_cmd} rebase origin/{shlex.quote(base_ref)}", log_prefix=self.log_prefix, redact_secrets=[github_token], mask_sensitive=self.github_webhook.mask_sensitive, @@ -1748,7 +1748,7 @@ async def rebase_pr(self, pull_request: PullRequest, reviewed_user: str) -> None # Force push the rebased branch rc, out, err = await run_command( - command=f"{git_cmd} push --force-with-lease origin {head_ref}", + command=f"{git_cmd} push --force-with-lease origin {shlex.quote(head_ref)}", log_prefix=self.log_prefix, redact_secrets=[github_token], mask_sensitive=self.github_webhook.mask_sensitive, diff --git a/webhook_server/tests/test_issue_comment_handler.py b/webhook_server/tests/test_issue_comment_handler.py index 15af4d144..10b740443 100644 --- a/webhook_server/tests/test_issue_comment_handler.py +++ b/webhook_server/tests/test_issue_comment_handler.py @@ -798,24 +798,23 @@ async def test_process_cherry_pick_command_existing_branches( mock_pull_request = Mock() mock_pull_request.title = "Test PR" mock_pull_request.labels = [] - # Patch is_merged as a method - with patch.object(mock_pull_request, "is_merged", new=Mock(return_value=False)): - with patch.object(issue_comment_handler.repository, "get_branch") as mock_get_branch: - with patch.object(mock_pull_request, "create_issue_comment") as mock_comment: - 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 branch2", - reviewed_user="test-user", - ) - mock_get_branch.assert_any_call("branch1") - mock_get_branch.assert_any_call("branch2") - mock_comment.assert_called_once() - assert mock_add_label.call_count == 2 + mock_pull_request.merged = False + with patch.object(issue_comment_handler.repository, "get_branch") as mock_get_branch: + with patch.object(mock_pull_request, "create_issue_comment") as mock_comment: + 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 branch2", + reviewed_user="test-user", + ) + mock_get_branch.assert_any_call("branch1") + mock_get_branch.assert_any_call("branch2") + mock_comment.assert_called_once() + assert mock_add_label.call_count == 2 @pytest.mark.asyncio async def test_process_cherry_pick_command_non_existing_branches( @@ -840,8 +839,7 @@ async def test_process_cherry_pick_command_merged_pr(self, issue_comment_handler """Test processing cherry pick command for merged PR.""" mock_pull_request = Mock() mock_pull_request.labels = [] - # Set merged_at in hook_data to simulate a merged PR at comment time - issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2026-01-01T00:00:00Z"} + mock_pull_request.merged = True with patch.object(issue_comment_handler.repository, "get_branch"): with patch.object( issue_comment_handler.runner_handler, @@ -883,9 +881,8 @@ async def test_process_cherry_pick_command_merged_pr_multiple_branches( mock_pull_request = Mock() mock_pull_request.title = "Test PR" mock_pull_request.labels = [] + mock_pull_request.merged = True - # Set merged_at in hook_data to simulate a merged PR at comment time - issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2026-01-01T00:00:00Z"} with patch.object(issue_comment_handler.repository, "get_branch"): with patch.object( issue_comment_handler.runner_handler, @@ -943,8 +940,7 @@ async def test_process_cherry_pick_command_merged_pr_assign_disabled( issue_comment_handler.github_webhook.cherry_pick_assign_to_pr_author = False mock_pull_request = Mock() mock_pull_request.labels = [] - # Set merged_at in hook_data to simulate a merged PR at comment time - issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2026-01-01T00:00:00Z"} + mock_pull_request.merged = True with patch.object(issue_comment_handler.repository, "get_branch"): with patch.object( issue_comment_handler.runner_handler, @@ -980,9 +976,8 @@ async def test_process_cherry_pick_command_skips_already_cherry_picked( existing_label = Mock() existing_label.name = f"{CHERRY_PICK_LABEL_PREFIX}branch1" mock_pull_request.labels = [existing_label] + mock_pull_request.merged = True - # Set merged_at in hook_data to simulate a merged PR at comment time - issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2026-01-01T00:00:00Z"} with ( patch.object(issue_comment_handler.repository, "get_branch"), patch.object( @@ -1036,8 +1031,7 @@ async def test_process_cherry_pick_command_all_already_cherry_picked( label2 = Mock() label2.name = f"{CHERRY_PICK_LABEL_PREFIX}branch2" mock_pull_request.labels = [label1, label2] - - issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2026-01-01T00:00:00Z"} + mock_pull_request.merged = True with ( patch.object(issue_comment_handler.repository, "get_branch"), patch.object( @@ -1077,8 +1071,7 @@ async def test_process_cherry_pick_command_skips_when_add_label_fails( mock_pull_request = Mock() mock_pull_request.title = "Test PR" mock_pull_request.labels = [] # No existing labels in snapshot - - issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2024-01-01T00:00:00Z"} + mock_pull_request.merged = True with ( patch.object(issue_comment_handler.repository, "get_branch"), @@ -1114,9 +1107,7 @@ async def test_process_cherry_pick_command_skips_already_cherry_picked_unmerged( existing_label = Mock() existing_label.name = f"{CHERRY_PICK_LABEL_PREFIX}branch1" mock_pull_request.labels = [existing_label] - - # Set merged_at to None to simulate an unmerged PR - issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": None} + mock_pull_request.merged = False with ( patch.object(issue_comment_handler.repository, "get_branch"), patch.object( @@ -2041,8 +2032,7 @@ async def test_process_cherry_pick_retry_multiple_branches_rejected( async def test_process_cherry_pick_retry_not_merged(self, issue_comment_handler: IssueCommentHandler) -> None: """Test cherry-pick-retry rejects if PR is not merged.""" mock_pull_request = Mock() - # PR not merged - issue_comment_handler.hook_data["issue"] = {"number": 123, "pull_request": {}} + mock_pull_request.merged = False with patch("asyncio.to_thread", new_callable=AsyncMock, side_effect=lambda f, *a, **k: f(*a, **k)): await issue_comment_handler.process_cherry_pick_retry_command( @@ -2059,11 +2049,7 @@ async def test_process_cherry_pick_retry_label_missing(self, issue_comment_handl """Test cherry-pick-retry rejects if the cherry-pick label is missing.""" mock_pull_request = Mock() mock_pull_request.labels = [] # No labels - # PR is merged - issue_comment_handler.hook_data["issue"] = { - "number": 123, - "pull_request": {"merged_at": "2024-01-01T00:00:00Z"}, - } + mock_pull_request.merged = True with patch("asyncio.to_thread", new_callable=AsyncMock, side_effect=lambda f, *a, **k: f(*a, **k)): await issue_comment_handler.process_cherry_pick_retry_command( @@ -2084,12 +2070,7 @@ async def test_process_cherry_pick_retry_success(self, issue_comment_handler: Is mock_label = Mock() mock_label.name = f"{CHERRY_PICK_LABEL_PREFIX}release-1.0" mock_pull_request.labels = [mock_label] - - # PR is merged - issue_comment_handler.hook_data["issue"] = { - "number": 123, - "pull_request": {"merged_at": "2024-01-01T00:00:00Z"}, - } + mock_pull_request.merged = True # Mock existing cherry-pick PR to close (created by bot) mock_existing_cp_pr = Mock() @@ -2135,11 +2116,7 @@ async def test_process_cherry_pick_retry_no_existing_pr(self, issue_comment_hand mock_label = Mock() mock_label.name = f"{CHERRY_PICK_LABEL_PREFIX}release-1.0" mock_pull_request.labels = [mock_label] - - issue_comment_handler.hook_data["issue"] = { - "number": 123, - "pull_request": {"merged_at": "2024-01-01T00:00:00Z"}, - } + mock_pull_request.merged = True # No open PRs matching issue_comment_handler.repository.get_pulls = Mock(return_value=[]) @@ -2170,11 +2147,7 @@ async def test_process_cherry_pick_retry_skips_pr_with_different_body_url( mock_label = Mock() mock_label.name = f"{CHERRY_PICK_LABEL_PREFIX}release-1.0" mock_pull_request.labels = [mock_label] - - issue_comment_handler.hook_data["issue"] = { - "number": 123, - "pull_request": {"merged_at": "2024-01-01T00:00:00Z"}, - } + mock_pull_request.merged = True # PR with matching title but body references a different original PR mock_other_pr = Mock() @@ -2213,11 +2186,7 @@ async def test_process_cherry_pick_retry_skips_human_created_pr( mock_label = Mock() mock_label.name = f"{CHERRY_PICK_LABEL_PREFIX}release-1.0" mock_pull_request.labels = [mock_label] - - issue_comment_handler.hook_data["issue"] = { - "number": 123, - "pull_request": {"merged_at": "2024-01-01T00:00:00Z"}, - } + mock_pull_request.merged = True # Human-created PR with matching title and body URL mock_human_pr = Mock() From e5044afca66de63ea26821faf93a5f22473235bd Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Wed, 10 Jun 2026 20:06:48 +0300 Subject: [PATCH 16/17] fix: materialize PaginatedList in thread pool and use direct property access MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Convert get_pulls(state="open") back to list() inside github_api_call() to avoid blocking event loop on PaginatedList page boundary fetches. - Remove github_api_call lambda wrappers on .title, .user.login, .body — after list() materialization these properties are cached and safe to access directly. - Restore "found N open PRs to scan" debug log line. --- .../libs/handlers/issue_comment_handler.py | 24 +++++++------------ 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index 5cf46c0c4..23090d20f 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -679,21 +679,20 @@ async def process_cherry_pick_retry_command( f"{self.log_prefix} Cherry-pick retry: looking for open PRs with title prefix " f"'{pr_title_prefix}' referencing {original_pr_url}" ) + # Convert PaginatedList to list inside github_api_call to avoid blocking iteration. + # After materialization, basic properties (.title, .number, .user.login, .body) are cached. open_pulls = await github_api_call( - lambda: self.repository.get_pulls(state="open"), + lambda: list(self.repository.get_pulls(state="open")), logger=self.logger, log_prefix=self.log_prefix, ) + self.logger.debug(f"{self.log_prefix} Cherry-pick retry: found {len(open_pulls)} open PRs to scan") closed_old_pr = False - # Iterate lazily — PyGithub's PaginatedList fetches pages on demand for open_pr in open_pulls: - pr_title = await github_api_call( - lambda _pr=open_pr: _pr.title, logger=self.logger, log_prefix=self.log_prefix - ) - if not pr_title.startswith(pr_title_prefix): + if not open_pr.title.startswith(pr_title_prefix): self.logger.debug( f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} title " - f"'{pr_title}' does not match prefix '{pr_title_prefix}', skipping" + f"'{open_pr.title}' does not match prefix '{pr_title_prefix}', skipping" ) continue @@ -708,13 +707,10 @@ async def process_cherry_pick_retry_command( ) break - pr_author_login = await github_api_call( - lambda _pr=open_pr: _pr.user.login, logger=self.logger, log_prefix=self.log_prefix - ) - if pr_author_login != self.github_webhook.app_bot_login: + if open_pr.user.login != self.github_webhook.app_bot_login: self.logger.debug( f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author " - f"'{pr_author_login}' is not our app bot " + f"'{open_pr.user.login}' is not our app bot " f"'{self.github_webhook.app_bot_login}', skipping" ) continue @@ -722,9 +718,7 @@ async def process_cherry_pick_retry_command( self.logger.debug(f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} author matches our app bot") # Check if the PR body references the original PR - pr_body = await github_api_call( - lambda _pr=open_pr: _pr.body or "", logger=self.logger, log_prefix=self.log_prefix - ) + pr_body = open_pr.body or "" if original_pr_url not in pr_body: self.logger.debug( f"{self.log_prefix} Cherry-pick retry: PR #{open_pr.number} body does not " From 47a6efde34fdbd7018126e5113130d74aab56107 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Wed, 10 Jun 2026 20:38:54 +0300 Subject: [PATCH 17/17] fix: guard empty target_branch in cherry-pick-retry and fix generic auth message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - issue_comment_handler.py: add explicit empty-string check after .strip() in process_cherry_pick_retry_command. Whitespace-only args like "/cherry-pick-retry " bypass the shared arg parser check since whitespace is truthy, but strip to empty string. - owners_files_handler.py: change "not allowed to run retest commands" to "not allowed to run commands" — the method is now used for /cherry-pick, /rebase, /verified, /wip, not just retest. - test_owners_files_handler.py: update assertion to match new message. --- webhook_server/libs/handlers/issue_comment_handler.py | 8 ++++++++ webhook_server/libs/handlers/owners_files_handler.py | 2 +- webhook_server/tests/test_owners_files_handler.py | 5 +---- 3 files changed, 10 insertions(+), 5 deletions(-) diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index 23090d20f..1222ac3b6 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -630,6 +630,14 @@ async def process_cherry_pick_retry_command( """ target_branch = command_args.strip() + if not target_branch: + msg = "cherry-pick-retry requires a branch name" + self.logger.debug(f"{self.log_prefix} {msg}") + await github_api_call( + pull_request.create_issue_comment, msg, logger=self.logger, log_prefix=self.log_prefix + ) + return + # Validate exactly one branch name if " " in target_branch: msg = "cherry-pick-retry accepts exactly one branch name" diff --git a/webhook_server/libs/handlers/owners_files_handler.py b/webhook_server/libs/handlers/owners_files_handler.py index d9d27b20a..ef54b946c 100644 --- a/webhook_server/libs/handlers/owners_files_handler.py +++ b/webhook_server/libs/handlers/owners_files_handler.py @@ -481,7 +481,7 @@ async def is_user_valid_to_run_commands(self, pull_request: PullRequest, reviewe allow_user_comment = f"/{COMMAND_ADD_ALLOWED_USER_STR} @{reviewed_user}" comment_msg = f""" -{reviewed_user} is not allowed to run retest commands. +{reviewed_user} is not allowed to run commands. maintainers can allow it by comment `{allow_user_comment}` Maintainers: - {"\n - ".join(allowed_user_to_approve)} diff --git a/webhook_server/tests/test_owners_files_handler.py b/webhook_server/tests/test_owners_files_handler.py index e8d7f5551..13ce2af74 100644 --- a/webhook_server/tests/test_owners_files_handler.py +++ b/webhook_server/tests/test_owners_files_handler.py @@ -578,10 +578,7 @@ async def test_is_user_valid_to_run_commands_invalid_user_no_approval( assert result is False mock_create_comment.assert_called_once() - assert ( - "invalid_user is not allowed to run retest commands" - in mock_create_comment.call_args[0][0] - ) + assert "invalid_user is not allowed to run commands" in mock_create_comment.call_args[0][0] @pytest.mark.asyncio async def test_valid_users_to_run_commands(self, owners_file_handler: OwnersFileHandler) -> None: