diff --git a/webhook_server/libs/github_api.py b/webhook_server/libs/github_api.py index 7bd17d8c3..b7e6d94e0 100644 --- a/webhook_server/libs/github_api.py +++ b/webhook_server/libs/github_api.py @@ -195,6 +195,8 @@ def __init__(self, hook_data: dict[Any, Any], headers: Headers, logger: logging. github_app_api=github_app_api, repository=self.repository_full_name ) + 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.") return @@ -537,6 +539,31 @@ 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 = 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: + self.app_bot_login = await github_api_call( + lambda: _github_app_api.get_user().login, + logger=self.logger, + log_prefix=self.log_prefix, + ) + except asyncio.CancelledError: + raise + except Exception: + 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}" # Start webhook routing context step diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index 8796aefa2..1222ac3b6 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) @@ -526,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 @@ -558,6 +608,165 @@ 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() + + 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" + 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 + 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( + 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}]" + 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}" + ) + # 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: 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: + 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"'{open_pr.title}' does not match prefix '{pr_title_prefix}', skipping" + ) + continue + + 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 + + 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"'{open_pr.user.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") + + # Check if the PR body references the original PR + 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 " + f"contain original PR URL '{original_pr_url}', skipping" + ) + 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}") + 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/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/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..9f22f2453 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -1261,6 +1261,107 @@ 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 {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, + 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, 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 not rc_diff: + # git diff itself failed — report as worktree error, not pre-commit + 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, + "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", + log_prefix=self.log_prefix, + redact_secrets=[github_token], + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc_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')}" + " --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.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: + # 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") + # Push the branch rc, out, err = await run_command( command=push_command, @@ -1484,6 +1585,191 @@ 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 + + # 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 + ) + + # Check if PR was created by our app's bot + # 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" + ) + + 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 + 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) + 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={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 {shlex.quote(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 {shlex.quote(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/{shlex.quote(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 {shlex.quote(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..10b740443 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, @@ -796,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( @@ -838,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, @@ -881,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, @@ -941,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, @@ -978,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( @@ -1034,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( @@ -1075,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"), @@ -1112,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( @@ -1733,3 +1726,551 @@ 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_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.""" + mock_pull_request = Mock() + 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( + 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 + 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( + 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] + mock_pull_request.merged = True + + # 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 = "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() + + 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] + mock_pull_request.merged = True + + # 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_pr_with_different_body_url( + self, issue_comment_handler: IssueCommentHandler + ) -> None: + """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() + mock_label.name = f"{CHERRY_PICK_LABEL_PREFIX}release-1.0" + mock_pull_request.labels = [mock_label] + mock_pull_request.merged = True + + # 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.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)), + 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", + ) + # 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() + + @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] + mock_pull_request.merged = True + + # 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.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]) + + 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_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: diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index 07e62542f..1b8f693fa 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -2883,3 +2883,588 @@ 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.app_bot_login = "manage-repositories-app[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) + 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.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.user.type = "User" + mock_pr.assignees = [] + mock_pr.labels = [] # No labels by default + 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 = "manage-repositories-app[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 = "manage-repositories-app[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 = "manage-repositories-app[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() + + @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.""" + + @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}" + # 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( + 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}" + + @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) 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"