From bfec32d64bb7e2d7f64f569ca9582c897000c950 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Mon, 10 Nov 2025 13:02:58 +0200 Subject: [PATCH 1/2] feat: Add /reprocess command for full PR workflow reprocessing Implement new user command `/reprocess` that triggers complete PR workflow reprocessing from scratch. This is useful when webhooks fail, processing is interrupted, or configuration changes require re-evaluation. Changes: - Add COMMAND_REPROCESS_STR constant - Implement process_command_reprocess() with merged PR check - Add helper methods for duplicate prevention: - _welcome_comment_exists() - check for existing welcome message - _tracking_issue_exists() - check for existing tracking issue - Extract reusable process_new_or_reprocess_pull_request() method - Add command handler in issue_comment_handler with OWNERS permission check - Update welcome message documentation - Add comprehensive tests (24 test cases, 90.22% coverage) - Update README with full command documentation Features: - Permission-gated (requires user in OWNERS file) - Skips merged PRs with clear message - Prevents duplicate welcome messages and tracking issues - Re-runs full workflow: reviewers, labels, checks, CI/CD - Comprehensive logging with logger.step() tracking --- README.md | 52 +++- .../libs/handlers/issue_comment_handler.py | 9 + .../libs/handlers/pull_request_handler.py | 95 ++++++ .../tests/test_issue_comment_handler.py | 147 +++++++++ .../tests/test_pull_request_handler.py | 283 ++++++++++++++++++ webhook_server/utils/constants.py | 1 + 6 files changed, 575 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index e12f23a49..4578eeb8f 100644 --- a/README.md +++ b/README.md @@ -1126,18 +1126,46 @@ Users can interact with the webhook server through GitHub comments on pull reque ### Pull Request Commands -| Command | Description | Example | -| ------------------- | ----------------------------- | ------------------- | -| `/verified` | Mark PR as verified | `/verified` | -| `/verified cancel` | Remove verification | `/verified cancel` | -| `/hold` | Block PR merging | `/hold` | -| `/hold cancel` | Unblock PR merging | `/hold cancel` | -| `/wip` | Mark as work in progress | `/wip` | -| `/wip cancel` | Remove WIP status | `/wip cancel` | -| `/lgtm` | Approve changes | `/lgtm` | -| `/approve` | Approve PR | `/approve` | -| `/assign-reviewers` | Assign OWNERS-based reviewers | `/assign-reviewers` | -| `/check-can-merge` | Check merge readiness | `/check-can-merge` | +| Command | Description | Example | +| ------------------- | --------------------------------------------------- | ------------------- | +| `/verified` | Mark PR as verified | `/verified` | +| `/verified cancel` | Remove verification | `/verified cancel` | +| `/hold` | Block PR merging | `/hold` | +| `/hold cancel` | Unblock PR merging | `/hold cancel` | +| `/wip` | Mark as work in progress | `/wip` | +| `/wip cancel` | Remove WIP status | `/wip cancel` | +| `/lgtm` | Approve changes | `/lgtm` | +| `/approve` | Approve PR | `/approve` | +| `/assign-reviewers` | Assign OWNERS-based reviewers | `/assign-reviewers` | +| `/check-can-merge` | Check merge readiness | `/check-can-merge` | +| `/reprocess` | Trigger complete PR workflow reprocessing (OWNERS only) | `/reprocess` | + +### Workflow Management + +#### PR Reprocessing + +The `/reprocess` command triggers complete PR workflow reprocessing from scratch, equivalent to reopening or synchronizing the PR. + +**Permissions**: Requires user to be in repository OWNERS file (same as `/retest`) + +**Use Cases**: +- Webhook delivery failed or was missed +- Processing interrupted mid-workflow +- OWNERS file changed and reviewers need reassignment +- Configuration changed and checks need re-evaluation +- PR got into inconsistent state and needs full reset + +**Behavior**: +- Re-runs entire PR workflow including reviewer assignment, label updates, check queuing, and CI/CD tests +- Won't create duplicate welcome messages or tracking issues if they already exist +- Respects current repository configuration and OWNERS file + +**Example**: + +```bash +# Comment on the pull request +/reprocess +``` ### Testing Commands diff --git a/webhook_server/libs/handlers/issue_comment_handler.py b/webhook_server/libs/handlers/issue_comment_handler.py index 19aa8ba7d..ee048e1b7 100644 --- a/webhook_server/libs/handlers/issue_comment_handler.py +++ b/webhook_server/libs/handlers/issue_comment_handler.py @@ -23,6 +23,7 @@ COMMAND_ASSIGN_REVIEWERS_STR, COMMAND_CHECK_CAN_MERGE_STR, COMMAND_CHERRY_PICK_STR, + COMMAND_REPROCESS_STR, COMMAND_RETEST_STR, CONVENTIONAL_TITLE_STR, HOLD_LABEL_STR, @@ -160,6 +161,7 @@ async def user_commands( ) -> None: available_commands: list[str] = [ COMMAND_RETEST_STR, + COMMAND_REPROCESS_STR, COMMAND_CHERRY_PICK_STR, COMMAND_ASSIGN_REVIEWERS_STR, COMMAND_CHECK_CAN_MERGE_STR, @@ -239,6 +241,13 @@ async def user_commands( pull_request=pull_request, command_args=_args, reviewed_user=reviewed_user ) + elif _command == COMMAND_REPROCESS_STR: + if not await self.owners_file_handler.is_user_valid_to_run_commands( + pull_request=pull_request, reviewed_user=reviewed_user + ): + return + await self.pull_request_handler.process_command_reprocess(pull_request=pull_request) + elif _command == BUILD_AND_PUSH_CONTAINER_STR: if self.github_webhook.build_and_push_container: await self.runner_handler.run_build_container( diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index d0d73dd78..aed456d6a 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -309,6 +309,7 @@ def _prepare_welcome_comment(self) -> str: * `/hold cancel` - Unblock PR merging * `/verified` - Mark PR as verified * `/verified cancel` - Remove verification status +* `/reprocess` - Trigger complete PR workflow reprocessing (useful if webhook failed or configuration changed) #### Review & Approval * `/lgtm` - Approve changes (looks good to me) @@ -1115,3 +1116,97 @@ async def skip_if_pull_request_already_merged(self, pull_request: PullRequest) - return True return False + + async def _welcome_comment_exists(self, pull_request: PullRequest) -> bool: + """Check if welcome message already exists for this PR.""" + comments = [ + comment + for comment in await asyncio.to_thread(pull_request.get_issue_comments) + if self.github_webhook.issue_url_for_welcome_msg in comment.body + ] + return len(comments) > 0 + + async def _tracking_issue_exists(self, pull_request: PullRequest) -> bool: + """Check if tracking issue already exists for this PR.""" + expected_body = self._generate_issue_body(pull_request=pull_request) + issues = [issue for issue in await asyncio.to_thread(self.repository.get_issues) if issue.body == expected_body] + return len(issues) > 0 + + async def process_new_or_reprocess_pull_request(self, pull_request: PullRequest) -> None: + """Process a new or reprocessed PR - handles welcome message, tracking issue, and full workflow. + + This method extracts the core logic from the "opened" event handler to make it reusable + for both new PRs and the /reprocess command. It includes duplicate prevention checks. + """ + self.logger.step( # type: ignore[attr-defined] + f"{self.log_prefix} {format_task_fields('pr_initialization', 'pr_management', 'started')} " + f"Starting PR initialization workflow", + ) + + tasks: list[Coroutine[Any, Any, Any]] = [] + + # Add welcome message if it doesn't exist yet + if not await self._welcome_comment_exists(pull_request=pull_request): + self.logger.info(f"{self.log_prefix} Adding welcome message to PR") + welcome_msg = self._prepare_welcome_comment() + tasks.append(asyncio.to_thread(pull_request.create_issue_comment, body=welcome_msg)) + else: + self.logger.info(f"{self.log_prefix} Welcome message already exists, skipping") + + # Add tracking issue if it doesn't exist yet + if not await self._tracking_issue_exists(pull_request=pull_request): + self.logger.info(f"{self.log_prefix} Creating tracking issue for PR") + tasks.append(self.create_issue_for_new_pull_request(pull_request=pull_request)) + else: + self.logger.info(f"{self.log_prefix} Tracking issue already exists, skipping") + + # Always run these tasks + tasks.append(self.set_wip_label_based_on_title(pull_request=pull_request)) + tasks.append(self.process_opened_or_synchronize_pull_request(pull_request=pull_request)) + + self.logger.step( # type: ignore[attr-defined] + f"{self.log_prefix} {format_task_fields('pr_initialization', 'pr_management', 'processing')} " + f"Executing initialization tasks", + ) + results = await asyncio.gather(*tasks, return_exceptions=True) + for result in results: + if isinstance(result, Exception): + self.logger.error(f"{self.log_prefix} Async task failed: {result}") + + # Set auto merge only after all initialization is done + self.logger.step( # type: ignore[attr-defined] + f"{self.log_prefix} {format_task_fields('pr_initialization', 'pr_management', 'processing')} " + f"Setting auto-merge configuration", + ) + await self.set_pull_request_automerge(pull_request=pull_request) + + self.logger.step( # type: ignore[attr-defined] + f"{self.log_prefix} {format_task_fields('pr_initialization', 'pr_management', 'completed')} " + f"PR initialization workflow completed", + ) + + async def process_command_reprocess(self, pull_request: PullRequest) -> None: + """Handle /reprocess command - triggers full PR workflow from scratch.""" + self.logger.step( # type: ignore[attr-defined] + f"{self.log_prefix} {format_task_fields('reprocess_command', 'pr_management', 'started')} " + f"Starting /reprocess command execution for PR #{pull_request.number}", + ) + + # Check if PR is already merged - skip if merged + if await asyncio.to_thread(lambda: pull_request.is_merged()): + self.logger.info(f"{self.log_prefix} PR is already merged, skipping reprocess") + self.logger.step( # type: ignore[attr-defined] + f"{self.log_prefix} {format_task_fields('reprocess_command', 'pr_management', 'completed')} " + f"/reprocess command completed (PR already merged - skipped)", + ) + return + + self.logger.info(f"{self.log_prefix} Executing full PR reprocessing workflow") + + # Call the extracted reusable method + await self.process_new_or_reprocess_pull_request(pull_request=pull_request) + + self.logger.step( # type: ignore[attr-defined] + f"{self.log_prefix} {format_task_fields('reprocess_command', 'pr_management', 'completed')} " + f"/reprocess command completed successfully", + ) diff --git a/webhook_server/tests/test_issue_comment_handler.py b/webhook_server/tests/test_issue_comment_handler.py index 35e3c420e..d6291b808 100644 --- a/webhook_server/tests/test_issue_comment_handler.py +++ b/webhook_server/tests/test_issue_comment_handler.py @@ -11,6 +11,7 @@ COMMAND_ASSIGN_REVIEWERS_STR, COMMAND_CHECK_CAN_MERGE_STR, COMMAND_CHERRY_PICK_STR, + COMMAND_REPROCESS_STR, COMMAND_RETEST_STR, HOLD_LABEL_STR, REACTIONS, @@ -758,3 +759,149 @@ async def test_process_retest_command_async_task_exception( pull_request=mock_pull_request, command_args="tox", reviewed_user="test-user" ) mock_error.assert_called_once() + + @pytest.mark.asyncio + async def test_user_commands_reprocess_command_registration( + self, issue_comment_handler: IssueCommentHandler + ) -> None: + """Test that reprocess command is in available_commands list.""" + # Verify COMMAND_REPROCESS_STR is in the available_commands list + # by checking if the command is recognized (doesn't return early for unsupported command) + mock_pull_request = Mock() + + with ( + patch.object( + issue_comment_handler.owners_file_handler, + "is_user_valid_to_run_commands", + new=AsyncMock(return_value=True), + ), + patch.object( + issue_comment_handler.pull_request_handler, "process_command_reprocess", new=AsyncMock() + ) as mock_reprocess, + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=COMMAND_REPROCESS_STR, + reviewed_user="test-user", + issue_comment_id=123, + ) + # Command should be recognized and processed + mock_reprocess.assert_awaited_once_with(pull_request=mock_pull_request) + + @pytest.mark.asyncio + async def test_user_commands_reprocess_authorized_user(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test reprocess command with authorized user (in OWNERS).""" + mock_pull_request = Mock() + + with ( + patch.object( + issue_comment_handler.owners_file_handler, + "is_user_valid_to_run_commands", + new=AsyncMock(return_value=True), + ), + patch.object( + issue_comment_handler.pull_request_handler, "process_command_reprocess", new=AsyncMock() + ) as mock_reprocess, + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()) as mock_reaction, + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=COMMAND_REPROCESS_STR, + reviewed_user="approver1", # From fixture: all_pull_request_approvers + issue_comment_id=123, + ) + # Verify user validation was called + issue_comment_handler.owners_file_handler.is_user_valid_to_run_commands.assert_awaited_once_with( + pull_request=mock_pull_request, reviewed_user="approver1" + ) + # Verify reprocess handler was called + mock_reprocess.assert_awaited_once_with(pull_request=mock_pull_request) + # Verify reaction was added + mock_reaction.assert_awaited_once_with( + pull_request=mock_pull_request, issue_comment_id=123, reaction=REACTIONS.ok + ) + + @pytest.mark.asyncio + async def test_user_commands_reprocess_unauthorized_user(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test reprocess command with unauthorized user (not in OWNERS).""" + mock_pull_request = Mock() + + with ( + patch.object( + issue_comment_handler.owners_file_handler, + "is_user_valid_to_run_commands", + new=AsyncMock(return_value=False), + ), + patch.object( + issue_comment_handler.pull_request_handler, "process_command_reprocess", new=AsyncMock() + ) as mock_reprocess, + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()) as mock_reaction, + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=COMMAND_REPROCESS_STR, + reviewed_user="unauthorized-user", + issue_comment_id=123, + ) + # Verify user validation was called + issue_comment_handler.owners_file_handler.is_user_valid_to_run_commands.assert_awaited_once_with( + pull_request=mock_pull_request, reviewed_user="unauthorized-user" + ) + # Verify reprocess handler was NOT called + mock_reprocess.assert_not_awaited() + # Reaction should still be added before permission check + mock_reaction.assert_awaited_once_with( + pull_request=mock_pull_request, issue_comment_id=123, reaction=REACTIONS.ok + ) + + @pytest.mark.asyncio + async def test_user_commands_reprocess_with_args(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test reprocess command with additional arguments (should ignore args).""" + mock_pull_request = Mock() + + with ( + patch.object( + issue_comment_handler.owners_file_handler, + "is_user_valid_to_run_commands", + new=AsyncMock(return_value=True), + ), + patch.object( + issue_comment_handler.pull_request_handler, "process_command_reprocess", new=AsyncMock() + ) as mock_reprocess, + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()), + ): + # Command with args (should be processed but args ignored) + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=f"{COMMAND_REPROCESS_STR} some-args", + reviewed_user="test-user", + issue_comment_id=123, + ) + # Verify reprocess was called (args are ignored) + mock_reprocess.assert_awaited_once_with(pull_request=mock_pull_request) + + @pytest.mark.asyncio + async def test_user_commands_reprocess_reaction_added(self, issue_comment_handler: IssueCommentHandler) -> None: + """Test that reaction is added to comment for reprocess command.""" + mock_pull_request = Mock() + + with ( + patch.object( + issue_comment_handler.owners_file_handler, + "is_user_valid_to_run_commands", + new=AsyncMock(return_value=True), + ), + patch.object(issue_comment_handler.pull_request_handler, "process_command_reprocess", new=AsyncMock()), + patch.object(issue_comment_handler, "create_comment_reaction", new=AsyncMock()) as mock_reaction, + ): + await issue_comment_handler.user_commands( + pull_request=mock_pull_request, + command=COMMAND_REPROCESS_STR, + reviewed_user="test-user", + issue_comment_id=456, + ) + # Verify reaction was added with correct comment ID and reaction type + mock_reaction.assert_awaited_once_with( + pull_request=mock_pull_request, issue_comment_id=456, reaction=REACTIONS.ok + ) diff --git a/webhook_server/tests/test_pull_request_handler.py b/webhook_server/tests/test_pull_request_handler.py index 9d3edb100..fabb7b5d1 100644 --- a/webhook_server/tests/test_pull_request_handler.py +++ b/webhook_server/tests/test_pull_request_handler.py @@ -1087,3 +1087,286 @@ async def test_close_issue_for_merged_or_closed_pr_without_issue( pull_request=mock_pull_request, hook_action="closed" ) # Should not find any matching issues + + # /reprocess command tests + + @pytest.mark.asyncio + async def test_process_command_reprocess_merged_pr( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test /reprocess command on merged PR - should reject and skip.""" + # Mock is_merged to return True + with ( + patch.object(mock_pull_request, "is_merged", new=Mock(return_value=True)), + patch.object( + pull_request_handler, "process_new_or_reprocess_pull_request", new=AsyncMock() + ) as mock_process_new, + ): + await pull_request_handler.process_command_reprocess(pull_request=mock_pull_request) + + # Verify is_merged was checked + mock_pull_request.is_merged.assert_called_once() + + # Verify workflow was NOT executed + mock_process_new.assert_not_awaited() + + @pytest.mark.asyncio + async def test_process_command_reprocess_open_pr_success( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test /reprocess command on open PR - should trigger full workflow.""" + # Mock is_merged to return False + with ( + patch.object(mock_pull_request, "is_merged", new=Mock(return_value=False)), + patch.object( + pull_request_handler, "process_new_or_reprocess_pull_request", new=AsyncMock() + ) as mock_process_new, + ): + await pull_request_handler.process_command_reprocess(pull_request=mock_pull_request) + + # Verify is_merged was checked + mock_pull_request.is_merged.assert_called_once() + + # Verify workflow was executed + mock_process_new.assert_awaited_once_with(pull_request=mock_pull_request) + + @pytest.mark.asyncio + async def test_welcome_comment_exists_true( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test _welcome_comment_exists returns True when welcome message exists.""" + mock_comment = Mock() + mock_comment.body = f"Some text {pull_request_handler.github_webhook.issue_url_for_welcome_msg} more text" + + with patch.object(mock_pull_request, "get_issue_comments", return_value=[mock_comment]): + result = await pull_request_handler._welcome_comment_exists(pull_request=mock_pull_request) + assert result is True + + @pytest.mark.asyncio + async def test_welcome_comment_exists_false( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test _welcome_comment_exists returns False when no welcome message.""" + mock_comment = Mock() + mock_comment.body = "Regular comment without welcome URL" + + with patch.object(mock_pull_request, "get_issue_comments", return_value=[mock_comment]): + result = await pull_request_handler._welcome_comment_exists(pull_request=mock_pull_request) + assert result is False + + @pytest.mark.asyncio + async def test_welcome_comment_exists_empty_comments( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test _welcome_comment_exists returns False when no comments.""" + with patch.object(mock_pull_request, "get_issue_comments", return_value=[]): + result = await pull_request_handler._welcome_comment_exists(pull_request=mock_pull_request) + assert result is False + + @pytest.mark.asyncio + async def test_tracking_issue_exists_true( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test _tracking_issue_exists returns True when tracking issue exists.""" + mock_pull_request.number = 123 + expected_body = pull_request_handler._generate_issue_body(pull_request=mock_pull_request) + + mock_issue = Mock() + mock_issue.body = expected_body + + with patch.object(pull_request_handler.repository, "get_issues", return_value=[mock_issue]): + result = await pull_request_handler._tracking_issue_exists(pull_request=mock_pull_request) + assert result is True + + @pytest.mark.asyncio + async def test_tracking_issue_exists_false( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test _tracking_issue_exists returns False when no tracking issue.""" + mock_issue = Mock() + mock_issue.body = "Some other issue body" + + with patch.object(pull_request_handler.repository, "get_issues", return_value=[mock_issue]): + result = await pull_request_handler._tracking_issue_exists(pull_request=mock_pull_request) + assert result is False + + @pytest.mark.asyncio + async def test_tracking_issue_exists_empty_issues( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test _tracking_issue_exists returns False when no issues.""" + with patch.object(pull_request_handler.repository, "get_issues", return_value=[]): + result = await pull_request_handler._tracking_issue_exists(pull_request=mock_pull_request) + assert result is False + + @pytest.mark.asyncio + async def test_process_new_or_reprocess_pull_request_full_workflow( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test process_new_or_reprocess_pull_request - full workflow without duplicates.""" + # Mock welcome message and tracking issue don't exist + with ( + patch.object( + pull_request_handler, "_welcome_comment_exists", new=AsyncMock(return_value=False) + ) as mock_welcome_check, + patch.object( + pull_request_handler, "_tracking_issue_exists", new=AsyncMock(return_value=False) + ) as mock_issue_check, + patch.object(mock_pull_request, "create_issue_comment", new=Mock()) as mock_comment, + patch.object( + pull_request_handler, "create_issue_for_new_pull_request", new=AsyncMock() + ) as mock_create_issue, + patch.object(pull_request_handler, "set_wip_label_based_on_title", new=AsyncMock()) as mock_wip, + patch.object( + pull_request_handler, "process_opened_or_synchronize_pull_request", new=AsyncMock() + ) as mock_process, + patch.object(pull_request_handler, "set_pull_request_automerge", new=AsyncMock()) as mock_automerge, + ): + await pull_request_handler.process_new_or_reprocess_pull_request(pull_request=mock_pull_request) + + # Verify duplicate checks were called + mock_welcome_check.assert_awaited_once_with(pull_request=mock_pull_request) + mock_issue_check.assert_awaited_once_with(pull_request=mock_pull_request) + + # Verify welcome message was created with the correct marker + mock_comment.assert_called_once() + assert pull_request_handler.github_webhook.issue_url_for_welcome_msg in mock_comment.call_args[1]["body"] + + # Verify tracking issue was created + mock_create_issue.assert_awaited_once_with(pull_request=mock_pull_request) + + # Verify other tasks were executed + mock_wip.assert_awaited_once_with(pull_request=mock_pull_request) + mock_process.assert_awaited_once_with(pull_request=mock_pull_request) + mock_automerge.assert_awaited_once_with(pull_request=mock_pull_request) + + @pytest.mark.asyncio + async def test_process_new_or_reprocess_pull_request_skip_welcome_duplicate( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test process_new_or_reprocess_pull_request - skip welcome if already exists.""" + # Mock welcome message exists, tracking issue doesn't + with ( + patch.object( + pull_request_handler, "_welcome_comment_exists", new=AsyncMock(return_value=True) + ) as mock_welcome_check, + patch.object(pull_request_handler, "_tracking_issue_exists", new=AsyncMock(return_value=False)), + patch.object(mock_pull_request, "create_issue_comment", new=Mock()) as mock_comment, + patch.object(pull_request_handler, "create_issue_for_new_pull_request", new=AsyncMock()), + patch.object(pull_request_handler, "set_wip_label_based_on_title", new=AsyncMock()), + patch.object(pull_request_handler, "process_opened_or_synchronize_pull_request", new=AsyncMock()), + patch.object(pull_request_handler, "set_pull_request_automerge", new=AsyncMock()), + ): + await pull_request_handler.process_new_or_reprocess_pull_request(pull_request=mock_pull_request) + + # Verify welcome check was called + mock_welcome_check.assert_awaited_once_with(pull_request=mock_pull_request) + + # Verify welcome message was NOT created (already exists) + mock_comment.assert_not_called() + + @pytest.mark.asyncio + async def test_process_new_or_reprocess_pull_request_skip_issue_duplicate( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test process_new_or_reprocess_pull_request - skip tracking issue if already exists.""" + # Mock welcome doesn't exist, tracking issue exists + with ( + patch.object(pull_request_handler, "_welcome_comment_exists", new=AsyncMock(return_value=False)), + patch.object( + pull_request_handler, "_tracking_issue_exists", new=AsyncMock(return_value=True) + ) as mock_issue_check, + patch.object(mock_pull_request, "create_issue_comment", new=Mock()), + patch.object( + pull_request_handler, "create_issue_for_new_pull_request", new=AsyncMock() + ) as mock_create_issue, + patch.object(pull_request_handler, "set_wip_label_based_on_title", new=AsyncMock()), + patch.object(pull_request_handler, "process_opened_or_synchronize_pull_request", new=AsyncMock()), + patch.object(pull_request_handler, "set_pull_request_automerge", new=AsyncMock()), + ): + await pull_request_handler.process_new_or_reprocess_pull_request(pull_request=mock_pull_request) + + # Verify issue check was called + mock_issue_check.assert_awaited_once_with(pull_request=mock_pull_request) + + # Verify tracking issue was NOT created (already exists) + mock_create_issue.assert_not_awaited() + + @pytest.mark.asyncio + async def test_process_new_or_reprocess_pull_request_skip_both_duplicates( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test process_new_or_reprocess_pull_request - skip both welcome and issue if exist.""" + # Mock both already exist + with ( + patch.object(pull_request_handler, "_welcome_comment_exists", new=AsyncMock(return_value=True)), + patch.object(pull_request_handler, "_tracking_issue_exists", new=AsyncMock(return_value=True)), + patch.object(mock_pull_request, "create_issue_comment", new=Mock()) as mock_comment, + patch.object( + pull_request_handler, "create_issue_for_new_pull_request", new=AsyncMock() + ) as mock_create_issue, + patch.object(pull_request_handler, "set_wip_label_based_on_title", new=AsyncMock()) as mock_wip, + patch.object( + pull_request_handler, "process_opened_or_synchronize_pull_request", new=AsyncMock() + ) as mock_process, + patch.object(pull_request_handler, "set_pull_request_automerge", new=AsyncMock()), + ): + await pull_request_handler.process_new_or_reprocess_pull_request(pull_request=mock_pull_request) + + # Verify neither welcome nor issue were created + mock_comment.assert_not_called() + mock_create_issue.assert_not_awaited() + + # Verify workflow tasks still executed + mock_wip.assert_awaited_once() + mock_process.assert_awaited_once() + + @pytest.mark.asyncio + async def test_process_new_or_reprocess_pull_request_parallel_execution( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test process_new_or_reprocess_pull_request executes tasks in parallel.""" + # Mock nothing exists - full workflow + with ( + patch.object(pull_request_handler, "_welcome_comment_exists", new=AsyncMock(return_value=False)), + patch.object(pull_request_handler, "_tracking_issue_exists", new=AsyncMock(return_value=False)), + patch.object(mock_pull_request, "create_issue_comment", new=Mock()), + patch.object(pull_request_handler, "create_issue_for_new_pull_request", new=AsyncMock()), + patch.object(pull_request_handler, "set_wip_label_based_on_title", new=AsyncMock()), + patch.object(pull_request_handler, "process_opened_or_synchronize_pull_request", new=AsyncMock()), + patch.object(pull_request_handler, "set_pull_request_automerge", new=AsyncMock()), + patch("asyncio.gather", new=AsyncMock(return_value=[])) as mock_gather, + ): + await pull_request_handler.process_new_or_reprocess_pull_request(pull_request=mock_pull_request) + + # Verify asyncio.gather was called (parallel execution) + mock_gather.assert_awaited_once() + + @pytest.mark.asyncio + async def test_process_new_or_reprocess_pull_request_exception_handling( + self, pull_request_handler: PullRequestHandler, mock_pull_request: Mock + ) -> None: + """Test process_new_or_reprocess_pull_request handles exceptions gracefully.""" + # Mock one task fails + with ( + patch.object(pull_request_handler, "_welcome_comment_exists", new=AsyncMock(return_value=False)), + patch.object(pull_request_handler, "_tracking_issue_exists", new=AsyncMock(return_value=False)), + patch.object(mock_pull_request, "create_issue_comment", new=Mock()), + patch.object( + pull_request_handler, + "create_issue_for_new_pull_request", + new=AsyncMock(side_effect=Exception("Test error")), + ), + patch.object(pull_request_handler, "set_wip_label_based_on_title", new=AsyncMock()), + patch.object(pull_request_handler, "process_opened_or_synchronize_pull_request", new=AsyncMock()), + patch.object(pull_request_handler, "set_pull_request_automerge", new=AsyncMock()) as mock_automerge, + patch.object(pull_request_handler.logger, "error") as mock_logger_error, + ): + # Should not raise exception - errors are caught and logged + await pull_request_handler.process_new_or_reprocess_pull_request(pull_request=mock_pull_request) + + # Verify error was logged + mock_logger_error.assert_called() + + # Verify automerge still executed (after errors) + mock_automerge.assert_awaited_once() diff --git a/webhook_server/utils/constants.py b/webhook_server/utils/constants.py index 78ee56dbd..17ca4a00d 100644 --- a/webhook_server/utils/constants.py +++ b/webhook_server/utils/constants.py @@ -30,6 +30,7 @@ HOLD_LABEL_STR: str = "hold" SIZE_LABEL_PREFIX: str = "size/" COMMAND_RETEST_STR: str = "retest" +COMMAND_REPROCESS_STR: str = "reprocess" COMMAND_CHERRY_PICK_STR: str = "cherry-pick" COMMAND_ASSIGN_REVIEWERS_STR: str = "assign-reviewers" COMMAND_CHECK_CAN_MERGE_STR: str = "check-can-merge" From 69b5284a6cfc2e2bf07deb6e528621200016cd22 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Mon, 10 Nov 2025 13:55:47 +0200 Subject: [PATCH 2/2] fix: Move GitHub API iterations inside asyncio.to_thread to prevent blocking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixed blocking GitHub API calls in _welcome_comment_exists and _tracking_issue_exists methods by moving the entire fetch-and-filter operation inside background threads: - _welcome_comment_exists: Moved comment iteration inside nested function - _tracking_issue_exists: Moved issue iteration inside nested function This prevents synchronous HTTP calls from blocking the event loop when iterating over PaginatedList objects. Addresses CodeRabbit review comment about keeping GitHub API calls off the event loop. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../libs/handlers/pull_request_handler.py | 21 ++++++++++++------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index aed456d6a..d2dc9421b 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -1119,18 +1119,23 @@ async def skip_if_pull_request_already_merged(self, pull_request: PullRequest) - async def _welcome_comment_exists(self, pull_request: PullRequest) -> bool: """Check if welcome message already exists for this PR.""" - comments = [ - comment - for comment in await asyncio.to_thread(pull_request.get_issue_comments) - if self.github_webhook.issue_url_for_welcome_msg in comment.body - ] - return len(comments) > 0 + + def check_comments() -> bool: + return any( + self.github_webhook.issue_url_for_welcome_msg in comment.body + for comment in pull_request.get_issue_comments() + ) + + return await asyncio.to_thread(check_comments) async def _tracking_issue_exists(self, pull_request: PullRequest) -> bool: """Check if tracking issue already exists for this PR.""" expected_body = self._generate_issue_body(pull_request=pull_request) - issues = [issue for issue in await asyncio.to_thread(self.repository.get_issues) if issue.body == expected_body] - return len(issues) > 0 + + def check_issues() -> bool: + return any(issue.body == expected_body for issue in self.repository.get_issues()) + + return await asyncio.to_thread(check_issues) async def process_new_or_reprocess_pull_request(self, pull_request: PullRequest) -> None: """Process a new or reprocessed PR - handles welcome message, tracking issue, and full workflow.