From 9f5d08b7f990014587da7494c7172feb3a236045 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 31 Mar 2026 12:36:09 +0300 Subject: [PATCH 1/8] feat: detect clean rebase on PR synchronize and preserve review labels When a PR is synchronized, detect if it's a clean rebase (same code changes replayed on a newer base) by comparing SHA-256 hashes of git diff output. On clean rebase, preserve review labels (approved-by, lgtm-by, verified, commented-by, changes-requested-by) and post a comment listing preserved labels. Closes #1059 --- .../libs/handlers/pull_request_handler.py | 146 +++- .../tests/test_clean_rebase_detection.py | 656 ++++++++++++++++++ .../tests/test_pull_request_handler.py | 21 +- 3 files changed, 813 insertions(+), 10 deletions(-) create mode 100644 webhook_server/tests/test_clean_rebase_detection.py diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 43e5a3a8..1ae394d7 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -1,6 +1,8 @@ from __future__ import annotations import asyncio +import hashlib +import shlex import traceback from collections.abc import Coroutine from typing import TYPE_CHECKING, Any @@ -40,6 +42,7 @@ VERIFIED_LABEL_STR, WIP_STR, ) +from webhook_server.utils.helpers import run_command if TYPE_CHECKING: from webhook_server.libs.github_api import GithubWebhook @@ -68,6 +71,103 @@ def __init__(self, github_webhook: GithubWebhook, owners_file_handler: OwnersFil github_webhook=self.github_webhook, owners_file_handler=self.owners_file_handler ) + async def _is_clean_rebase(self, pull_request: PullRequest) -> bool: + """Detect whether a synchronize event is a clean rebase (same code changes on a newer base). + + Compares the sha256 hash of the diff between merge-base..HEAD for both + the old head (before) and the new head (after). If the hashes match, + the push was a pure rebase with no code changes. + + Args: + pull_request: The GitHub pull request object. + + Returns: + True if the push is a clean rebase, False otherwise. + On any git command failure, returns False (conservative). + """ + if not self.github_webhook._repo_cloned: + self.logger.debug(f"{self.log_prefix} Clean rebase detection: skipped, repository not cloned") + return False + + before_sha: str = self.hook_data["before"] + after_sha: str = self.hook_data["after"] + base_ref: str = await asyncio.to_thread(lambda: pull_request.base.ref) + clone_dir: str = self.github_webhook.clone_repo_dir + + clone_dir_q = shlex.quote(clone_dir) + before_sha_q = shlex.quote(before_sha) + after_sha_q = shlex.quote(after_sha) + base_ref_q = shlex.quote(f"origin/{base_ref}") + + # Step 1: Fetch the old head SHA (may not be in the clone) + rc, _, _ = await run_command( + command=f"git -C {clone_dir_q} fetch origin {before_sha_q}", + log_prefix=self.log_prefix, + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc: + self.logger.warning(f"{self.log_prefix} Clean rebase detection: failed to fetch old head {before_sha[:7]}") + return False + + # Step 2: Get merge-base for old head + rc, old_merge_base_out, _ = await run_command( + command=f"git -C {clone_dir_q} merge-base {base_ref_q} {before_sha_q}", + log_prefix=self.log_prefix, + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc: + self.logger.warning( + f"{self.log_prefix} Clean rebase detection: failed to find merge-base for old head {before_sha[:7]}" + ) + return False + old_merge_base = old_merge_base_out.strip() + old_merge_base_q = shlex.quote(old_merge_base) + + # Step 3: Get merge-base for new head + rc, new_merge_base_out, _ = await run_command( + command=f"git -C {clone_dir_q} merge-base {base_ref_q} {after_sha_q}", + log_prefix=self.log_prefix, + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc: + self.logger.warning( + f"{self.log_prefix} Clean rebase detection: failed to find merge-base for new head {after_sha[:7]}" + ) + return False + new_merge_base = new_merge_base_out.strip() + new_merge_base_q = shlex.quote(new_merge_base) + + # Step 4: Compute diff hash for old range + rc, old_diff, _ = await run_command( + command=f"git -C {clone_dir_q} diff {old_merge_base_q}..{before_sha_q}", + log_prefix=self.log_prefix, + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc: + self.logger.warning(f"{self.log_prefix} Clean rebase detection: failed to compute diff for old range") + return False + + # Step 5: Compute diff hash for new range + rc, new_diff, _ = await run_command( + command=f"git -C {clone_dir_q} diff {new_merge_base_q}..{after_sha_q}", + log_prefix=self.log_prefix, + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc: + self.logger.warning(f"{self.log_prefix} Clean rebase detection: failed to compute diff for new range") + return False + + # Step 6: Compare hashes + old_hash = hashlib.sha256(old_diff.encode()).hexdigest() + new_hash = hashlib.sha256(new_diff.encode()).hexdigest() + + is_clean = old_hash == new_hash + self.logger.info( + f"{self.log_prefix} Clean rebase detection: " + f"old_hash={old_hash[:12]}, new_hash={new_hash[:12]}, is_clean_rebase={is_clean}" + ) + return is_clean + async def process_pull_request_webhook_data(self, pull_request: PullRequest) -> None: hook_action: str = self.hook_data["action"] if self.ctx: @@ -122,10 +222,43 @@ async def process_pull_request_webhook_data(self, pull_request: PullRequest) -> return if hook_action == "synchronize": - sync_tasks: list[Coroutine[Any, Any, Any]] = [] + clean_rebase = await self._is_clean_rebase(pull_request=pull_request) + + if clean_rebase: + before_sha: str = self.hook_data["before"] + labels = await asyncio.to_thread(lambda: list(pull_request.labels)) + review_labels = [ + label.name + for label in labels + if label.name.startswith(APPROVED_BY_LABEL_PREFIX) + or label.name.startswith(LGTM_BY_LABEL_PREFIX) + or label.name.startswith(COMMENTED_BY_LABEL_PREFIX) + or label.name.startswith(CHANGED_REQUESTED_BY_LABEL_PREFIX) + or label.name == VERIFIED_LABEL_STR + ] + + if review_labels: + labels_str = ", ".join(f"`{lbl}`" for lbl in review_labels) + comment_body = ( + f"**Clean rebase detected** \u2014 no code changes compared to " + f"previous head (`{before_sha[:7]}`).\n" + f"The following labels were preserved: {labels_str}." + ) + else: + comment_body = ( + f"**Clean rebase detected** \u2014 no code changes compared to " + f"previous head (`{before_sha[:7]}`)." + ) - sync_tasks.append(self.process_opened_or_synchronize_pull_request(pull_request=pull_request)) - sync_tasks.append(self.remove_labels_when_pull_request_sync(pull_request=pull_request)) + sync_tasks = [ + asyncio.to_thread(pull_request.create_issue_comment, body=comment_body), + self.process_opened_or_synchronize_pull_request(pull_request=pull_request, is_clean_rebase=True), + ] + else: + sync_tasks = [ + self.process_opened_or_synchronize_pull_request(pull_request=pull_request, is_clean_rebase=False), + self.remove_labels_when_pull_request_sync(pull_request=pull_request), + ] results = await asyncio.gather(*sync_tasks, return_exceptions=True) @@ -835,7 +968,9 @@ def _find_matching_issue() -> Any | None: ) await asyncio.to_thread(matching_issue.edit, state="closed") - async def process_opened_or_synchronize_pull_request(self, pull_request: PullRequest) -> None: + async def process_opened_or_synchronize_pull_request( + self, pull_request: PullRequest, is_clean_rebase: bool = False + ) -> None: if self.ctx: self.ctx.start_step("pr_workflow_setup") @@ -865,7 +1000,8 @@ async def process_opened_or_synchronize_pull_request(self, pull_request: PullReq if self.github_webhook.build_and_push_container: setup_tasks.append(self.check_run_handler.set_check_queued(name=BUILD_CONTAINER_STR)) - setup_tasks.append(self._process_verified_for_update_or_new_pull_request(pull_request=pull_request)) + if not is_clean_rebase: + setup_tasks.append(self._process_verified_for_update_or_new_pull_request(pull_request=pull_request)) setup_tasks.append(self.labels_handler.add_size_label(pull_request=pull_request)) setup_tasks.append(self.add_pull_request_owner_as_assingee(pull_request=pull_request)) diff --git a/webhook_server/tests/test_clean_rebase_detection.py b/webhook_server/tests/test_clean_rebase_detection.py new file mode 100644 index 00000000..279c505f --- /dev/null +++ b/webhook_server/tests/test_clean_rebase_detection.py @@ -0,0 +1,656 @@ +"""Tests for clean rebase detection in PullRequestHandler. + +Tests the _is_clean_rebase method and the modified synchronize handler +that preserves review labels on clean rebases. +""" + +from __future__ import annotations + +import hashlib +import shlex +from typing import Any +from unittest.mock import AsyncMock, MagicMock, Mock, patch + +import pytest + +from webhook_server.libs.github_api import GithubWebhook +from webhook_server.libs.handlers.owners_files_handler import OwnersFileHandler +from webhook_server.libs.handlers.pull_request_handler import PullRequestHandler +from webhook_server.tests.conftest import TEST_GITHUB_TOKEN +from webhook_server.utils.constants import ( + APPROVED_BY_LABEL_PREFIX, + CHANGED_REQUESTED_BY_LABEL_PREFIX, + COMMENTED_BY_LABEL_PREFIX, + LGTM_BY_LABEL_PREFIX, + VERIFIED_LABEL_STR, +) + + +@pytest.fixture +def mock_github_webhook() -> Mock: + """Create a mock GithubWebhook instance.""" + mock_webhook = Mock(spec=GithubWebhook) + mock_webhook.hook_data = { + "action": "synchronize", + "before": "aaa1111111111111111111111111111111111111", + "after": "bbb2222222222222222222222222222222222222", + "pull_request": {"number": 42, "merged": False, "title": "Test PR"}, + "sender": {"login": "test-user"}, + "label": {"name": "bug"}, + } + mock_webhook.logger = MagicMock() + mock_webhook.log_prefix = "[TEST]" + mock_webhook.repository_full_name = "test-org/test-repo" + mock_webhook.repository = Mock() + mock_webhook.clone_repo_dir = "/tmp/test-clone-dir" + mock_webhook.mask_sensitive = True + mock_webhook.issue_url_for_welcome_msg = "welcome-message-url" + mock_webhook.parent_committer = "test-user" + mock_webhook.auto_verified_and_merged_users = ["test-user"] + mock_webhook.create_issue_for_new_pr = True + mock_webhook.verified_job = True + mock_webhook.build_and_push_container = True + mock_webhook.container_repository_and_tag = Mock(return_value="test-repo:pr-42") + mock_webhook.can_be_merged_required_labels = [] + mock_webhook.set_auto_merge_prs = [] + mock_webhook.auto_merge_enabled = True + mock_webhook.container_repository = "docker.io/org/repo" + mock_webhook.conventional_title = False + mock_webhook.minimum_lgtm = 1 + mock_webhook.container_repository_username = "test-user" + mock_webhook.container_repository_password = "test-password" # pragma: allowlist secret + mock_webhook.github_api = Mock() + mock_webhook.tox = True + mock_webhook.pre_commit = True + mock_webhook.python_module_install = False + mock_webhook.pypi = False + mock_webhook.token = TEST_GITHUB_TOKEN + mock_webhook.auto_verify_cherry_picked_prs = True + mock_webhook.cherry_pick_assign_to_pr_author = True + mock_webhook.last_commit = Mock() + mock_webhook.ctx = None + mock_webhook.enabled_labels = None + mock_webhook.custom_check_runs = [] + mock_webhook.ai_features = None + mock_webhook.required_conversation_resolution = False + mock_webhook.config = Mock() + mock_webhook.config.get_value = Mock(return_value=None) + mock_webhook._repo_cloned = True + return mock_webhook + + +@pytest.fixture +def mock_owners_file_handler() -> Mock: + """Create a mock OwnersFileHandler instance.""" + mock_handler = Mock(spec=OwnersFileHandler) + mock_handler.all_pull_request_approvers = ["approver1", "approver2"] + mock_handler.all_pull_request_reviewers = ["reviewer1", "reviewer2"] + mock_handler.root_approvers = ["root-approver"] + mock_handler.root_reviewers = ["root-reviewer"] + mock_handler.assign_reviewers = AsyncMock() + return mock_handler + + +@pytest.fixture +def handler(mock_github_webhook: Mock, mock_owners_file_handler: Mock) -> PullRequestHandler: + """Create a PullRequestHandler instance with mocked dependencies.""" + handler = PullRequestHandler(mock_github_webhook, mock_owners_file_handler) + + handler.labels_handler = Mock() + handler.labels_handler._add_label = AsyncMock() + handler.labels_handler._remove_label = AsyncMock() + handler.labels_handler.add_size_label = AsyncMock() + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[]) + handler.labels_handler.wip_or_hold_labels_exists = Mock(return_value=False) + handler.labels_handler.is_label_enabled = Mock(return_value=True) + + handler.check_run_handler = Mock() + handler.check_run_handler.set_check_queued = AsyncMock() + handler.check_run_handler.set_check_in_progress = AsyncMock() + handler.check_run_handler.set_check_success = AsyncMock() + handler.check_run_handler.set_check_failure = AsyncMock() + + handler.runner_handler = Mock() + handler.runner_handler.run_container_build = AsyncMock() + handler.runner_handler.run_tox = AsyncMock() + handler.runner_handler.run_pre_commit = AsyncMock() + handler.runner_handler.run_conventional_title_check = AsyncMock() + handler.runner_handler.run_build_container = AsyncMock() + handler.runner_handler.run_install_python_module = AsyncMock() + handler.runner_handler.run_podman_command = AsyncMock(return_value=(0, "", "")) + handler.runner_handler.cherry_pick = AsyncMock() + + return handler + + +@pytest.fixture +def mock_pull_request() -> Mock: + """Create a mock PullRequest instance.""" + mock_pr = MagicMock() + mock_pr.number = 42 + mock_pr.title = "Test PR" + mock_pr.body = "Test PR body" + mock_pr.html_url = "https://github.com/test/repo/pull/42" + mock_pr.labels = [] + mock_pr.create_issue_comment = Mock() + mock_pr.edit = Mock() + mock_pr.is_merged = Mock(return_value=False) + mock_pr.base = Mock() + mock_pr.base.ref = "main" + mock_pr.head = Mock() + mock_pr.head.ref = "feature-branch" + mock_pr.head.user = Mock() + mock_pr.head.user.login = "test-user" + mock_pr.user = Mock() + mock_pr.user.login = "owner1" + mock_pr.mergeable = True + mock_pr.mergeable_state = "clean" + mock_pr.enable_automerge = Mock() + mock_pr.add_to_assignees = Mock() + mock_pr.get_issue_comments = Mock(return_value=[]) + mock_pr.raw_data = {} + return mock_pr + + +class TestIsCleanRebase: + """Test suite for _is_clean_rebase method.""" + + @pytest.mark.asyncio + async def test_clean_rebase_detected_when_diffs_match( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that _is_clean_rebase returns True when old and new diffs produce the same hash.""" + before_sha = handler.hook_data["before"] + after_sha = handler.hook_data["after"] + diff_content = "diff --git a/file.py b/file.py\n+hello\n" + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + if f"fetch origin {before_sha}" in command: + return (True, "", "") + if "merge-base" in command and before_sha in command: + return (True, "old_merge_base_sha\n", "") + if "merge-base" in command and after_sha in command: + return (True, "new_merge_base_sha\n", "") + if "diff" in command and "old_merge_base_sha" in command: + return (True, diff_content, "") + if "diff" in command and "new_merge_base_sha" in command: + return (True, diff_content, "") + return (False, "", "unexpected command") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + result = await handler._is_clean_rebase(mock_pull_request) + + assert result is True + + @pytest.mark.asyncio + async def test_not_clean_rebase_when_diffs_differ( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that _is_clean_rebase returns False when old and new diffs produce different hashes.""" + before_sha = handler.hook_data["before"] + after_sha = handler.hook_data["after"] + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + if f"fetch origin {before_sha}" in command: + return (True, "", "") + if "merge-base" in command and before_sha in command: + return (True, "old_merge_base_sha\n", "") + if "merge-base" in command and after_sha in command: + return (True, "new_merge_base_sha\n", "") + if "diff" in command and "old_merge_base_sha" in command: + return (True, "old diff content\n", "") + if "diff" in command and "new_merge_base_sha" in command: + return (True, "new diff content with changes\n", "") + return (False, "", "unexpected command") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + result = await handler._is_clean_rebase(mock_pull_request) + + assert result is False + + @pytest.mark.asyncio + async def test_returns_false_when_fetch_fails(self, handler: PullRequestHandler, mock_pull_request: Mock) -> None: + """Test that _is_clean_rebase returns False when git fetch of old SHA fails.""" + before_sha = handler.hook_data["before"] + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + if f"fetch origin {before_sha}" in command: + return (False, "", "error: could not fetch") + return (True, "", "") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + result = await handler._is_clean_rebase(mock_pull_request) + + assert result is False + handler.logger.warning.assert_called() + + @pytest.mark.asyncio + async def test_returns_false_when_old_merge_base_fails( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that _is_clean_rebase returns False when merge-base for old SHA fails.""" + before_sha = handler.hook_data["before"] + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + if f"fetch origin {before_sha}" in command: + return (True, "", "") + if "merge-base" in command and before_sha in command: + return (False, "", "fatal: not a valid commit") + return (True, "", "") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + result = await handler._is_clean_rebase(mock_pull_request) + + assert result is False + handler.logger.warning.assert_called() + + @pytest.mark.asyncio + async def test_returns_false_when_new_merge_base_fails( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that _is_clean_rebase returns False when merge-base for new SHA fails.""" + before_sha = handler.hook_data["before"] + after_sha = handler.hook_data["after"] + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + if f"fetch origin {before_sha}" in command: + return (True, "", "") + if "merge-base" in command and before_sha in command: + return (True, "old_merge_base_sha\n", "") + if "merge-base" in command and after_sha in command: + return (False, "", "fatal: not a valid commit") + return (True, "", "") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + result = await handler._is_clean_rebase(mock_pull_request) + + assert result is False + handler.logger.warning.assert_called() + + @pytest.mark.asyncio + async def test_returns_false_when_old_diff_fails( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that _is_clean_rebase returns False when git diff for old range fails.""" + before_sha = handler.hook_data["before"] + after_sha = handler.hook_data["after"] + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + if f"fetch origin {before_sha}" in command: + return (True, "", "") + if "merge-base" in command and before_sha in command: + return (True, "old_merge_base_sha\n", "") + if "merge-base" in command and after_sha in command: + return (True, "new_merge_base_sha\n", "") + if "diff" in command and "old_merge_base_sha" in command: + return (False, "", "fatal: bad revision") + return (True, "", "") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + result = await handler._is_clean_rebase(mock_pull_request) + + assert result is False + handler.logger.warning.assert_called() + + @pytest.mark.asyncio + async def test_returns_false_when_new_diff_fails( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that _is_clean_rebase returns False when git diff for new range fails.""" + before_sha = handler.hook_data["before"] + after_sha = handler.hook_data["after"] + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + if f"fetch origin {before_sha}" in command: + return (True, "", "") + if "merge-base" in command and before_sha in command: + return (True, "old_merge_base_sha\n", "") + if "merge-base" in command and after_sha in command: + return (True, "new_merge_base_sha\n", "") + if "diff" in command and "old_merge_base_sha" in command: + return (True, "some diff\n", "") + if "diff" in command and "new_merge_base_sha" in command: + return (False, "", "fatal: bad revision") + return (True, "", "") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + result = await handler._is_clean_rebase(mock_pull_request) + + assert result is False + handler.logger.warning.assert_called() + + @pytest.mark.asyncio + async def test_uses_correct_clone_dir(self, handler: PullRequestHandler, mock_pull_request: Mock) -> None: + """Test that _is_clean_rebase uses the correct clone_repo_dir in git commands.""" + clone_dir = handler.github_webhook.clone_repo_dir + commands_received: list[str] = [] + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + commands_received.append(command) + if "fetch" in command: + return (True, "", "") + if "merge-base" in command: + return (True, "merge_base_sha\n", "") + if "diff" in command: + return (True, "diff content\n", "") + return (True, "", "") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + await handler._is_clean_rebase(mock_pull_request) + + # All git commands should use -C with the clone dir (shlex-quoted) + clone_dir_q = shlex.quote(clone_dir) + for cmd in commands_received: + assert f"git -C {clone_dir_q}" in cmd + + @pytest.mark.asyncio + async def test_uses_correct_base_ref(self, handler: PullRequestHandler, mock_pull_request: Mock) -> None: + """Test that _is_clean_rebase uses pull_request.base.ref for merge-base commands.""" + mock_pull_request.base.ref = "develop" + commands_received: list[str] = [] + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + commands_received.append(command) + if "fetch" in command: + return (True, "", "") + if "merge-base" in command: + return (True, "merge_base_sha\n", "") + if "diff" in command: + return (True, "diff content\n", "") + return (True, "", "") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + await handler._is_clean_rebase(mock_pull_request) + + # merge-base commands should reference origin/{base_ref} (shlex-quoted) + merge_base_cmds = [c for c in commands_received if "merge-base" in c] + for cmd in merge_base_cmds: + assert shlex.quote("origin/develop") in cmd + + @pytest.mark.asyncio + async def test_hashes_diff_output_with_sha256(self, handler: PullRequestHandler, mock_pull_request: Mock) -> None: + """Test that _is_clean_rebase hashes diff output using sha256.""" + before_sha = handler.hook_data["before"] + after_sha = handler.hook_data["after"] + diff_a = "diff --git a/file.py\n+line1\n" + diff_b = "diff --git a/file.py\n+line1\n+line2\n" + + # Ensure different diffs produce different hashes + hash_a = hashlib.sha256(diff_a.encode()).hexdigest() + hash_b = hashlib.sha256(diff_b.encode()).hexdigest() + assert hash_a != hash_b + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + if f"fetch origin {before_sha}" in command: + return (True, "", "") + if "merge-base" in command and before_sha in command: + return (True, "old_merge_base\n", "") + if "merge-base" in command and after_sha in command: + return (True, "new_merge_base\n", "") + if "diff" in command and "old_merge_base" in command: + return (True, diff_a, "") + if "diff" in command and "new_merge_base" in command: + return (True, diff_b, "") + return (False, "", "unexpected") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + result = await handler._is_clean_rebase(mock_pull_request) + + assert result is False + + @pytest.mark.asyncio + async def test_returns_false_when_repo_not_cloned( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that _is_clean_rebase returns False early when repository is not cloned.""" + handler.github_webhook._repo_cloned = False + + with patch( + "webhook_server.libs.handlers.pull_request_handler.run_command", new_callable=AsyncMock + ) as mock_run_cmd: + result = await handler._is_clean_rebase(mock_pull_request) + + assert result is False + # run_command should never be called when repo is not cloned + mock_run_cmd.assert_not_called() + handler.logger.debug.assert_called() + + @pytest.mark.asyncio + async def test_git_commands_use_shlex_quote(self, handler: PullRequestHandler, mock_pull_request: Mock) -> None: + """Test that all git command arguments are properly quoted with shlex.quote().""" + before_sha = handler.hook_data["before"] + after_sha = handler.hook_data["after"] + clone_dir = handler.github_webhook.clone_repo_dir + commands_received: list[str] = [] + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + commands_received.append(command) + if "fetch" in command: + return (True, "", "") + if "merge-base" in command: + return (True, "merge_base_sha\n", "") + if "diff" in command: + return (True, "diff content\n", "") + return (True, "", "") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + await handler._is_clean_rebase(mock_pull_request) + + # Verify shlex.quote is used for all interpolated values + clone_dir_q = shlex.quote(clone_dir) + before_sha_q = shlex.quote(before_sha) + after_sha_q = shlex.quote(after_sha) + base_ref_q = shlex.quote("origin/main") + + # Check fetch command + fetch_cmds = [c for c in commands_received if "fetch" in c] + assert len(fetch_cmds) == 1 + assert f"git -C {clone_dir_q} fetch origin {before_sha_q}" == fetch_cmds[0] + + # Check merge-base commands use quoted base_ref + merge_base_cmds = [c for c in commands_received if "merge-base" in c] + assert len(merge_base_cmds) == 2 + assert f"git -C {clone_dir_q} merge-base {base_ref_q} {before_sha_q}" == merge_base_cmds[0] + assert f"git -C {clone_dir_q} merge-base {base_ref_q} {after_sha_q}" == merge_base_cmds[1] + + +class TestSynchronizeWithCleanRebase: + """Test suite for synchronize handler with clean rebase detection.""" + + @pytest.mark.asyncio + async def test_synchronize_clean_rebase_skips_label_removal( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that synchronize with clean rebase does NOT call remove_labels_when_pull_request_sync.""" + with ( + patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), + patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock) as mock_process, + patch.object(handler, "remove_labels_when_pull_request_sync", new_callable=AsyncMock) as mock_remove_labels, + ): + await handler.process_pull_request_webhook_data(mock_pull_request) + + mock_process.assert_called_once_with(pull_request=mock_pull_request, is_clean_rebase=True) + mock_remove_labels.assert_not_called() + + @pytest.mark.asyncio + async def test_synchronize_not_clean_rebase_removes_labels( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that synchronize without clean rebase calls both process and remove_labels.""" + with ( + patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=False), + patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock) as mock_process, + patch.object(handler, "remove_labels_when_pull_request_sync", new_callable=AsyncMock) as mock_remove_labels, + ): + await handler.process_pull_request_webhook_data(mock_pull_request) + + mock_process.assert_called_once_with(pull_request=mock_pull_request, is_clean_rebase=False) + mock_remove_labels.assert_called_once_with(pull_request=mock_pull_request) + + @pytest.mark.asyncio + async def test_synchronize_clean_rebase_posts_comment_with_preserved_labels( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that clean rebase posts a comment listing preserved review labels.""" + approved_label = Mock() + approved_label.name = f"{APPROVED_BY_LABEL_PREFIX}reviewer1" + lgtm_label = Mock() + lgtm_label.name = f"{LGTM_BY_LABEL_PREFIX}reviewer2" + non_review_label = Mock() + non_review_label.name = "bug" + mock_pull_request.labels = [approved_label, lgtm_label, non_review_label] + + before_sha = handler.hook_data["before"] + + with ( + patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), + patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock), + ): + await handler.process_pull_request_webhook_data(mock_pull_request) + + # create_issue_comment is called via asyncio.to_thread which executes it + mock_pull_request.create_issue_comment.assert_called_once() + comment_body = mock_pull_request.create_issue_comment.call_args.kwargs["body"] + assert "Clean rebase detected" in comment_body + assert before_sha[:7] in comment_body + assert f"`{approved_label.name}`" in comment_body + assert f"`{lgtm_label.name}`" in comment_body + assert "bug" not in comment_body + + @pytest.mark.asyncio + async def test_synchronize_clean_rebase_no_review_labels_simple_comment( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that clean rebase with no review labels posts a simple comment.""" + non_review_label = Mock() + non_review_label.name = "bug" + mock_pull_request.labels = [non_review_label] + + before_sha = handler.hook_data["before"] + + with ( + patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), + patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock), + ): + await handler.process_pull_request_webhook_data(mock_pull_request) + + mock_pull_request.create_issue_comment.assert_called_once() + comment_body = mock_pull_request.create_issue_comment.call_args.kwargs["body"] + assert "Clean rebase detected" in comment_body + assert before_sha[:7] in comment_body + assert "preserved" not in comment_body + + @pytest.mark.asyncio + async def test_synchronize_clean_rebase_preserves_verified_label( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that clean rebase recognizes verified label as a review label to preserve.""" + verified_label = Mock() + verified_label.name = VERIFIED_LABEL_STR + mock_pull_request.labels = [verified_label] + + with ( + patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), + patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock), + ): + await handler.process_pull_request_webhook_data(mock_pull_request) + + mock_pull_request.create_issue_comment.assert_called_once() + comment_body = mock_pull_request.create_issue_comment.call_args.kwargs["body"] + assert f"`{VERIFIED_LABEL_STR}`" in comment_body + + @pytest.mark.asyncio + async def test_synchronize_clean_rebase_preserves_changes_requested_label( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that clean rebase recognizes changes-requested label as a review label.""" + cr_label = Mock() + cr_label.name = f"{CHANGED_REQUESTED_BY_LABEL_PREFIX}reviewer1" + mock_pull_request.labels = [cr_label] + + with ( + patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), + patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock), + ): + await handler.process_pull_request_webhook_data(mock_pull_request) + + mock_pull_request.create_issue_comment.assert_called_once() + comment_body = mock_pull_request.create_issue_comment.call_args.kwargs["body"] + assert f"`{cr_label.name}`" in comment_body + + @pytest.mark.asyncio + async def test_synchronize_clean_rebase_preserves_commented_by_label( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that clean rebase recognizes commented-by label as a review label.""" + commented_label = Mock() + commented_label.name = f"{COMMENTED_BY_LABEL_PREFIX}reviewer1" + mock_pull_request.labels = [commented_label] + + with ( + patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), + patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock), + ): + await handler.process_pull_request_webhook_data(mock_pull_request) + + mock_pull_request.create_issue_comment.assert_called_once() + comment_body = mock_pull_request.create_issue_comment.call_args.kwargs["body"] + assert f"`{commented_label.name}`" in comment_body + + @pytest.mark.asyncio + async def test_synchronize_clean_rebase_skips_verified_label_processing( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that clean rebase path skips _process_verified_for_update_or_new_pull_request. + + This is the MOST IMPORTANT fix: on clean rebase, verified label should NOT be removed + for non-auto-verified users. The is_clean_rebase flag passed to + process_opened_or_synchronize_pull_request should cause it to skip the verified + label processing. + """ + # Set parent_committer to a NON-auto-verified user + handler.github_webhook.parent_committer = "external-contributor" + handler.github_webhook.auto_verified_and_merged_users = ["auto-user"] + + verified_label = Mock() + verified_label.name = VERIFIED_LABEL_STR + mock_pull_request.labels = [verified_label] + + with patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True): + await handler.process_pull_request_webhook_data(mock_pull_request) + + # Verified label should NOT have been removed (no _remove_label call for verified) + remove_label_calls = handler.labels_handler._remove_label.call_args_list + verified_removed = any(call.kwargs.get("label") == VERIFIED_LABEL_STR for call in remove_label_calls) + assert not verified_removed, ( + "Verified label was removed during clean rebase - " + "_process_verified_for_update_or_new_pull_request should be skipped" + ) + + @pytest.mark.asyncio + async def test_synchronize_clean_rebase_handles_task_failure_gracefully( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that clean rebase path handles task failures via gather's return_exceptions. + + When the clean rebase path runs comment posting and process in parallel via gather, + an exception in one task should be logged but not crash the other. + """ + mock_pull_request.labels = [] + + with ( + patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), + patch.object( + handler, + "process_opened_or_synchronize_pull_request", + new_callable=AsyncMock, + side_effect=RuntimeError("test error"), + ), + ): + # Should not raise even though process_opened_or_synchronize_pull_request fails + await handler.process_pull_request_webhook_data(mock_pull_request) + + # Error should be logged + handler.logger.error.assert_called() + error_msg = str(handler.logger.error.call_args) + assert "test error" in error_msg diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index ed9ec186..0168dd3b 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -247,12 +247,17 @@ async def test_process_pull_request_webhook_data_synchronize_action( ) -> None: """Test processing pull request webhook data when action is synchronize.""" pull_request_handler.hook_data["action"] = "synchronize" + pull_request_handler.hook_data["before"] = "aaa1111111111111111111111111111111111111" + pull_request_handler.hook_data["after"] = "bbb2222222222222222222222222222222222222" - with patch.object(pull_request_handler, "process_opened_or_synchronize_pull_request") as mock_process: - with patch.object(pull_request_handler, "remove_labels_when_pull_request_sync") as mock_remove_labels: - await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) - mock_process.assert_called_once_with(pull_request=mock_pull_request) - mock_remove_labels.assert_called_once_with(pull_request=mock_pull_request) + with ( + patch.object(pull_request_handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=False), + patch.object(pull_request_handler, "process_opened_or_synchronize_pull_request") as mock_process, + patch.object(pull_request_handler, "remove_labels_when_pull_request_sync") as mock_remove_labels, + ): + await pull_request_handler.process_pull_request_webhook_data(mock_pull_request) + mock_process.assert_called_once_with(pull_request=mock_pull_request, is_clean_rebase=False) + mock_remove_labels.assert_called_once_with(pull_request=mock_pull_request) @pytest.mark.asyncio async def test_process_pull_request_webhook_data_closed_action_not_merged( @@ -2371,8 +2376,11 @@ async def test_process_synchronize_async_exception( ) -> None: """Test exception handling in async tasks for synchronize event.""" mock_github_webhook.hook_data["action"] = "synchronize" + mock_github_webhook.hook_data["before"] = "aaa1111111111111111111111111111111111111" + mock_github_webhook.hook_data["after"] = "bbb2222222222222222222222222222222222222" with ( + patch.object(pull_request_handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=False), patch.object( pull_request_handler, "process_opened_or_synchronize_pull_request", @@ -3106,8 +3114,11 @@ async def test_process_synchronize_action_calls_test_oracle_with_pr_synchronized Verifies trigger='pr-synchronized' when a PR is synchronized. """ pull_request_handler.hook_data["action"] = "synchronize" + pull_request_handler.hook_data["before"] = "aaa1111111111111111111111111111111111111" + pull_request_handler.hook_data["after"] = "bbb2222222222222222222222222222222222222" with ( + patch.object(pull_request_handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=False), patch.object(pull_request_handler, "process_opened_or_synchronize_pull_request"), patch.object(pull_request_handler, "remove_labels_when_pull_request_sync"), patch( From 444d046da07ab235141f733859bbcde1b251f337 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 31 Mar 2026 12:49:34 +0300 Subject: [PATCH 2/8] fix: use --binary flag in clean rebase diff to prevent false positives Binary files produce generic "Binary files differ" text in git diff, which would hash identically for different binary changes, causing false positive clean rebase detection. The --binary flag outputs full binary content (base85-encoded) ensuring different changes produce different hashes. --- .../libs/handlers/pull_request_handler.py | 4 +-- .../tests/test_clean_rebase_detection.py | 32 +++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 1ae394d7..c8906ea3 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -139,7 +139,7 @@ async def _is_clean_rebase(self, pull_request: PullRequest) -> bool: # Step 4: Compute diff hash for old range rc, old_diff, _ = await run_command( - command=f"git -C {clone_dir_q} diff {old_merge_base_q}..{before_sha_q}", + command=f"git -C {clone_dir_q} diff --binary {old_merge_base_q}..{before_sha_q}", log_prefix=self.log_prefix, mask_sensitive=self.github_webhook.mask_sensitive, ) @@ -149,7 +149,7 @@ async def _is_clean_rebase(self, pull_request: PullRequest) -> bool: # Step 5: Compute diff hash for new range rc, new_diff, _ = await run_command( - command=f"git -C {clone_dir_q} diff {new_merge_base_q}..{after_sha_q}", + command=f"git -C {clone_dir_q} diff --binary {new_merge_base_q}..{after_sha_q}", log_prefix=self.log_prefix, mask_sensitive=self.github_webhook.mask_sensitive, ) diff --git a/webhook_server/tests/test_clean_rebase_detection.py b/webhook_server/tests/test_clean_rebase_detection.py index 279c505f..38157ffa 100644 --- a/webhook_server/tests/test_clean_rebase_detection.py +++ b/webhook_server/tests/test_clean_rebase_detection.py @@ -453,6 +453,38 @@ async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tupl assert f"git -C {clone_dir_q} merge-base {base_ref_q} {before_sha_q}" == merge_base_cmds[0] assert f"git -C {clone_dir_q} merge-base {base_ref_q} {after_sha_q}" == merge_base_cmds[1] + @pytest.mark.asyncio + async def test_diff_commands_use_binary_flag(self, handler: PullRequestHandler, mock_pull_request: Mock) -> None: + """Test that git diff commands use --binary flag to prevent false positives with binary files.""" + before_sha = handler.hook_data["before"] + after_sha = handler.hook_data["after"] + clone_dir = handler.github_webhook.clone_repo_dir + commands_received: list[str] = [] + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + commands_received.append(command) + if "fetch" in command: + return (True, "", "") + if "merge-base" in command: + return (True, "merge_base_sha\n", "") + if "diff" in command: + return (True, "diff content\n", "") + return (True, "", "") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + await handler._is_clean_rebase(mock_pull_request) + + clone_dir_q = shlex.quote(clone_dir) + merge_base_q = shlex.quote("merge_base_sha") + before_sha_q = shlex.quote(before_sha) + after_sha_q = shlex.quote(after_sha) + + # Check diff commands include --binary flag + diff_cmds = [c for c in commands_received if "diff" in c] + assert len(diff_cmds) == 2 + assert f"git -C {clone_dir_q} diff --binary {merge_base_q}..{before_sha_q}" == diff_cmds[0] + assert f"git -C {clone_dir_q} diff --binary {merge_base_q}..{after_sha_q}" == diff_cmds[1] + class TestSynchronizeWithCleanRebase: """Test suite for synchronize handler with clean rebase detection.""" From 7650231e07b4bfdc09dd4490f55cc52581e716be Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 31 Mar 2026 13:12:04 +0300 Subject: [PATCH 3/8] fix: make clean rebase detection fail-closed and comment best-effort - Wrap _is_clean_rebase() in try/except to return False on any unexpected exception (re-raises CancelledError) - Add timeout=60 to all run_command calls to prevent hangs - Extract comment posting into _post_clean_rebase_comment() helper with its own try/except so CI/setup always runs even if comment fails - Fix test to verify hashlib.sha256 is actually called --- .../libs/handlers/pull_request_handler.py | 225 ++++++++++-------- .../tests/test_clean_rebase_detection.py | 171 ++++++++++++- 2 files changed, 288 insertions(+), 108 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index c8906ea3..6e1a48cd 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -83,90 +83,137 @@ async def _is_clean_rebase(self, pull_request: PullRequest) -> bool: Returns: True if the push is a clean rebase, False otherwise. - On any git command failure, returns False (conservative). + On any failure (git command, exception, timeout), returns False (conservative). """ - if not self.github_webhook._repo_cloned: - self.logger.debug(f"{self.log_prefix} Clean rebase detection: skipped, repository not cloned") - return False + try: + if not self.github_webhook._repo_cloned: + self.logger.debug(f"{self.log_prefix} Clean rebase detection: skipped, repository not cloned") + return False + + before_sha: str = self.hook_data["before"] + after_sha: str = self.hook_data["after"] + base_ref: str = await asyncio.to_thread(lambda: pull_request.base.ref) + clone_dir: str = self.github_webhook.clone_repo_dir + + clone_dir_q = shlex.quote(clone_dir) + before_sha_q = shlex.quote(before_sha) + after_sha_q = shlex.quote(after_sha) + base_ref_q = shlex.quote(f"origin/{base_ref}") + + # Step 1: Fetch the old head SHA (may not be in the clone) + rc, _, _ = await run_command( + command=f"git -C {clone_dir_q} fetch origin {before_sha_q}", + log_prefix=self.log_prefix, + mask_sensitive=self.github_webhook.mask_sensitive, + timeout=60, + ) + if not rc: + self.logger.warning( + f"{self.log_prefix} Clean rebase detection: failed to fetch old head {before_sha[:7]}" + ) + return False + + # Step 2: Get merge-base for old head + rc, old_merge_base_out, _ = await run_command( + command=f"git -C {clone_dir_q} merge-base {base_ref_q} {before_sha_q}", + log_prefix=self.log_prefix, + mask_sensitive=self.github_webhook.mask_sensitive, + timeout=60, + ) + if not rc: + self.logger.warning( + f"{self.log_prefix} Clean rebase detection: failed to find merge-base for old head {before_sha[:7]}" + ) + return False + old_merge_base = old_merge_base_out.strip() + old_merge_base_q = shlex.quote(old_merge_base) + + # Step 3: Get merge-base for new head + rc, new_merge_base_out, _ = await run_command( + command=f"git -C {clone_dir_q} merge-base {base_ref_q} {after_sha_q}", + log_prefix=self.log_prefix, + mask_sensitive=self.github_webhook.mask_sensitive, + timeout=60, + ) + if not rc: + self.logger.warning( + f"{self.log_prefix} Clean rebase detection: failed to find merge-base for new head {after_sha[:7]}" + ) + return False + new_merge_base = new_merge_base_out.strip() + new_merge_base_q = shlex.quote(new_merge_base) + + # Step 4: Compute diff hash for old range + rc, old_diff, _ = await run_command( + command=f"git -C {clone_dir_q} diff --binary {old_merge_base_q}..{before_sha_q}", + log_prefix=self.log_prefix, + mask_sensitive=self.github_webhook.mask_sensitive, + timeout=60, + ) + if not rc: + self.logger.warning(f"{self.log_prefix} Clean rebase detection: failed to compute diff for old range") + return False + + # Step 5: Compute diff hash for new range + rc, new_diff, _ = await run_command( + command=f"git -C {clone_dir_q} diff --binary {new_merge_base_q}..{after_sha_q}", + log_prefix=self.log_prefix, + mask_sensitive=self.github_webhook.mask_sensitive, + timeout=60, + ) + if not rc: + self.logger.warning(f"{self.log_prefix} Clean rebase detection: failed to compute diff for new range") + return False - before_sha: str = self.hook_data["before"] - after_sha: str = self.hook_data["after"] - base_ref: str = await asyncio.to_thread(lambda: pull_request.base.ref) - clone_dir: str = self.github_webhook.clone_repo_dir - - clone_dir_q = shlex.quote(clone_dir) - before_sha_q = shlex.quote(before_sha) - after_sha_q = shlex.quote(after_sha) - base_ref_q = shlex.quote(f"origin/{base_ref}") - - # Step 1: Fetch the old head SHA (may not be in the clone) - rc, _, _ = await run_command( - command=f"git -C {clone_dir_q} fetch origin {before_sha_q}", - log_prefix=self.log_prefix, - mask_sensitive=self.github_webhook.mask_sensitive, - ) - if not rc: - self.logger.warning(f"{self.log_prefix} Clean rebase detection: failed to fetch old head {before_sha[:7]}") - return False + # Step 6: Compare hashes + old_hash = hashlib.sha256(old_diff.encode()).hexdigest() + new_hash = hashlib.sha256(new_diff.encode()).hexdigest() - # Step 2: Get merge-base for old head - rc, old_merge_base_out, _ = await run_command( - command=f"git -C {clone_dir_q} merge-base {base_ref_q} {before_sha_q}", - log_prefix=self.log_prefix, - mask_sensitive=self.github_webhook.mask_sensitive, - ) - if not rc: - self.logger.warning( - f"{self.log_prefix} Clean rebase detection: failed to find merge-base for old head {before_sha[:7]}" + is_clean = old_hash == new_hash + self.logger.info( + f"{self.log_prefix} Clean rebase detection: " + f"old_hash={old_hash[:12]}, new_hash={new_hash[:12]}, is_clean_rebase={is_clean}" ) - return False - old_merge_base = old_merge_base_out.strip() - old_merge_base_q = shlex.quote(old_merge_base) - - # Step 3: Get merge-base for new head - rc, new_merge_base_out, _ = await run_command( - command=f"git -C {clone_dir_q} merge-base {base_ref_q} {after_sha_q}", - log_prefix=self.log_prefix, - mask_sensitive=self.github_webhook.mask_sensitive, - ) - if not rc: - self.logger.warning( - f"{self.log_prefix} Clean rebase detection: failed to find merge-base for new head {after_sha[:7]}" + return is_clean + except asyncio.CancelledError: + raise + except Exception: + self.logger.exception( + f"{self.log_prefix} Clean rebase detection failed unexpectedly; treating as non-clean" ) return False - new_merge_base = new_merge_base_out.strip() - new_merge_base_q = shlex.quote(new_merge_base) - - # Step 4: Compute diff hash for old range - rc, old_diff, _ = await run_command( - command=f"git -C {clone_dir_q} diff --binary {old_merge_base_q}..{before_sha_q}", - log_prefix=self.log_prefix, - mask_sensitive=self.github_webhook.mask_sensitive, - ) - if not rc: - self.logger.warning(f"{self.log_prefix} Clean rebase detection: failed to compute diff for old range") - return False - - # Step 5: Compute diff hash for new range - rc, new_diff, _ = await run_command( - command=f"git -C {clone_dir_q} diff --binary {new_merge_base_q}..{after_sha_q}", - log_prefix=self.log_prefix, - mask_sensitive=self.github_webhook.mask_sensitive, - ) - if not rc: - self.logger.warning(f"{self.log_prefix} Clean rebase detection: failed to compute diff for new range") - return False - # Step 6: Compare hashes - old_hash = hashlib.sha256(old_diff.encode()).hexdigest() - new_hash = hashlib.sha256(new_diff.encode()).hexdigest() + async def _post_clean_rebase_comment(self, pull_request: PullRequest, before_sha: str) -> None: + """Post a comment about clean rebase detection. Best-effort -- failures are logged but don't block CI.""" + try: + labels = await asyncio.to_thread(lambda: list(pull_request.labels)) + review_labels = [ + label.name + for label in labels + if label.name.startswith(APPROVED_BY_LABEL_PREFIX) + or label.name.startswith(LGTM_BY_LABEL_PREFIX) + or label.name.startswith(COMMENTED_BY_LABEL_PREFIX) + or label.name.startswith(CHANGED_REQUESTED_BY_LABEL_PREFIX) + or label.name == VERIFIED_LABEL_STR + ] + + if review_labels: + labels_str = ", ".join(f"`{lbl}`" for lbl in review_labels) + comment_body = ( + f"**Clean rebase detected** \u2014 no code changes compared to " + f"previous head (`{before_sha[:7]}`).\n" + f"The following labels were preserved: {labels_str}." + ) + else: + comment_body = ( + f"**Clean rebase detected** \u2014 no code changes compared to previous head (`{before_sha[:7]}`)." + ) - is_clean = old_hash == new_hash - self.logger.info( - f"{self.log_prefix} Clean rebase detection: " - f"old_hash={old_hash[:12]}, new_hash={new_hash[:12]}, is_clean_rebase={is_clean}" - ) - return is_clean + await asyncio.to_thread(pull_request.create_issue_comment, body=comment_body) + except asyncio.CancelledError: + raise + except Exception: + self.logger.exception(f"{self.log_prefix} Failed to post clean-rebase comment") async def process_pull_request_webhook_data(self, pull_request: PullRequest) -> None: hook_action: str = self.hook_data["action"] @@ -226,32 +273,8 @@ async def process_pull_request_webhook_data(self, pull_request: PullRequest) -> if clean_rebase: before_sha: str = self.hook_data["before"] - labels = await asyncio.to_thread(lambda: list(pull_request.labels)) - review_labels = [ - label.name - for label in labels - if label.name.startswith(APPROVED_BY_LABEL_PREFIX) - or label.name.startswith(LGTM_BY_LABEL_PREFIX) - or label.name.startswith(COMMENTED_BY_LABEL_PREFIX) - or label.name.startswith(CHANGED_REQUESTED_BY_LABEL_PREFIX) - or label.name == VERIFIED_LABEL_STR - ] - - if review_labels: - labels_str = ", ".join(f"`{lbl}`" for lbl in review_labels) - comment_body = ( - f"**Clean rebase detected** \u2014 no code changes compared to " - f"previous head (`{before_sha[:7]}`).\n" - f"The following labels were preserved: {labels_str}." - ) - else: - comment_body = ( - f"**Clean rebase detected** \u2014 no code changes compared to " - f"previous head (`{before_sha[:7]}`)." - ) - sync_tasks = [ - asyncio.to_thread(pull_request.create_issue_comment, body=comment_body), + self._post_clean_rebase_comment(pull_request=pull_request, before_sha=before_sha), self.process_opened_or_synchronize_pull_request(pull_request=pull_request, is_clean_rebase=True), ] else: diff --git a/webhook_server/tests/test_clean_rebase_detection.py b/webhook_server/tests/test_clean_rebase_detection.py index 38157ffa..30ceef0f 100644 --- a/webhook_server/tests/test_clean_rebase_detection.py +++ b/webhook_server/tests/test_clean_rebase_detection.py @@ -6,6 +6,7 @@ from __future__ import annotations +import asyncio import hashlib import shlex from typing import Any @@ -375,13 +376,8 @@ async def test_hashes_diff_output_with_sha256(self, handler: PullRequestHandler, diff_a = "diff --git a/file.py\n+line1\n" diff_b = "diff --git a/file.py\n+line1\n+line2\n" - # Ensure different diffs produce different hashes - hash_a = hashlib.sha256(diff_a.encode()).hexdigest() - hash_b = hashlib.sha256(diff_b.encode()).hexdigest() - assert hash_a != hash_b - async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: - if f"fetch origin {before_sha}" in command: + if "fetch origin" in command: return (True, "", "") if "merge-base" in command and before_sha in command: return (True, "old_merge_base\n", "") @@ -393,10 +389,17 @@ async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tupl return (True, diff_b, "") return (False, "", "unexpected") - with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + with ( + patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command), + patch( + "webhook_server.libs.handlers.pull_request_handler.hashlib.sha256", + wraps=hashlib.sha256, + ) as mock_sha256, + ): result = await handler._is_clean_rebase(mock_pull_request) assert result is False + assert mock_sha256.call_count == 2 @pytest.mark.asyncio async def test_returns_false_when_repo_not_cloned( @@ -453,6 +456,58 @@ async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tupl assert f"git -C {clone_dir_q} merge-base {base_ref_q} {before_sha_q}" == merge_base_cmds[0] assert f"git -C {clone_dir_q} merge-base {base_ref_q} {after_sha_q}" == merge_base_cmds[1] + @pytest.mark.asyncio + async def test_returns_false_on_unexpected_exception( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that _is_clean_rebase returns False and logs when an unexpected exception occurs.""" + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + raise RuntimeError("unexpected git failure") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + result = await handler._is_clean_rebase(mock_pull_request) + + assert result is False + handler.logger.exception.assert_called_once() + assert "treating as non-clean" in str(handler.logger.exception.call_args) + + @pytest.mark.asyncio + async def test_reraises_cancelled_error(self, handler: PullRequestHandler, mock_pull_request: Mock) -> None: + """Test that _is_clean_rebase re-raises asyncio.CancelledError instead of catching it.""" + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + raise asyncio.CancelledError() + + with ( + patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command), + pytest.raises(asyncio.CancelledError), + ): + await handler._is_clean_rebase(mock_pull_request) + + @pytest.mark.asyncio + async def test_run_command_calls_include_timeout( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that all run_command calls include timeout=60 to prevent hanging.""" + timeouts_received: list[int | None] = [] + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + timeouts_received.append(kwargs.get("timeout")) + if "fetch" in command: + return (True, "", "") + if "merge-base" in command: + return (True, "merge_base_sha\n", "") + if "diff" in command: + return (True, "diff content\n", "") + return (True, "", "") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + await handler._is_clean_rebase(mock_pull_request) + + assert len(timeouts_received) == 5 + assert all(t == 60 for t in timeouts_received) + @pytest.mark.asyncio async def test_diff_commands_use_binary_flag(self, handler: PullRequestHandler, mock_pull_request: Mock) -> None: """Test that git diff commands use --binary flag to prevent false positives with binary files.""" @@ -686,3 +741,105 @@ async def test_synchronize_clean_rebase_handles_task_failure_gracefully( handler.logger.error.assert_called() error_msg = str(handler.logger.error.call_args) assert "test error" in error_msg + + +class TestPostCleanRebaseComment: + """Test suite for _post_clean_rebase_comment method.""" + + @pytest.mark.asyncio + async def test_posts_comment_with_review_labels(self, handler: PullRequestHandler, mock_pull_request: Mock) -> None: + """Test that _post_clean_rebase_comment posts a comment listing preserved review labels.""" + approved_label = Mock() + approved_label.name = f"{APPROVED_BY_LABEL_PREFIX}reviewer1" + lgtm_label = Mock() + lgtm_label.name = f"{LGTM_BY_LABEL_PREFIX}reviewer2" + non_review_label = Mock() + non_review_label.name = "bug" + mock_pull_request.labels = [approved_label, lgtm_label, non_review_label] + + before_sha = "abc1234567890" # pragma: allowlist secret + await handler._post_clean_rebase_comment(pull_request=mock_pull_request, before_sha=before_sha) + + mock_pull_request.create_issue_comment.assert_called_once() + comment_body = mock_pull_request.create_issue_comment.call_args.kwargs["body"] + assert "Clean rebase detected" in comment_body + assert before_sha[:7] in comment_body + assert f"`{approved_label.name}`" in comment_body + assert f"`{lgtm_label.name}`" in comment_body + assert "bug" not in comment_body + + @pytest.mark.asyncio + async def test_posts_simple_comment_without_review_labels( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that _post_clean_rebase_comment posts a simple comment when no review labels exist.""" + non_review_label = Mock() + non_review_label.name = "bug" + mock_pull_request.labels = [non_review_label] + + before_sha = "abc1234567890" # pragma: allowlist secret + await handler._post_clean_rebase_comment(pull_request=mock_pull_request, before_sha=before_sha) + + mock_pull_request.create_issue_comment.assert_called_once() + comment_body = mock_pull_request.create_issue_comment.call_args.kwargs["body"] + assert "Clean rebase detected" in comment_body + assert "preserved" not in comment_body + + @pytest.mark.asyncio + async def test_logs_and_does_not_raise_on_label_fetch_failure( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that _post_clean_rebase_comment logs the error and does not raise when labels fetch fails.""" + # Make the labels property on the mock's type raise when accessed + type(mock_pull_request).labels = property(lambda self: (_ for _ in ()).throw(RuntimeError("API error"))) + + before_sha = "abc1234567890" # pragma: allowlist secret + # Should not raise + await handler._post_clean_rebase_comment(pull_request=mock_pull_request, before_sha=before_sha) + + handler.logger.exception.assert_called_once() + assert "Failed to post clean-rebase comment" in str(handler.logger.exception.call_args) + + @pytest.mark.asyncio + async def test_logs_and_does_not_raise_on_comment_post_failure( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that _post_clean_rebase_comment logs the error when comment posting fails.""" + mock_pull_request.labels = [] + mock_pull_request.create_issue_comment = Mock(side_effect=RuntimeError("API error")) + + before_sha = "abc1234567890" # pragma: allowlist secret + # Should not raise + await handler._post_clean_rebase_comment(pull_request=mock_pull_request, before_sha=before_sha) + + handler.logger.exception.assert_called_once() + assert "Failed to post clean-rebase comment" in str(handler.logger.exception.call_args) + + @pytest.mark.asyncio + async def test_reraises_cancelled_error(self, handler: PullRequestHandler, mock_pull_request: Mock) -> None: + """Test that _post_clean_rebase_comment re-raises asyncio.CancelledError.""" + type(mock_pull_request).labels = property(lambda self: (_ for _ in ()).throw(asyncio.CancelledError())) + + before_sha = "abc1234567890" # pragma: allowlist secret + with pytest.raises(asyncio.CancelledError): + await handler._post_clean_rebase_comment(pull_request=mock_pull_request, before_sha=before_sha) + + @pytest.mark.asyncio + async def test_synchronize_continues_when_comment_fails( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that synchronize handler's process_opened_or_synchronize runs even if comment fails. + + This verifies that _post_clean_rebase_comment failure does not block CI processing. + """ + mock_pull_request.labels = [] + mock_pull_request.create_issue_comment = Mock(side_effect=RuntimeError("API error")) + + with ( + patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), + patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock) as mock_process, + ): + await handler.process_pull_request_webhook_data(mock_pull_request) + + # process_opened_or_synchronize_pull_request should still have been called + mock_process.assert_called_once_with(pull_request=mock_pull_request, is_clean_rebase=True) From 455e316c31c02470c57b04bc44d7db029832a505 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 31 Mar 2026 14:07:12 +0300 Subject: [PATCH 4/8] fix: guard empty merge-base, use labels_handler, fix test leaks - Guard against empty merge-base stdout before building diff ranges - Use labels_handler.pull_request_labels_names() instead of direct pull_request.labels access in _post_clean_rebase_comment - Replace class-level type() mutations with scoped AsyncMock in tests --- .../libs/handlers/pull_request_handler.py | 26 ++-- .../tests/test_clean_rebase_detection.py | 117 +++++++++++------- 2 files changed, 91 insertions(+), 52 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 6e1a48cd..4e57249e 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -126,6 +126,11 @@ async def _is_clean_rebase(self, pull_request: PullRequest) -> bool: ) return False old_merge_base = old_merge_base_out.strip() + if not old_merge_base: + self.logger.warning( + f"{self.log_prefix} Clean rebase detection: empty merge-base for old head {before_sha[:7]}" + ) + return False old_merge_base_q = shlex.quote(old_merge_base) # Step 3: Get merge-base for new head @@ -141,6 +146,11 @@ async def _is_clean_rebase(self, pull_request: PullRequest) -> bool: ) return False new_merge_base = new_merge_base_out.strip() + if not new_merge_base: + self.logger.warning( + f"{self.log_prefix} Clean rebase detection: empty merge-base for new head {after_sha[:7]}" + ) + return False new_merge_base_q = shlex.quote(new_merge_base) # Step 4: Compute diff hash for old range @@ -186,15 +196,15 @@ async def _is_clean_rebase(self, pull_request: PullRequest) -> bool: async def _post_clean_rebase_comment(self, pull_request: PullRequest, before_sha: str) -> None: """Post a comment about clean rebase detection. Best-effort -- failures are logged but don't block CI.""" try: - labels = await asyncio.to_thread(lambda: list(pull_request.labels)) + label_names = await self.labels_handler.pull_request_labels_names(pull_request=pull_request) review_labels = [ - label.name - for label in labels - if label.name.startswith(APPROVED_BY_LABEL_PREFIX) - or label.name.startswith(LGTM_BY_LABEL_PREFIX) - or label.name.startswith(COMMENTED_BY_LABEL_PREFIX) - or label.name.startswith(CHANGED_REQUESTED_BY_LABEL_PREFIX) - or label.name == VERIFIED_LABEL_STR + name + for name in label_names + if name.startswith(APPROVED_BY_LABEL_PREFIX) + or name.startswith(LGTM_BY_LABEL_PREFIX) + or name.startswith(COMMENTED_BY_LABEL_PREFIX) + or name.startswith(CHANGED_REQUESTED_BY_LABEL_PREFIX) + or name == VERIFIED_LABEL_STR ] if review_labels: diff --git a/webhook_server/tests/test_clean_rebase_detection.py b/webhook_server/tests/test_clean_rebase_detection.py index 30ceef0f..c4251b2c 100644 --- a/webhook_server/tests/test_clean_rebase_detection.py +++ b/webhook_server/tests/test_clean_rebase_detection.py @@ -456,6 +456,54 @@ async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tupl assert f"git -C {clone_dir_q} merge-base {base_ref_q} {before_sha_q}" == merge_base_cmds[0] assert f"git -C {clone_dir_q} merge-base {base_ref_q} {after_sha_q}" == merge_base_cmds[1] + @pytest.mark.asyncio + async def test_returns_false_when_old_merge_base_stdout_empty( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that _is_clean_rebase returns False when old merge-base stdout is empty after strip.""" + before_sha = handler.hook_data["before"] + after_sha = handler.hook_data["after"] + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + if f"fetch origin {before_sha}" in command: + return (True, "", "") + if "merge-base" in command and before_sha in command: + return (True, " \n", "") # empty after strip + if "merge-base" in command and after_sha in command: + return (True, "new_merge_base_sha\n", "") + return (True, "", "") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + result = await handler._is_clean_rebase(mock_pull_request) + + assert result is False + handler.logger.warning.assert_called() + assert "empty merge-base" in str(handler.logger.warning.call_args) + + @pytest.mark.asyncio + async def test_returns_false_when_new_merge_base_stdout_empty( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that _is_clean_rebase returns False when new merge-base stdout is empty after strip.""" + before_sha = handler.hook_data["before"] + after_sha = handler.hook_data["after"] + + async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: + if f"fetch origin {before_sha}" in command: + return (True, "", "") + if "merge-base" in command and before_sha in command: + return (True, "old_merge_base_sha\n", "") + if "merge-base" in command and after_sha in command: + return (True, "\n", "") # empty after strip + return (True, "", "") + + with patch("webhook_server.libs.handlers.pull_request_handler.run_command", side_effect=mock_run_command): + result = await handler._is_clean_rebase(mock_pull_request) + + assert result is False + handler.logger.warning.assert_called() + assert "empty merge-base" in str(handler.logger.warning.call_args) + @pytest.mark.asyncio async def test_returns_false_on_unexpected_exception( self, handler: PullRequestHandler, mock_pull_request: Mock @@ -579,13 +627,9 @@ async def test_synchronize_clean_rebase_posts_comment_with_preserved_labels( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that clean rebase posts a comment listing preserved review labels.""" - approved_label = Mock() - approved_label.name = f"{APPROVED_BY_LABEL_PREFIX}reviewer1" - lgtm_label = Mock() - lgtm_label.name = f"{LGTM_BY_LABEL_PREFIX}reviewer2" - non_review_label = Mock() - non_review_label.name = "bug" - mock_pull_request.labels = [approved_label, lgtm_label, non_review_label] + approved_name = f"{APPROVED_BY_LABEL_PREFIX}reviewer1" + lgtm_name = f"{LGTM_BY_LABEL_PREFIX}reviewer2" + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[approved_name, lgtm_name, "bug"]) before_sha = handler.hook_data["before"] @@ -600,8 +644,8 @@ async def test_synchronize_clean_rebase_posts_comment_with_preserved_labels( comment_body = mock_pull_request.create_issue_comment.call_args.kwargs["body"] assert "Clean rebase detected" in comment_body assert before_sha[:7] in comment_body - assert f"`{approved_label.name}`" in comment_body - assert f"`{lgtm_label.name}`" in comment_body + assert f"`{approved_name}`" in comment_body + assert f"`{lgtm_name}`" in comment_body assert "bug" not in comment_body @pytest.mark.asyncio @@ -609,9 +653,7 @@ async def test_synchronize_clean_rebase_no_review_labels_simple_comment( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that clean rebase with no review labels posts a simple comment.""" - non_review_label = Mock() - non_review_label.name = "bug" - mock_pull_request.labels = [non_review_label] + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=["bug"]) before_sha = handler.hook_data["before"] @@ -632,9 +674,7 @@ async def test_synchronize_clean_rebase_preserves_verified_label( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that clean rebase recognizes verified label as a review label to preserve.""" - verified_label = Mock() - verified_label.name = VERIFIED_LABEL_STR - mock_pull_request.labels = [verified_label] + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[VERIFIED_LABEL_STR]) with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), @@ -651,9 +691,8 @@ async def test_synchronize_clean_rebase_preserves_changes_requested_label( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that clean rebase recognizes changes-requested label as a review label.""" - cr_label = Mock() - cr_label.name = f"{CHANGED_REQUESTED_BY_LABEL_PREFIX}reviewer1" - mock_pull_request.labels = [cr_label] + cr_name = f"{CHANGED_REQUESTED_BY_LABEL_PREFIX}reviewer1" + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[cr_name]) with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), @@ -663,16 +702,15 @@ async def test_synchronize_clean_rebase_preserves_changes_requested_label( mock_pull_request.create_issue_comment.assert_called_once() comment_body = mock_pull_request.create_issue_comment.call_args.kwargs["body"] - assert f"`{cr_label.name}`" in comment_body + assert f"`{cr_name}`" in comment_body @pytest.mark.asyncio async def test_synchronize_clean_rebase_preserves_commented_by_label( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that clean rebase recognizes commented-by label as a review label.""" - commented_label = Mock() - commented_label.name = f"{COMMENTED_BY_LABEL_PREFIX}reviewer1" - mock_pull_request.labels = [commented_label] + commented_name = f"{COMMENTED_BY_LABEL_PREFIX}reviewer1" + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[commented_name]) with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), @@ -682,7 +720,7 @@ async def test_synchronize_clean_rebase_preserves_commented_by_label( mock_pull_request.create_issue_comment.assert_called_once() comment_body = mock_pull_request.create_issue_comment.call_args.kwargs["body"] - assert f"`{commented_label.name}`" in comment_body + assert f"`{commented_name}`" in comment_body @pytest.mark.asyncio async def test_synchronize_clean_rebase_skips_verified_label_processing( @@ -699,9 +737,7 @@ async def test_synchronize_clean_rebase_skips_verified_label_processing( handler.github_webhook.parent_committer = "external-contributor" handler.github_webhook.auto_verified_and_merged_users = ["auto-user"] - verified_label = Mock() - verified_label.name = VERIFIED_LABEL_STR - mock_pull_request.labels = [verified_label] + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[VERIFIED_LABEL_STR]) with patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True): await handler.process_pull_request_webhook_data(mock_pull_request) @@ -723,7 +759,7 @@ async def test_synchronize_clean_rebase_handles_task_failure_gracefully( When the clean rebase path runs comment posting and process in parallel via gather, an exception in one task should be logged but not crash the other. """ - mock_pull_request.labels = [] + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[]) with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), @@ -749,13 +785,9 @@ class TestPostCleanRebaseComment: @pytest.mark.asyncio async def test_posts_comment_with_review_labels(self, handler: PullRequestHandler, mock_pull_request: Mock) -> None: """Test that _post_clean_rebase_comment posts a comment listing preserved review labels.""" - approved_label = Mock() - approved_label.name = f"{APPROVED_BY_LABEL_PREFIX}reviewer1" - lgtm_label = Mock() - lgtm_label.name = f"{LGTM_BY_LABEL_PREFIX}reviewer2" - non_review_label = Mock() - non_review_label.name = "bug" - mock_pull_request.labels = [approved_label, lgtm_label, non_review_label] + approved_name = f"{APPROVED_BY_LABEL_PREFIX}reviewer1" + lgtm_name = f"{LGTM_BY_LABEL_PREFIX}reviewer2" + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[approved_name, lgtm_name, "bug"]) before_sha = "abc1234567890" # pragma: allowlist secret await handler._post_clean_rebase_comment(pull_request=mock_pull_request, before_sha=before_sha) @@ -764,8 +796,8 @@ async def test_posts_comment_with_review_labels(self, handler: PullRequestHandle comment_body = mock_pull_request.create_issue_comment.call_args.kwargs["body"] assert "Clean rebase detected" in comment_body assert before_sha[:7] in comment_body - assert f"`{approved_label.name}`" in comment_body - assert f"`{lgtm_label.name}`" in comment_body + assert f"`{approved_name}`" in comment_body + assert f"`{lgtm_name}`" in comment_body assert "bug" not in comment_body @pytest.mark.asyncio @@ -773,9 +805,7 @@ async def test_posts_simple_comment_without_review_labels( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that _post_clean_rebase_comment posts a simple comment when no review labels exist.""" - non_review_label = Mock() - non_review_label.name = "bug" - mock_pull_request.labels = [non_review_label] + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=["bug"]) before_sha = "abc1234567890" # pragma: allowlist secret await handler._post_clean_rebase_comment(pull_request=mock_pull_request, before_sha=before_sha) @@ -790,8 +820,7 @@ async def test_logs_and_does_not_raise_on_label_fetch_failure( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that _post_clean_rebase_comment logs the error and does not raise when labels fetch fails.""" - # Make the labels property on the mock's type raise when accessed - type(mock_pull_request).labels = property(lambda self: (_ for _ in ()).throw(RuntimeError("API error"))) + handler.labels_handler.pull_request_labels_names = AsyncMock(side_effect=RuntimeError("API error")) before_sha = "abc1234567890" # pragma: allowlist secret # Should not raise @@ -805,7 +834,7 @@ async def test_logs_and_does_not_raise_on_comment_post_failure( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that _post_clean_rebase_comment logs the error when comment posting fails.""" - mock_pull_request.labels = [] + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[]) mock_pull_request.create_issue_comment = Mock(side_effect=RuntimeError("API error")) before_sha = "abc1234567890" # pragma: allowlist secret @@ -818,7 +847,7 @@ async def test_logs_and_does_not_raise_on_comment_post_failure( @pytest.mark.asyncio async def test_reraises_cancelled_error(self, handler: PullRequestHandler, mock_pull_request: Mock) -> None: """Test that _post_clean_rebase_comment re-raises asyncio.CancelledError.""" - type(mock_pull_request).labels = property(lambda self: (_ for _ in ()).throw(asyncio.CancelledError())) + handler.labels_handler.pull_request_labels_names = AsyncMock(side_effect=asyncio.CancelledError()) before_sha = "abc1234567890" # pragma: allowlist secret with pytest.raises(asyncio.CancelledError): @@ -832,7 +861,7 @@ async def test_synchronize_continues_when_comment_fails( This verifies that _post_clean_rebase_comment failure does not block CI processing. """ - mock_pull_request.labels = [] + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[]) mock_pull_request.create_issue_comment = Mock(side_effect=RuntimeError("API error")) with ( From 7a320e74575b8286d4bbc8540717ab5704b0ce31 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 31 Mar 2026 14:31:38 +0300 Subject: [PATCH 5/8] fix: use webhook payload for base_ref and stub test_oracle in tests - Read base.ref from hook_data instead of live PyGithub API call - Stub call_test_oracle in all synchronize tests to prevent background task leaks - Rename unused pull_request param to _pull_request in _is_clean_rebase --- .../libs/handlers/pull_request_handler.py | 8 +-- .../tests/test_clean_rebase_detection.py | 50 +++++++++++++++++-- 2 files changed, 50 insertions(+), 8 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 4e57249e..eff21433 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -71,7 +71,7 @@ def __init__(self, github_webhook: GithubWebhook, owners_file_handler: OwnersFil github_webhook=self.github_webhook, owners_file_handler=self.owners_file_handler ) - async def _is_clean_rebase(self, pull_request: PullRequest) -> bool: + async def _is_clean_rebase(self, _pull_request: PullRequest) -> bool: """Detect whether a synchronize event is a clean rebase (same code changes on a newer base). Compares the sha256 hash of the diff between merge-base..HEAD for both @@ -79,7 +79,7 @@ async def _is_clean_rebase(self, pull_request: PullRequest) -> bool: the push was a pure rebase with no code changes. Args: - pull_request: The GitHub pull request object. + _pull_request: The GitHub pull request object (unused; kept for caller compatibility). Returns: True if the push is a clean rebase, False otherwise. @@ -92,7 +92,7 @@ async def _is_clean_rebase(self, pull_request: PullRequest) -> bool: before_sha: str = self.hook_data["before"] after_sha: str = self.hook_data["after"] - base_ref: str = await asyncio.to_thread(lambda: pull_request.base.ref) + base_ref: str = self.hook_data["pull_request"]["base"]["ref"] clone_dir: str = self.github_webhook.clone_repo_dir clone_dir_q = shlex.quote(clone_dir) @@ -279,7 +279,7 @@ async def process_pull_request_webhook_data(self, pull_request: PullRequest) -> return if hook_action == "synchronize": - clean_rebase = await self._is_clean_rebase(pull_request=pull_request) + clean_rebase = await self._is_clean_rebase(_pull_request=pull_request) if clean_rebase: before_sha: str = self.hook_data["before"] diff --git a/webhook_server/tests/test_clean_rebase_detection.py b/webhook_server/tests/test_clean_rebase_detection.py index c4251b2c..f89ef1bc 100644 --- a/webhook_server/tests/test_clean_rebase_detection.py +++ b/webhook_server/tests/test_clean_rebase_detection.py @@ -35,7 +35,7 @@ def mock_github_webhook() -> Mock: "action": "synchronize", "before": "aaa1111111111111111111111111111111111111", "after": "bbb2222222222222222222222222222222222222", - "pull_request": {"number": 42, "merged": False, "title": "Test PR"}, + "pull_request": {"number": 42, "merged": False, "title": "Test PR", "base": {"ref": "main"}}, "sender": {"login": "test-user"}, "label": {"name": "bug"}, } @@ -346,8 +346,8 @@ async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tupl @pytest.mark.asyncio async def test_uses_correct_base_ref(self, handler: PullRequestHandler, mock_pull_request: Mock) -> None: - """Test that _is_clean_rebase uses pull_request.base.ref for merge-base commands.""" - mock_pull_request.base.ref = "develop" + """Test that _is_clean_rebase uses hook_data base.ref for merge-base commands.""" + handler.hook_data["pull_request"]["base"]["ref"] = "develop" commands_received: list[str] = [] async def mock_run_command(command: str, log_prefix: str, **kwargs: Any) -> tuple[bool, str, str]: @@ -601,6 +601,10 @@ async def test_synchronize_clean_rebase_skips_label_removal( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock) as mock_process, patch.object(handler, "remove_labels_when_pull_request_sync", new_callable=AsyncMock) as mock_remove_labels, + patch( + "webhook_server.libs.handlers.pull_request_handler.call_test_oracle", + new_callable=AsyncMock, + ), ): await handler.process_pull_request_webhook_data(mock_pull_request) @@ -616,6 +620,10 @@ async def test_synchronize_not_clean_rebase_removes_labels( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=False), patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock) as mock_process, patch.object(handler, "remove_labels_when_pull_request_sync", new_callable=AsyncMock) as mock_remove_labels, + patch( + "webhook_server.libs.handlers.pull_request_handler.call_test_oracle", + new_callable=AsyncMock, + ), ): await handler.process_pull_request_webhook_data(mock_pull_request) @@ -636,6 +644,10 @@ async def test_synchronize_clean_rebase_posts_comment_with_preserved_labels( with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock), + patch( + "webhook_server.libs.handlers.pull_request_handler.call_test_oracle", + new_callable=AsyncMock, + ), ): await handler.process_pull_request_webhook_data(mock_pull_request) @@ -660,6 +672,10 @@ async def test_synchronize_clean_rebase_no_review_labels_simple_comment( with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock), + patch( + "webhook_server.libs.handlers.pull_request_handler.call_test_oracle", + new_callable=AsyncMock, + ), ): await handler.process_pull_request_webhook_data(mock_pull_request) @@ -679,6 +695,10 @@ async def test_synchronize_clean_rebase_preserves_verified_label( with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock), + patch( + "webhook_server.libs.handlers.pull_request_handler.call_test_oracle", + new_callable=AsyncMock, + ), ): await handler.process_pull_request_webhook_data(mock_pull_request) @@ -697,6 +717,10 @@ async def test_synchronize_clean_rebase_preserves_changes_requested_label( with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock), + patch( + "webhook_server.libs.handlers.pull_request_handler.call_test_oracle", + new_callable=AsyncMock, + ), ): await handler.process_pull_request_webhook_data(mock_pull_request) @@ -715,6 +739,10 @@ async def test_synchronize_clean_rebase_preserves_commented_by_label( with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock), + patch( + "webhook_server.libs.handlers.pull_request_handler.call_test_oracle", + new_callable=AsyncMock, + ), ): await handler.process_pull_request_webhook_data(mock_pull_request) @@ -739,7 +767,13 @@ async def test_synchronize_clean_rebase_skips_verified_label_processing( handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[VERIFIED_LABEL_STR]) - with patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True): + with ( + patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), + patch( + "webhook_server.libs.handlers.pull_request_handler.call_test_oracle", + new_callable=AsyncMock, + ), + ): await handler.process_pull_request_webhook_data(mock_pull_request) # Verified label should NOT have been removed (no _remove_label call for verified) @@ -769,6 +803,10 @@ async def test_synchronize_clean_rebase_handles_task_failure_gracefully( new_callable=AsyncMock, side_effect=RuntimeError("test error"), ), + patch( + "webhook_server.libs.handlers.pull_request_handler.call_test_oracle", + new_callable=AsyncMock, + ), ): # Should not raise even though process_opened_or_synchronize_pull_request fails await handler.process_pull_request_webhook_data(mock_pull_request) @@ -867,6 +905,10 @@ async def test_synchronize_continues_when_comment_fails( with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock) as mock_process, + patch( + "webhook_server.libs.handlers.pull_request_handler.call_test_oracle", + new_callable=AsyncMock, + ), ): await handler.process_pull_request_webhook_data(mock_pull_request) From b22d2c960dd6b7d7876d6b18411b968304304d2e Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 31 Mar 2026 14:57:02 +0300 Subject: [PATCH 6/8] fix: sync verified check run on clean rebase without removing label Add _sync_verified_check_for_clean_rebase() that refreshes the verified check run state on the new commit SHA to match the existing label, without removing/re-adding it. This prevents stale check state after clean rebases while preserving the label. --- .../libs/handlers/pull_request_handler.py | 20 +++++++++- .../tests/test_clean_rebase_detection.py | 38 +++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index eff21433..ca847cc9 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -1033,7 +1033,9 @@ async def process_opened_or_synchronize_pull_request( if self.github_webhook.build_and_push_container: setup_tasks.append(self.check_run_handler.set_check_queued(name=BUILD_CONTAINER_STR)) - if not is_clean_rebase: + if is_clean_rebase: + setup_tasks.append(self._sync_verified_check_for_clean_rebase(pull_request=pull_request)) + else: setup_tasks.append(self._process_verified_for_update_or_new_pull_request(pull_request=pull_request)) setup_tasks.append(self.labels_handler.add_size_label(pull_request=pull_request)) setup_tasks.append(self.add_pull_request_owner_as_assingee(pull_request=pull_request)) @@ -1395,6 +1397,22 @@ async def _process_verified_for_update_or_new_pull_request(self, pull_request: P 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) + async def _sync_verified_check_for_clean_rebase(self, pull_request: PullRequest) -> None: + """Sync the verified check run to the new commit SHA after a clean rebase. + + Unlike _process_verified_for_update_or_new_pull_request, this does NOT + remove/re-add the verified label. It only refreshes the check run state + to match the existing label on the new head commit. + """ + if not self.github_webhook.verified_job: + return + + label_names = await self.labels_handler.pull_request_labels_names(pull_request=pull_request) + if VERIFIED_LABEL_STR in label_names: + await self.check_run_handler.set_check_success(name=VERIFIED_LABEL_STR) + else: + await self.check_run_handler.set_check_queued(name=VERIFIED_LABEL_STR) + async def add_pull_request_owner_as_assingee(self, pull_request: PullRequest) -> None: try: self.logger.info(f"{self.log_prefix} Adding PR owner as assignee") diff --git a/webhook_server/tests/test_clean_rebase_detection.py b/webhook_server/tests/test_clean_rebase_detection.py index f89ef1bc..46b8035e 100644 --- a/webhook_server/tests/test_clean_rebase_detection.py +++ b/webhook_server/tests/test_clean_rebase_detection.py @@ -914,3 +914,41 @@ async def test_synchronize_continues_when_comment_fails( # process_opened_or_synchronize_pull_request should still have been called mock_process.assert_called_once_with(pull_request=mock_pull_request, is_clean_rebase=True) + + +class TestSyncVerifiedCheckForCleanRebase: + """Test suite for _sync_verified_check_for_clean_rebase method.""" + + @pytest.mark.asyncio + async def test_sets_check_success_when_verified_label_exists( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that check run is set to success when verified label is present on the PR.""" + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[VERIFIED_LABEL_STR, "other"]) + + await handler._sync_verified_check_for_clean_rebase(pull_request=mock_pull_request) + + handler.check_run_handler.set_check_success.assert_called_once_with(name=VERIFIED_LABEL_STR) + handler.check_run_handler.set_check_queued.assert_not_called() + + @pytest.mark.asyncio + async def test_sets_check_queued_when_verified_label_missing( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that check run is set to queued when verified label is not present on the PR.""" + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=["other"]) + + await handler._sync_verified_check_for_clean_rebase(pull_request=mock_pull_request) + + handler.check_run_handler.set_check_queued.assert_called_once_with(name=VERIFIED_LABEL_STR) + handler.check_run_handler.set_check_success.assert_not_called() + + @pytest.mark.asyncio + async def test_skips_when_verified_job_disabled(self, handler: PullRequestHandler, mock_pull_request: Mock) -> None: + """Test that neither check success nor queued is called when verified_job is disabled.""" + handler.github_webhook.verified_job = False + + await handler._sync_verified_check_for_clean_rebase(pull_request=mock_pull_request) + + handler.check_run_handler.set_check_success.assert_not_called() + handler.check_run_handler.set_check_queued.assert_not_called() From a003079a0b948cd99eeaa87e9df37a818fd59294 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 31 Mar 2026 15:32:49 +0300 Subject: [PATCH 7/8] fix: reuse label snapshot and update verification tip text - Fetch labels once in synchronize handler and pass to both helpers to avoid duplicate API calls and race windows - Update verification tip in welcome message to reflect clean rebase behavior --- .../libs/handlers/pull_request_handler.py | 29 ++++-- .../tests/test_clean_rebase_detection.py | 96 ++++++++++++++----- 2 files changed, 93 insertions(+), 32 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index ca847cc9..d7079a7e 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -193,10 +193,11 @@ async def _is_clean_rebase(self, _pull_request: PullRequest) -> bool: ) return False - async def _post_clean_rebase_comment(self, pull_request: PullRequest, before_sha: str) -> None: + async def _post_clean_rebase_comment( + self, pull_request: PullRequest, before_sha: str, label_names: list[str] + ) -> None: """Post a comment about clean rebase detection. Best-effort -- failures are logged but don't block CI.""" try: - label_names = await self.labels_handler.pull_request_labels_names(pull_request=pull_request) review_labels = [ name for name in label_names @@ -283,9 +284,14 @@ async def process_pull_request_webhook_data(self, pull_request: PullRequest) -> if clean_rebase: before_sha: str = self.hook_data["before"] + label_names = await self.labels_handler.pull_request_labels_names(pull_request=pull_request) sync_tasks = [ - self._post_clean_rebase_comment(pull_request=pull_request, before_sha=before_sha), - self.process_opened_or_synchronize_pull_request(pull_request=pull_request, is_clean_rebase=True), + self._post_clean_rebase_comment( + pull_request=pull_request, before_sha=before_sha, label_names=label_names + ), + self.process_opened_or_synchronize_pull_request( + pull_request=pull_request, is_clean_rebase=True, label_names=label_names + ), ] else: sync_tasks = [ @@ -644,7 +650,10 @@ def _prepare_tips_section(self) -> str: tips.append("* **WIP Status**: Use `/wip` when your PR is not ready for review") if self.labels_handler.is_label_enabled(VERIFIED_LABEL_STR): - tips.append("* **Verification**: The verified label is automatically removed on each new commit") + tips.append( + "* **Verification**: The verified label is removed on new commits" + " unless the push is detected as a clean rebase" + ) # Cherry-pick tip - check if cherry-pick labels are enabled if self.labels_handler.is_label_enabled(CHERRY_PICKED_LABEL): @@ -1002,7 +1011,7 @@ def _find_matching_issue() -> Any | None: await asyncio.to_thread(matching_issue.edit, state="closed") async def process_opened_or_synchronize_pull_request( - self, pull_request: PullRequest, is_clean_rebase: bool = False + self, pull_request: PullRequest, is_clean_rebase: bool = False, label_names: list[str] | None = None ) -> None: if self.ctx: self.ctx.start_step("pr_workflow_setup") @@ -1034,7 +1043,10 @@ async def process_opened_or_synchronize_pull_request( setup_tasks.append(self.check_run_handler.set_check_queued(name=BUILD_CONTAINER_STR)) if is_clean_rebase: - setup_tasks.append(self._sync_verified_check_for_clean_rebase(pull_request=pull_request)) + # label_names is guaranteed non-None when is_clean_rebase=True (caller always provides it) + setup_tasks.append( + self._sync_verified_check_for_clean_rebase(_pull_request=pull_request, label_names=label_names) # type: ignore[arg-type] + ) else: setup_tasks.append(self._process_verified_for_update_or_new_pull_request(pull_request=pull_request)) setup_tasks.append(self.labels_handler.add_size_label(pull_request=pull_request)) @@ -1397,7 +1409,7 @@ async def _process_verified_for_update_or_new_pull_request(self, pull_request: P 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) - async def _sync_verified_check_for_clean_rebase(self, pull_request: PullRequest) -> None: + async def _sync_verified_check_for_clean_rebase(self, _pull_request: PullRequest, label_names: list[str]) -> None: """Sync the verified check run to the new commit SHA after a clean rebase. Unlike _process_verified_for_update_or_new_pull_request, this does NOT @@ -1407,7 +1419,6 @@ async def _sync_verified_check_for_clean_rebase(self, pull_request: PullRequest) if not self.github_webhook.verified_job: return - label_names = await self.labels_handler.pull_request_labels_names(pull_request=pull_request) if VERIFIED_LABEL_STR in label_names: await self.check_run_handler.set_check_success(name=VERIFIED_LABEL_STR) else: diff --git a/webhook_server/tests/test_clean_rebase_detection.py b/webhook_server/tests/test_clean_rebase_detection.py index 46b8035e..16edbe9b 100644 --- a/webhook_server/tests/test_clean_rebase_detection.py +++ b/webhook_server/tests/test_clean_rebase_detection.py @@ -597,6 +597,8 @@ async def test_synchronize_clean_rebase_skips_label_removal( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that synchronize with clean rebase does NOT call remove_labels_when_pull_request_sync.""" + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=["bug"]) + with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock) as mock_process, @@ -608,7 +610,9 @@ async def test_synchronize_clean_rebase_skips_label_removal( ): await handler.process_pull_request_webhook_data(mock_pull_request) - mock_process.assert_called_once_with(pull_request=mock_pull_request, is_clean_rebase=True) + mock_process.assert_called_once_with( + pull_request=mock_pull_request, is_clean_rebase=True, label_names=["bug"] + ) mock_remove_labels.assert_not_called() @pytest.mark.asyncio @@ -637,7 +641,8 @@ async def test_synchronize_clean_rebase_posts_comment_with_preserved_labels( """Test that clean rebase posts a comment listing preserved review labels.""" approved_name = f"{APPROVED_BY_LABEL_PREFIX}reviewer1" lgtm_name = f"{LGTM_BY_LABEL_PREFIX}reviewer2" - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[approved_name, lgtm_name, "bug"]) + label_names = [approved_name, lgtm_name, "bug"] + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=label_names) before_sha = handler.hook_data["before"] @@ -651,6 +656,9 @@ async def test_synchronize_clean_rebase_posts_comment_with_preserved_labels( ): await handler.process_pull_request_webhook_data(mock_pull_request) + # Labels should be fetched exactly once (by the synchronize handler) + handler.labels_handler.pull_request_labels_names.assert_called_once_with(pull_request=mock_pull_request) + # create_issue_comment is called via asyncio.to_thread which executes it mock_pull_request.create_issue_comment.assert_called_once() comment_body = mock_pull_request.create_issue_comment.call_args.kwargs["body"] @@ -750,6 +758,32 @@ async def test_synchronize_clean_rebase_preserves_commented_by_label( comment_body = mock_pull_request.create_issue_comment.call_args.kwargs["body"] assert f"`{commented_name}`" in comment_body + @pytest.mark.asyncio + async def test_synchronize_clean_rebase_fetches_labels_only_once( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that the synchronize clean-rebase path fetches labels exactly once and reuses them.""" + label_names = [VERIFIED_LABEL_STR, f"{APPROVED_BY_LABEL_PREFIX}reviewer1"] + handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=label_names) + + with ( + patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), + patch.object(handler, "process_opened_or_synchronize_pull_request", new_callable=AsyncMock) as mock_process, + patch( + "webhook_server.libs.handlers.pull_request_handler.call_test_oracle", + new_callable=AsyncMock, + ), + ): + await handler.process_pull_request_webhook_data(mock_pull_request) + + # Labels fetched exactly once + handler.labels_handler.pull_request_labels_names.assert_called_once_with(pull_request=mock_pull_request) + + # label_names passed through to process_opened_or_synchronize_pull_request + mock_process.assert_called_once_with( + pull_request=mock_pull_request, is_clean_rebase=True, label_names=label_names + ) + @pytest.mark.asyncio async def test_synchronize_clean_rebase_skips_verified_label_processing( self, handler: PullRequestHandler, mock_pull_request: Mock @@ -825,10 +859,12 @@ async def test_posts_comment_with_review_labels(self, handler: PullRequestHandle """Test that _post_clean_rebase_comment posts a comment listing preserved review labels.""" approved_name = f"{APPROVED_BY_LABEL_PREFIX}reviewer1" lgtm_name = f"{LGTM_BY_LABEL_PREFIX}reviewer2" - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[approved_name, lgtm_name, "bug"]) + label_names = [approved_name, lgtm_name, "bug"] before_sha = "abc1234567890" # pragma: allowlist secret - await handler._post_clean_rebase_comment(pull_request=mock_pull_request, before_sha=before_sha) + await handler._post_clean_rebase_comment( + pull_request=mock_pull_request, before_sha=before_sha, label_names=label_names + ) mock_pull_request.create_issue_comment.assert_called_once() comment_body = mock_pull_request.create_issue_comment.call_args.kwargs["body"] @@ -843,10 +879,12 @@ async def test_posts_simple_comment_without_review_labels( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that _post_clean_rebase_comment posts a simple comment when no review labels exist.""" - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=["bug"]) + label_names = ["bug"] before_sha = "abc1234567890" # pragma: allowlist secret - await handler._post_clean_rebase_comment(pull_request=mock_pull_request, before_sha=before_sha) + await handler._post_clean_rebase_comment( + pull_request=mock_pull_request, before_sha=before_sha, label_names=label_names + ) mock_pull_request.create_issue_comment.assert_called_once() comment_body = mock_pull_request.create_issue_comment.call_args.kwargs["body"] @@ -854,30 +892,29 @@ async def test_posts_simple_comment_without_review_labels( assert "preserved" not in comment_body @pytest.mark.asyncio - async def test_logs_and_does_not_raise_on_label_fetch_failure( + async def test_does_not_call_pull_request_labels_names( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: - """Test that _post_clean_rebase_comment logs the error and does not raise when labels fetch fails.""" - handler.labels_handler.pull_request_labels_names = AsyncMock(side_effect=RuntimeError("API error")) + """Test that _post_clean_rebase_comment does NOT call pull_request_labels_names (labels are passed in).""" + label_names = ["bug"] before_sha = "abc1234567890" # pragma: allowlist secret - # Should not raise - await handler._post_clean_rebase_comment(pull_request=mock_pull_request, before_sha=before_sha) + await handler._post_clean_rebase_comment( + pull_request=mock_pull_request, before_sha=before_sha, label_names=label_names + ) - handler.logger.exception.assert_called_once() - assert "Failed to post clean-rebase comment" in str(handler.logger.exception.call_args) + handler.labels_handler.pull_request_labels_names.assert_not_called() @pytest.mark.asyncio async def test_logs_and_does_not_raise_on_comment_post_failure( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that _post_clean_rebase_comment logs the error when comment posting fails.""" - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[]) mock_pull_request.create_issue_comment = Mock(side_effect=RuntimeError("API error")) before_sha = "abc1234567890" # pragma: allowlist secret # Should not raise - await handler._post_clean_rebase_comment(pull_request=mock_pull_request, before_sha=before_sha) + await handler._post_clean_rebase_comment(pull_request=mock_pull_request, before_sha=before_sha, label_names=[]) handler.logger.exception.assert_called_once() assert "Failed to post clean-rebase comment" in str(handler.logger.exception.call_args) @@ -885,11 +922,13 @@ async def test_logs_and_does_not_raise_on_comment_post_failure( @pytest.mark.asyncio async def test_reraises_cancelled_error(self, handler: PullRequestHandler, mock_pull_request: Mock) -> None: """Test that _post_clean_rebase_comment re-raises asyncio.CancelledError.""" - handler.labels_handler.pull_request_labels_names = AsyncMock(side_effect=asyncio.CancelledError()) + mock_pull_request.create_issue_comment = Mock(side_effect=asyncio.CancelledError()) before_sha = "abc1234567890" # pragma: allowlist secret with pytest.raises(asyncio.CancelledError): - await handler._post_clean_rebase_comment(pull_request=mock_pull_request, before_sha=before_sha) + await handler._post_clean_rebase_comment( + pull_request=mock_pull_request, before_sha=before_sha, label_names=[] + ) @pytest.mark.asyncio async def test_synchronize_continues_when_comment_fails( @@ -913,7 +952,7 @@ async def test_synchronize_continues_when_comment_fails( await handler.process_pull_request_webhook_data(mock_pull_request) # process_opened_or_synchronize_pull_request should still have been called - mock_process.assert_called_once_with(pull_request=mock_pull_request, is_clean_rebase=True) + mock_process.assert_called_once_with(pull_request=mock_pull_request, is_clean_rebase=True, label_names=[]) class TestSyncVerifiedCheckForCleanRebase: @@ -924,9 +963,9 @@ async def test_sets_check_success_when_verified_label_exists( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that check run is set to success when verified label is present on the PR.""" - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[VERIFIED_LABEL_STR, "other"]) + label_names = [VERIFIED_LABEL_STR, "other"] - await handler._sync_verified_check_for_clean_rebase(pull_request=mock_pull_request) + await handler._sync_verified_check_for_clean_rebase(_pull_request=mock_pull_request, label_names=label_names) handler.check_run_handler.set_check_success.assert_called_once_with(name=VERIFIED_LABEL_STR) handler.check_run_handler.set_check_queued.assert_not_called() @@ -936,9 +975,9 @@ async def test_sets_check_queued_when_verified_label_missing( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that check run is set to queued when verified label is not present on the PR.""" - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=["other"]) + label_names = ["other"] - await handler._sync_verified_check_for_clean_rebase(pull_request=mock_pull_request) + await handler._sync_verified_check_for_clean_rebase(_pull_request=mock_pull_request, label_names=label_names) handler.check_run_handler.set_check_queued.assert_called_once_with(name=VERIFIED_LABEL_STR) handler.check_run_handler.set_check_success.assert_not_called() @@ -948,7 +987,18 @@ async def test_skips_when_verified_job_disabled(self, handler: PullRequestHandle """Test that neither check success nor queued is called when verified_job is disabled.""" handler.github_webhook.verified_job = False - await handler._sync_verified_check_for_clean_rebase(pull_request=mock_pull_request) + await handler._sync_verified_check_for_clean_rebase(_pull_request=mock_pull_request, label_names=[]) handler.check_run_handler.set_check_success.assert_not_called() handler.check_run_handler.set_check_queued.assert_not_called() + + @pytest.mark.asyncio + async def test_does_not_call_pull_request_labels_names( + self, handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test that _sync_verified_check_for_clean_rebase does NOT call pull_request_labels_names.""" + label_names = [VERIFIED_LABEL_STR] + + await handler._sync_verified_check_for_clean_rebase(_pull_request=mock_pull_request, label_names=label_names) + + handler.labels_handler.pull_request_labels_names.assert_not_called() From 57e035c2e8e447552b718843bfc87e6111716127 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Tue, 31 Mar 2026 15:59:01 +0300 Subject: [PATCH 8/8] fix: read labels from webhook payload instead of API call Use pull_request_data["labels"] from the webhook payload instead of calling pull_request_labels_names() to avoid blocking CI if the API call fails on the clean rebase path. --- .../libs/handlers/pull_request_handler.py | 2 +- .../tests/test_clean_rebase_detection.py | 39 ++++++++++--------- 2 files changed, 22 insertions(+), 19 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index d7079a7e..580f9ad8 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -284,7 +284,7 @@ async def process_pull_request_webhook_data(self, pull_request: PullRequest) -> if clean_rebase: before_sha: str = self.hook_data["before"] - label_names = await self.labels_handler.pull_request_labels_names(pull_request=pull_request) + label_names = [label["name"] for label in pull_request_data.get("labels", [])] sync_tasks = [ self._post_clean_rebase_comment( pull_request=pull_request, before_sha=before_sha, label_names=label_names diff --git a/webhook_server/tests/test_clean_rebase_detection.py b/webhook_server/tests/test_clean_rebase_detection.py index 16edbe9b..51cb9d03 100644 --- a/webhook_server/tests/test_clean_rebase_detection.py +++ b/webhook_server/tests/test_clean_rebase_detection.py @@ -597,7 +597,7 @@ async def test_synchronize_clean_rebase_skips_label_removal( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that synchronize with clean rebase does NOT call remove_labels_when_pull_request_sync.""" - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=["bug"]) + handler.hook_data["pull_request"]["labels"] = [{"name": "bug"}] with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), @@ -642,7 +642,7 @@ async def test_synchronize_clean_rebase_posts_comment_with_preserved_labels( approved_name = f"{APPROVED_BY_LABEL_PREFIX}reviewer1" lgtm_name = f"{LGTM_BY_LABEL_PREFIX}reviewer2" label_names = [approved_name, lgtm_name, "bug"] - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=label_names) + handler.hook_data["pull_request"]["labels"] = [{"name": name} for name in label_names] before_sha = handler.hook_data["before"] @@ -656,8 +656,8 @@ async def test_synchronize_clean_rebase_posts_comment_with_preserved_labels( ): await handler.process_pull_request_webhook_data(mock_pull_request) - # Labels should be fetched exactly once (by the synchronize handler) - handler.labels_handler.pull_request_labels_names.assert_called_once_with(pull_request=mock_pull_request) + # Labels should NOT be fetched via API call - they come from webhook payload + handler.labels_handler.pull_request_labels_names.assert_not_called() # create_issue_comment is called via asyncio.to_thread which executes it mock_pull_request.create_issue_comment.assert_called_once() @@ -673,7 +673,7 @@ async def test_synchronize_clean_rebase_no_review_labels_simple_comment( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that clean rebase with no review labels posts a simple comment.""" - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=["bug"]) + handler.hook_data["pull_request"]["labels"] = [{"name": "bug"}] before_sha = handler.hook_data["before"] @@ -698,7 +698,7 @@ async def test_synchronize_clean_rebase_preserves_verified_label( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: """Test that clean rebase recognizes verified label as a review label to preserve.""" - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[VERIFIED_LABEL_STR]) + handler.hook_data["pull_request"]["labels"] = [{"name": VERIFIED_LABEL_STR}] with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), @@ -720,7 +720,7 @@ async def test_synchronize_clean_rebase_preserves_changes_requested_label( ) -> None: """Test that clean rebase recognizes changes-requested label as a review label.""" cr_name = f"{CHANGED_REQUESTED_BY_LABEL_PREFIX}reviewer1" - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[cr_name]) + handler.hook_data["pull_request"]["labels"] = [{"name": cr_name}] with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), @@ -742,7 +742,7 @@ async def test_synchronize_clean_rebase_preserves_commented_by_label( ) -> None: """Test that clean rebase recognizes commented-by label as a review label.""" commented_name = f"{COMMENTED_BY_LABEL_PREFIX}reviewer1" - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[commented_name]) + handler.hook_data["pull_request"]["labels"] = [{"name": commented_name}] with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), @@ -759,12 +759,15 @@ async def test_synchronize_clean_rebase_preserves_commented_by_label( assert f"`{commented_name}`" in comment_body @pytest.mark.asyncio - async def test_synchronize_clean_rebase_fetches_labels_only_once( + async def test_synchronize_clean_rebase_reads_labels_from_webhook_payload( self, handler: PullRequestHandler, mock_pull_request: Mock ) -> None: - """Test that the synchronize clean-rebase path fetches labels exactly once and reuses them.""" - label_names = [VERIFIED_LABEL_STR, f"{APPROVED_BY_LABEL_PREFIX}reviewer1"] - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=label_names) + """Test that the synchronize clean-rebase path reads labels from hook_data payload, not via API.""" + approved_label = f"{APPROVED_BY_LABEL_PREFIX}reviewer1" + label_names = [VERIFIED_LABEL_STR, approved_label] + + # Set labels in the webhook payload (hook_data["pull_request"]["labels"]) + handler.hook_data["pull_request"]["labels"] = [{"name": name} for name in label_names] with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), @@ -776,10 +779,10 @@ async def test_synchronize_clean_rebase_fetches_labels_only_once( ): await handler.process_pull_request_webhook_data(mock_pull_request) - # Labels fetched exactly once - handler.labels_handler.pull_request_labels_names.assert_called_once_with(pull_request=mock_pull_request) + # Labels should NOT be fetched via API call + handler.labels_handler.pull_request_labels_names.assert_not_called() - # label_names passed through to process_opened_or_synchronize_pull_request + # label_names from payload passed through to process_opened_or_synchronize_pull_request mock_process.assert_called_once_with( pull_request=mock_pull_request, is_clean_rebase=True, label_names=label_names ) @@ -799,7 +802,7 @@ async def test_synchronize_clean_rebase_skips_verified_label_processing( handler.github_webhook.parent_committer = "external-contributor" handler.github_webhook.auto_verified_and_merged_users = ["auto-user"] - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[VERIFIED_LABEL_STR]) + handler.hook_data["pull_request"]["labels"] = [{"name": VERIFIED_LABEL_STR}] with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), @@ -827,7 +830,7 @@ async def test_synchronize_clean_rebase_handles_task_failure_gracefully( When the clean rebase path runs comment posting and process in parallel via gather, an exception in one task should be logged but not crash the other. """ - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[]) + handler.hook_data["pull_request"]["labels"] = [] with ( patch.object(handler, "_is_clean_rebase", new_callable=AsyncMock, return_value=True), @@ -938,7 +941,7 @@ async def test_synchronize_continues_when_comment_fails( This verifies that _post_clean_rebase_comment failure does not block CI processing. """ - handler.labels_handler.pull_request_labels_names = AsyncMock(return_value=[]) + handler.hook_data["pull_request"]["labels"] = [] mock_pull_request.create_issue_comment = Mock(side_effect=RuntimeError("API error")) with (