From 4338ce4283d32bb1842f84252eaea655cf29c3aa Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Wed, 18 Mar 2026 12:59:21 +0200 Subject: [PATCH 1/2] fix: handle cherry-pick edge cases after AI conflict resolution - Check CHERRY_PICK_HEAD before running --continue to handle auto-completed cherry-picks (e.g. modify/delete conflicts) - Handle empty cherry-pick result with --allow-empty commit - Add --label flags to gh pr create to prevent auto-verify/auto-merge race condition on AI-resolved cherry-pick PRs - Add ai-resolved-conflicts guard in set_pull_request_automerge() with active disable_automerge when already enabled - Return False for unexpected rev-parse errors instead of proceeding Closes #1043 Closes #1044 --- .../libs/handlers/pull_request_handler.py | 17 +++ .../libs/handlers/runner_handler.py | 28 ++++- webhook_server/tests/test_runner_handler.py | 106 +++++++++++++++--- 3 files changed, 134 insertions(+), 17 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index 9de0fe37..d2fe1bf1 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -905,6 +905,23 @@ async def set_pull_request_automerge(self, pull_request: PullRequest) -> None: self.logger.debug(f"{self.log_prefix} auto_merge: {auto_merge}, branch: {pull_request.base.ref}") if auto_merge: + # AI-resolved cherry-picks should NEVER be auto-merged + labels = await asyncio.to_thread(lambda: list(pull_request.labels)) + if any(label.name == AI_RESOLVED_CONFLICTS_LABEL for label in labels): + if pull_request.raw_data.get("auto_merge"): + try: + self.logger.info( + f"{self.log_prefix} AI-resolved cherry-pick has auto-merge enabled, disabling it" + ) + await asyncio.to_thread(pull_request.disable_automerge) + except Exception as exp: + self.logger.error( + f"{self.log_prefix} Failed to disable auto-merge for AI-resolved cherry-pick: {exp}" + ) + else: + self.logger.info(f"{self.log_prefix} AI-resolved cherry-pick detected, skipping auto-merge") + return + try: if not pull_request.raw_data.get("auto_merge"): self.logger.info( diff --git a/webhook_server/libs/handlers/runner_handler.py b/webhook_server/libs/handlers/runner_handler.py index 64859345..45781b79 100644 --- a/webhook_server/libs/handlers/runner_handler.py +++ b/webhook_server/libs/handlers/runner_handler.py @@ -786,7 +786,7 @@ async def _resolve_cherry_pick_with_ai( # Check if cherry-pick is still in progress (it may have auto-completed # after staging resolved files, e.g. for modify/delete conflicts) - rc_check, _, _ = await run_command( + rc_check, _, err_check = await run_command( command=f"{git_cmd} rev-parse --verify CHERRY_PICK_HEAD", log_prefix=self.log_prefix, redact_secrets=[github_token], @@ -801,9 +801,26 @@ async def _resolve_cherry_pick_with_ai( mask_sensitive=self.github_webhook.mask_sensitive, ) if not rc: - self.logger.error(f"{self.log_prefix} cherry-pick --continue failed after AI resolution: {err}") - return False + if "cherry-pick is now empty" in err: + self.logger.info( + f"{self.log_prefix} Cherry-pick is empty after AI resolution, committing with --allow-empty" + ) + rc_empty, _, err_empty = await run_command( + command=f"{git_cmd} -c core.editor=true commit --allow-empty -C CHERRY_PICK_HEAD", + log_prefix=self.log_prefix, + redact_secrets=[github_token], + mask_sensitive=self.github_webhook.mask_sensitive, + ) + if not rc_empty: + self.logger.error(f"{self.log_prefix} Failed to commit empty cherry-pick: {err_empty}") + return False + else: + self.logger.error(f"{self.log_prefix} cherry-pick --continue failed after AI resolution: {err}") + return False else: + if err_check and "needed a single revision" not in err_check.lower(): + self.logger.error(f"{self.log_prefix} Unexpected CHERRY_PICK_HEAD check error: {err_check}") + return False self.logger.info(f"{self.log_prefix} Cherry-pick already completed after staging resolved files") self.logger.info(f"{self.log_prefix} AI successfully resolved cherry-pick conflicts") @@ -988,11 +1005,16 @@ async def cherry_pick( cherry_picked_label = f"{CHERRY_PICKED_LABEL}-from-{source_branch}"[:49] + label_flags = f" --label {shlex.quote(cherry_picked_label)}" + if cherry_pick_had_conflicts: + label_flags += f" --label {shlex.quote(AI_RESOLVED_CONFLICTS_LABEL)}" + gh_pr_command = ( f"gh pr create --repo {shlex.quote(repo_full_name)}" f" --base {shlex.quote(target_branch)}" f" --head {shlex.quote(new_branch_name)}" f"{assignee_flag}" + f"{label_flags}" f" --title {shlex.quote(pr_title)}" f" --body {shlex.quote(pr_body)}" ) diff --git a/webhook_server/tests/test_runner_handler.py b/webhook_server/tests/test_runner_handler.py index aaaf403f..56e2e6aa 100644 --- a/webhook_server/tests/test_runner_handler.py +++ b/webhook_server/tests/test_runner_handler.py @@ -1494,7 +1494,7 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st with patch( "asyncio.to_thread", new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), - ) as mock_to_thread: + ): with patch( "webhook_server.libs.handlers.runner_handler.get_repository_github_app_token", return_value=None, @@ -1514,19 +1514,9 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st ] assert gh_cmd_calls, "gh pr create not called" gh_cmd_str = str(gh_cmd_calls[-1]) - assert "--label" not in gh_cmd_str, ( - "Labels should not be in gh pr create command" - ) - # Verify labels were added via PyGithub add_to_labels - add_labels_calls = [ - c - for c in mock_to_thread.call_args_list - if len(c.args) >= 1 and "add_to_labels" in str(c.args[0]) - ] - assert add_labels_calls, "add_to_labels not called via asyncio.to_thread" - labels_call_str = str(add_labels_calls[-1]) - assert "ai-resolved-conflicts" in labels_call_str - assert "CherryPicked-from-main" in labels_call_str + assert "--label" in gh_cmd_str, "Labels should be in gh pr create command" + assert "ai-resolved-conflicts" in gh_cmd_str + assert "CherryPicked-from-main" in gh_cmd_str @pytest.mark.asyncio async def test_cherry_pick_ai_resolves_modify_delete_conflict( @@ -1599,6 +1589,94 @@ async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, st "Cherry-pick conflicts were resolved by AI" in str(c) for c in comment_calls ) assert ai_comment, f"Expected AI comment, got: {comment_calls}" + # Verify labels are in gh pr create command + gh_cmd_calls = [ + c for c in mock_run_cmd.call_args_list if "gh pr create" in str(c) + ] + assert gh_cmd_calls, "gh pr create not called" + gh_cmd_str = str(gh_cmd_calls[-1]) + assert "--label" in gh_cmd_str + assert "ai-resolved-conflicts" in gh_cmd_str + + @pytest.mark.asyncio + async def test_cherry_pick_ai_resolves_empty_cherry_pick( + self, runner_handler: RunnerHandler, mock_pull_request: Mock + ) -> None: + """Cherry-pick conflict resolved by AI results in empty commit — committed with --allow-empty.""" + runner_handler.github_webhook.ai_features = { + "ai-provider": "claude", + "ai-model": "sonnet", + "resolve-cherry-pick-conflicts-with-ai": {"enabled": True, "timeout-minutes": 10}, + } + + async def run_command_side_effect(command: str, **kwargs: Any) -> tuple[bool, str, str]: + # Fail on cherry-pick (conflict) + if "cherry-pick" in command and "--continue" not in command and "rev-parse" not in command: + return (False, "", "CONFLICT (modify/delete): file.sh deleted in HEAD and modified in abc1234") + # CHERRY_PICK_HEAD exists — cherry-pick still in progress + if "rev-parse --verify CHERRY_PICK_HEAD" in command: + return (True, "abc123", "") + # cherry-pick --continue fails with empty cherry-pick + if "cherry-pick --continue" in command: + return ( + False, + "", + "The previous cherry-pick is now empty, possibly due to conflict resolution.\n" + "If you wish to commit it anyway, use:\n\n git commit --allow-empty\n", + ) + if "gh pr create" in command: + return (True, "https://github.com/test-org/test-repo/pull/99", "") + return (True, "success", "") + + with patch.object(runner_handler, "is_branch_exists", new=AsyncMock(return_value=Mock())): + with patch.object(runner_handler.check_run_handler, "set_check_in_progress"): + with patch.object(runner_handler.check_run_handler, "set_check_success") as mock_set_success: + with patch.object(runner_handler, "_checkout_worktree") as mock_checkout: + mock_checkout.return_value = AsyncMock() + mock_checkout.return_value.__aenter__ = AsyncMock( + return_value=(True, "/tmp/worktree-path", "", "") + ) + mock_checkout.return_value.__aexit__ = AsyncMock(return_value=None) + with patch( + "webhook_server.libs.handlers.runner_handler.run_command", + new=AsyncMock(side_effect=run_command_side_effect), + ) as mock_run_cmd: + with patch( + "webhook_server.libs.handlers.runner_handler.call_ai_cli", + new=AsyncMock(return_value=(True, "resolved")), + ) as mock_ai_cli: + with patch( + "asyncio.to_thread", + new=AsyncMock(side_effect=lambda fn, *a, **kw: fn(*a, **kw) if a or kw else fn()), + ): + with patch( + "webhook_server.libs.handlers.runner_handler.get_repository_github_app_token", + return_value=None, + ): + await runner_handler.cherry_pick(mock_pull_request, "main") + mock_set_success.assert_called_once() + mock_ai_cli.assert_called_once() + # Verify --allow-empty commit was called + allow_empty_calls = [ + c for c in mock_run_cmd.call_args_list if "commit --allow-empty" in str(c) + ] + assert allow_empty_calls, ( + "git commit --allow-empty should be called for empty cherry-pick" + ) + # Verify AI comment was posted + comment_calls = mock_pull_request.create_issue_comment.call_args_list + ai_comment = any( + "Cherry-pick conflicts were resolved by AI" in str(c) for c in comment_calls + ) + assert ai_comment, f"Expected AI comment, got: {comment_calls}" + # Verify labels are in gh pr create command + gh_cmd_calls = [ + c for c in mock_run_cmd.call_args_list if "gh pr create" in str(c) + ] + assert gh_cmd_calls, "gh pr create not called" + gh_cmd_str = str(gh_cmd_calls[-1]) + assert "--label" in gh_cmd_str + assert "ai-resolved-conflicts" in gh_cmd_str @pytest.mark.asyncio async def test_cherry_pick_ai_fails_fallback(self, runner_handler: RunnerHandler, mock_pull_request: Mock) -> None: From cab38fd0a30161be256958b0d7677fa2c082efb4 Mon Sep 17 00:00:00 2001 From: Meni Yakove Date: Wed, 18 Mar 2026 13:12:17 +0200 Subject: [PATCH 2/2] fix: use logger.exception for disable_automerge error handling Per CLAUDE.md convention, use logger.exception() instead of logger.error() to preserve full traceback in logs. --- webhook_server/libs/handlers/pull_request_handler.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/webhook_server/libs/handlers/pull_request_handler.py b/webhook_server/libs/handlers/pull_request_handler.py index d2fe1bf1..47e2e2e7 100644 --- a/webhook_server/libs/handlers/pull_request_handler.py +++ b/webhook_server/libs/handlers/pull_request_handler.py @@ -914,9 +914,9 @@ async def set_pull_request_automerge(self, pull_request: PullRequest) -> None: f"{self.log_prefix} AI-resolved cherry-pick has auto-merge enabled, disabling it" ) await asyncio.to_thread(pull_request.disable_automerge) - except Exception as exp: - self.logger.error( - f"{self.log_prefix} Failed to disable auto-merge for AI-resolved cherry-pick: {exp}" + except Exception: + self.logger.exception( + f"{self.log_prefix} Failed to disable auto-merge for AI-resolved cherry-pick" ) else: self.logger.info(f"{self.log_prefix} AI-resolved cherry-pick detected, skipping auto-merge")