Skip to content

fix: prevent duplicate cherry-pick PRs via label check #1055

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
myakove merged 1 commit into from
Mar 24, 2026
Merged

fix: prevent duplicate cherry-pick PRs via label check #1055

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 59 additions & 20 deletions webhook_server/libs/handlers/issue_comment_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,7 @@ async def process_cherry_pick_command(
"""Process cherry-pick command for pull requests.

This method handles cherry-pick requests for both unmerged and merged PRs.
Cherry-pick labels (cherry-pick/<branch-name>) are added in BOTH scenarios:
Cherry-pick labels (cherry-pick-<branch-name>) are added in BOTH scenarios:

**Unmerged PRs:**
- Labels indicate which branches need cherry-picking after the PR is merged
Expand All @@ -411,12 +411,17 @@ async def process_cherry_pick_command(

Example:
# Unmerged PR: /cherry-pick v1.0 v2.0
# - Adds labels: cherry-pick/v1.0, cherry-pick/v2.0
# - Adds labels: cherry-pick-v1.0, cherry-pick-v2.0
# - Posts comment explaining labels will trigger auto cherry-pick on merge

# Merged PR: /cherry-pick v1.0 v2.0
# - Adds labels: cherry-pick-v1.0, cherry-pick-v2.0 as idempotency markers
# - Executes cherry-pick to v1.0 and v2.0 immediately
# - Adds labels: cherry-pick/v1.0, cherry-pick/v2.0 to track completion
# - Remove the label to retry a failed cherry-pick

Duplicate prevention:
If a cherry-pick label already exists for a target branch, that branch is
skipped. To re-trigger a cherry-pick, remove the label and run the command again.
"""
_target_branches: list[str] = command_args.split()
_exits_target_branches: set[str] = set()
Expand All @@ -438,29 +443,63 @@ async def process_cherry_pick_command(
self.logger.info(f"{self.log_prefix} {_non_exits_target_branches_msg}")
await asyncio.to_thread(pull_request.create_issue_comment, _non_exits_target_branches_msg)

cp_labels: list[str] = [
f"{CHERRY_PICK_LABEL_PREFIX}{_target_branch}" for _target_branch in _exits_target_branches
]
if not _exits_target_branches:
return

# Filter out branches that already have cherry-pick labels
existing_labels = {label.name for label in await asyncio.to_thread(lambda: list(pull_request.labels))}
_already_cherry_picked: list[str] = []
_branches_to_process: set[str] = set()

for _branch in _exits_target_branches:
label_name = f"{CHERRY_PICK_LABEL_PREFIX}{_branch}"
if label_name in existing_labels:
_already_cherry_picked.append(_branch)
else:
_branches_to_process.add(_branch)
Comment thread
myakove marked this conversation as resolved.

if _already_cherry_picked:
already_msg = ", ".join(f"`{b}`" for b in _already_cherry_picked)
await asyncio.to_thread(
pull_request.create_issue_comment,
f"Cherry-pick label already present for: {already_msg}\n"
"To re-trigger, remove the cherry-pick label(s) and run the command again.",
)

if not _branches_to_process:
return

if _exits_target_branches:
if not self.hook_data["issue"].get("pull_request", {}).get("merged_at"):
info_msg: str = f"""
cp_labels: list[str] = [f"{CHERRY_PICK_LABEL_PREFIX}{_branch}" for _branch in _branches_to_process]

if not self.hook_data["issue"].get("pull_request", {}).get("merged_at"):
info_msg: str = f"""
Cherry-pick requested for PR: `{pull_request.title}` by user `{reviewed_user}`
Adding label/s `{" ".join([_cp_label for _cp_label in cp_labels])}` for automatic cheery-pick once the PR is merged
Adding label/s `{" ".join(cp_labels)}` for automatic cherry-pick once the PR is merged
"""

self.logger.info(f"{self.log_prefix} {info_msg}")
await asyncio.to_thread(pull_request.create_issue_comment, info_msg)
else:
for _exits_target_branch in _exits_target_branches:
await self.runner_handler.cherry_pick(
pull_request=pull_request,
target_branch=_exits_target_branch,
assign_to_pr_owner=self.github_webhook.cherry_pick_assign_to_pr_author,
self.logger.info(f"{self.log_prefix} {info_msg}")
await asyncio.to_thread(pull_request.create_issue_comment, info_msg)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
else:
for _branch in _branches_to_process:
label_added = await self.labels_handler._add_label(
pull_request=pull_request,
label=f"{CHERRY_PICK_LABEL_PREFIX}{_branch}",
)
if not label_added:
self.logger.info(
f"{self.log_prefix} Skipping cherry-pick for {_branch},"
" label already exists or could not be added"
)
continue
await self.runner_handler.cherry_pick(
pull_request=pull_request,
target_branch=_branch,
assign_to_pr_owner=self.github_webhook.cherry_pick_assign_to_pr_author,
)
return

for _cp_label in cp_labels:
await self.labels_handler._add_label(pull_request=pull_request, label=_cp_label)
for _cp_label in cp_labels:
await self.labels_handler._add_label(pull_request=pull_request, label=_cp_label)
Comment thread
myakove marked this conversation as resolved.

async def process_retest_command(
self, pull_request: PullRequest, command_args: str, reviewed_user: str, automerge: bool = False
Expand Down
198 changes: 194 additions & 4 deletions webhook_server/tests/test_issue_comment_handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from webhook_server.utils.constants import (
APPROVE_STR,
BUILD_AND_PUSH_CONTAINER_STR,
CHERRY_PICK_LABEL_PREFIX,
COMMAND_ASSIGN_REVIEWER_STR,
COMMAND_ASSIGN_REVIEWERS_STR,
COMMAND_CHECK_CAN_MERGE_STR,
Expand Down Expand Up @@ -794,6 +795,7 @@ async def test_process_cherry_pick_command_existing_branches(
"""Test processing cherry pick command with existing branches."""
mock_pull_request = Mock()
mock_pull_request.title = "Test PR"
mock_pull_request.labels = []
# Patch is_merged as a method
with patch.object(mock_pull_request, "is_merged", new=Mock(return_value=False)):
with patch.object(issue_comment_handler.repository, "get_branch") as mock_get_branch:
Expand All @@ -820,6 +822,7 @@ async def test_process_cherry_pick_command_non_existing_branches(
) -> None:
"""Test processing cherry pick command with non-existing branches."""
mock_pull_request = Mock()
mock_pull_request.labels = []

with patch.object(issue_comment_handler.repository, "get_branch", side_effect=Exception("Branch not found")):
with patch.object(mock_pull_request, "create_issue_comment") as mock_comment:
Expand All @@ -834,6 +837,7 @@ async def test_process_cherry_pick_command_non_existing_branches(
async def test_process_cherry_pick_command_merged_pr(self, issue_comment_handler: IssueCommentHandler) -> None:
"""Test processing cherry pick command for merged PR."""
mock_pull_request = Mock()
mock_pull_request.labels = []
# Set merged_at in hook_data to simulate a merged PR at comment time
issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2026-01-01T00:00:00Z"}
with patch.object(issue_comment_handler.repository, "get_branch"):
Expand All @@ -846,6 +850,7 @@ async def test_process_cherry_pick_command_merged_pr(self, issue_comment_handler
issue_comment_handler.labels_handler,
"_add_label",
new_callable=AsyncMock,
return_value=True,
) as mock_add_label:
await issue_comment_handler.process_cherry_pick_command(
pull_request=mock_pull_request,
Expand All @@ -859,7 +864,7 @@ async def test_process_cherry_pick_command_merged_pr(self, issue_comment_handler
)
mock_add_label.assert_called_once_with(
pull_request=mock_pull_request,
label="cherry-pick-branch1",
label=f"{CHERRY_PICK_LABEL_PREFIX}branch1",
)

@pytest.mark.asyncio
Expand All @@ -875,6 +880,7 @@ async def test_process_cherry_pick_command_merged_pr_multiple_branches(
"""
mock_pull_request = Mock()
mock_pull_request.title = "Test PR"
mock_pull_request.labels = []

# Set merged_at in hook_data to simulate a merged PR at comment time
issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2026-01-01T00:00:00Z"}
Expand All @@ -888,6 +894,7 @@ async def test_process_cherry_pick_command_merged_pr_multiple_branches(
issue_comment_handler.labels_handler,
"_add_label",
new_callable=AsyncMock,
return_value=True,
) as mock_add_label:
# Execute cherry-pick command with multiple branches
await issue_comment_handler.process_cherry_pick_command(
Expand Down Expand Up @@ -916,9 +923,15 @@ async def test_process_cherry_pick_command_merged_pr_multiple_branches(

# Verify labels were added exactly once for each branch (not duplicated)
assert mock_add_label.call_count == 3
mock_add_label.assert_any_call(pull_request=mock_pull_request, label="cherry-pick-branch1")
mock_add_label.assert_any_call(pull_request=mock_pull_request, label="cherry-pick-branch2")
mock_add_label.assert_any_call(pull_request=mock_pull_request, label="cherry-pick-branch3")
mock_add_label.assert_any_call(
pull_request=mock_pull_request, label=f"{CHERRY_PICK_LABEL_PREFIX}branch1"
)
mock_add_label.assert_any_call(
pull_request=mock_pull_request, label=f"{CHERRY_PICK_LABEL_PREFIX}branch2"
)
mock_add_label.assert_any_call(
pull_request=mock_pull_request, label=f"{CHERRY_PICK_LABEL_PREFIX}branch3"
)

@pytest.mark.asyncio
async def test_process_cherry_pick_command_merged_pr_assign_disabled(
Expand All @@ -927,6 +940,7 @@ async def test_process_cherry_pick_command_merged_pr_assign_disabled(
"""Test cherry-pick on merged PR passes assign_to_pr_owner=False when config disabled."""
issue_comment_handler.github_webhook.cherry_pick_assign_to_pr_author = False
mock_pull_request = Mock()
mock_pull_request.labels = []
# Set merged_at in hook_data to simulate a merged PR at comment time
issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2026-01-01T00:00:00Z"}
with patch.object(issue_comment_handler.repository, "get_branch"):
Expand All @@ -939,6 +953,7 @@ async def test_process_cherry_pick_command_merged_pr_assign_disabled(
issue_comment_handler.labels_handler,
"_add_label",
new_callable=AsyncMock,
return_value=True,
):
await issue_comment_handler.process_cherry_pick_command(
pull_request=mock_pull_request,
Expand All @@ -951,6 +966,181 @@ async def test_process_cherry_pick_command_merged_pr_assign_disabled(
assign_to_pr_owner=False,
)

@pytest.mark.asyncio
async def test_process_cherry_pick_command_skips_already_cherry_picked(
self, issue_comment_handler: IssueCommentHandler
) -> None:
"""Test cherry-pick skips branches whose cherry-pick label already exists on the PR."""
mock_pull_request = Mock()
mock_pull_request.title = "Test PR"

# Simulate existing cherry-pick label for branch1
existing_label = Mock()
existing_label.name = f"{CHERRY_PICK_LABEL_PREFIX}branch1"
mock_pull_request.labels = [existing_label]

# Set merged_at in hook_data to simulate a merged PR at comment time
issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2026-01-01T00:00:00Z"}
with (
patch.object(issue_comment_handler.repository, "get_branch"),
patch.object(
issue_comment_handler.runner_handler,
"cherry_pick",
new_callable=AsyncMock,
) as mock_cherry_pick,
patch.object(
issue_comment_handler.labels_handler,
"_add_label",
new_callable=AsyncMock,
return_value=True,
) as mock_add_label,
patch.object(mock_pull_request, "create_issue_comment") as mock_comment,
):
await issue_comment_handler.process_cherry_pick_command(
pull_request=mock_pull_request,
command_args="branch1 branch2",
reviewed_user="test-user",
)

# branch1 should be skipped (label already exists), branch2 should proceed
mock_cherry_pick.assert_called_once_with(
pull_request=mock_pull_request,
target_branch="branch2",
assign_to_pr_owner=True,
)

# Only branch2 label should be added
mock_add_label.assert_called_once_with(
pull_request=mock_pull_request,
label=f"{CHERRY_PICK_LABEL_PREFIX}branch2",
)

# Verify "already present" comment was posted for branch1
assert any(
"already present" in call.args[0] and "branch1" in call.args[0] for call in mock_comment.call_args_list
), f"Expected 'already present' comment for branch1, got: {mock_comment.call_args_list}"

@pytest.mark.asyncio
async def test_process_cherry_pick_command_all_already_cherry_picked(
self, issue_comment_handler: IssueCommentHandler
) -> None:
"""Test cherry-pick does nothing when all branches already have cherry-pick labels."""
mock_pull_request = Mock()
mock_pull_request.title = "Test PR"

# All branches already cherry-picked
label1 = Mock()
label1.name = f"{CHERRY_PICK_LABEL_PREFIX}branch1"
label2 = Mock()
label2.name = f"{CHERRY_PICK_LABEL_PREFIX}branch2"
mock_pull_request.labels = [label1, label2]

issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2026-01-01T00:00:00Z"}
with (
patch.object(issue_comment_handler.repository, "get_branch"),
patch.object(
issue_comment_handler.runner_handler,
"cherry_pick",
new_callable=AsyncMock,
) as mock_cherry_pick,
patch.object(
issue_comment_handler.labels_handler,
"_add_label",
new_callable=AsyncMock,
) as mock_add_label,
patch.object(mock_pull_request, "create_issue_comment") as mock_comment,
):
await issue_comment_handler.process_cherry_pick_command(
pull_request=mock_pull_request,
command_args="branch1 branch2",
reviewed_user="test-user",
)

# No cherry-picks should be triggered
mock_cherry_pick.assert_not_called()

# No labels should be added
mock_add_label.assert_not_called()

# "already present" comment should be posted
assert any("already present" in call.args[0] for call in mock_comment.call_args_list), (
f"Expected 'already present' comment, got: {mock_comment.call_args_list}"
)

@pytest.mark.asyncio
async def test_process_cherry_pick_command_skips_when_add_label_fails(
self, issue_comment_handler: IssueCommentHandler
) -> None:
"""Test cherry_pick is skipped when _add_label returns False (TOCTOU protection)."""
mock_pull_request = Mock()
mock_pull_request.title = "Test PR"
mock_pull_request.labels = [] # No existing labels in snapshot

issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": "2024-01-01T00:00:00Z"}

with (
patch.object(issue_comment_handler.repository, "get_branch"),
# _add_label returns False (label was added by another webhook in the meantime)
patch.object(
issue_comment_handler.labels_handler,
"_add_label",
new_callable=AsyncMock,
return_value=False,
),
patch.object(
issue_comment_handler.runner_handler,
"cherry_pick",
new_callable=AsyncMock,
) as mock_cherry_pick,
):
await issue_comment_handler.process_cherry_pick_command(
pull_request=mock_pull_request,
command_args="branch1",
reviewed_user="test-user",
)
mock_cherry_pick.assert_not_called()

@pytest.mark.asyncio
async def test_process_cherry_pick_command_skips_already_cherry_picked_unmerged(
self, issue_comment_handler: IssueCommentHandler
) -> None:
"""Test cherry-pick label check works for unmerged PRs too."""
mock_pull_request = Mock()
mock_pull_request.title = "Test PR"

# Simulate existing cherry-pick label for branch1
existing_label = Mock()
existing_label.name = f"{CHERRY_PICK_LABEL_PREFIX}branch1"
mock_pull_request.labels = [existing_label]

# Set merged_at to None to simulate an unmerged PR
issue_comment_handler.hook_data["issue"]["pull_request"] = {"merged_at": None}
with (
patch.object(issue_comment_handler.repository, "get_branch"),
patch.object(
issue_comment_handler.labels_handler,
"_add_label",
new_callable=AsyncMock,
) as mock_add_label,
patch.object(mock_pull_request, "create_issue_comment") as mock_comment,
):
await issue_comment_handler.process_cherry_pick_command(
pull_request=mock_pull_request,
command_args="branch1 branch2",
reviewed_user="test-user",
)

# Verify "already present" comment was posted for branch1
assert any(
"already present" in call.args[0] and "branch1" in call.args[0] for call in mock_comment.call_args_list
), f"Expected 'already present' comment for branch1, got: {mock_comment.call_args_list}"

# Only branch2 label should be added (not branch1)
mock_add_label.assert_called_once_with(
pull_request=mock_pull_request,
label=f"{CHERRY_PICK_LABEL_PREFIX}branch2",
)

@pytest.mark.asyncio
async def test_process_retest_command_no_target_tests(self, issue_comment_handler: IssueCommentHandler) -> None:
"""Test processing retest command with no target tests."""
Expand Down