fix: prevent duplicate cherry-pick PRs via label check#1055
Conversation
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughProcess_cherry_pick_command was changed to use hyphenated labels ( Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webhook_server/libs/handlers/issue_comment_handler.py`:
- Around line 482-490: The loop currently calls runner_handler.cherry_pick for
each branch and delays adding the cp_labels until after the whole loop, risking
missing idempotency markers when a later cherry-pick fails; fix by moving the
label add inside the per-branch loop so each successful cherry-pick records its
source PR label before proceeding to the next branch: inside the for _branch in
_branches_to_process loop (where runner_handler.cherry_pick is called),
immediately call labels_handler._add_label for the corresponding cp_label (use a
zip or map between _branches_to_process and cp_labels or derive the label for
_branch) and only add the label after the await runner_handler.cherry_pick
returns successfully. Ensure you do not duplicate labels if they already exist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2fc7e64e-38df-43b9-ae23-4c12be4e88f2
📒 Files selected for processing (2)
webhook_server/libs/handlers/issue_comment_handler.pywebhook_server/tests/test_issue_comment_handler.py
myakove
force-pushed
the
fix/issue-1054-prevent-duplicate-cherry-picks
branch
from
0d17391 to
858c93e
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webhook_server/libs/handlers/issue_comment_handler.py`:
- Around line 419-423: Update the example wording that describes the cherry-pick
label so it no longer implies completion; instead state that the label is set
before runner_handler.cherry_pick() as an idempotency/request marker and may
remain after a failed attempt, and clarify that removal of the label is required
to re-trigger the operation; modify the paragraph that currently treats the
label as a completion marker to explicitly call it an "idempotency/request
marker" and mention runner_handler.cherry_pick() to show when the label is
applied.
- Around line 449-458: The current TOCTOU comes from using a snapshot
(existing_labels / _branches_to_process) to decide work but ignoring the
_add_label() result as a merged-path lock before calling cherry_pick(); update
the loop that processes _branches_to_process so that for each branch you call
_add_label(branch) and only call cherry_pick(...) when _add_label returned True,
otherwise skip/record it as already handled (respecting _already_cherry_picked
semantics) and do not call cherry_pick; apply the same fix in the other
processing block that handles branches (the code around where _add_label and
cherry_pick are currently called) and add a regression test that simulates
_add_label() returning False on a merged PR and asserts that cherry_pick() is
not invoked for that branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 185be1d8-42a5-46c9-93d2-5f4271154630
📒 Files selected for processing (2)
webhook_server/libs/handlers/issue_comment_handler.pywebhook_server/tests/test_issue_comment_handler.py
myakove
force-pushed
the
fix/issue-1054-prevent-duplicate-cherry-picks
branch
from
858c93e to
9226052
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitia right now we check via lables, another approch is to check via our comment when cherry pick secceded knowen parts are I need to know if this is better and if it worth the extra api calls (since it will be more api calls) |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me look at the cherry-pick comment posting code to give you a well-informed opinion. 🏁 Script executed: Length of output: 12881
Comparison
Key issue with the comment approachLooking at the actual code in f"Cherry-picked PR {pull_request.title} into {target_branch}: {cherry_pick_pr_url}"It's a URL, not My recommendation: stick with the label-based approachHIGH reason — the comment approach doesn't actually replace labels; it adds a second mechanism on top. Since labels are still required for the unmerged flow, you'd end up maintaining both systems. For the merged flow, the label already gets added ( MEDIUM reason — the "remove label to re-trigger" escape hatch is already intentional and documented in the docstring. The comment approach has no clean re-trigger mechanism (you can't un-post a comment to re-enable processing). The comment-based approach would only add value if you wanted to detect cherry-picks that were done outside of this system (e.g., manually), but that's a separate use case. For the problem in 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@webhook_server/libs/handlers/issue_comment_handler.py`:
- Around line 474-481: Fix the typo and remove the redundant list comprehension
in the info message: in issue_comment_handler.py update the info_msg string used
near pull_request.create_issue_comment to change "cheery-pick" → "cherry-pick"
and replace `" ".join([_cp_label for _cp_label in cp_labels])` with `"
".join(cp_labels)` (referencing pull_request, info_msg, and cp_labels to locate
the code).
In `@webhook_server/tests/test_issue_comment_handler.py`:
- Around line 984-996: The nested context managers around patch.object calls
should be consolidated into a single with statement to address Ruff SIM117:
combine the patches for issue_comment_handler.repository.get_branch,
issue_comment_handler.runner_handler.cherry_pick (AsyncMock),
issue_comment_handler.labels_handler._add_label (AsyncMock), and
mock_pull_request.create_issue_comment into one with ... as ... tuple-style
context so all mocks (mock_cherry_pick, mock_add_label, mock_comment, etc.) are
created in a single block; apply the same consolidation pattern to the other
occurrences (the blocks around lines 1038-1048, 1078-1090, 1113-1119) while
preserving the new_callable and return_value arguments for AsyncMock patches.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 55c7c169-ebe3-4c66-93e0-c8e1ecc500a4
📒 Files selected for processing (2)
webhook_server/libs/handlers/issue_comment_handler.pywebhook_server/tests/test_issue_comment_handler.py
Skip cherry-pick for branches whose cherry-pick label already exists on the PR. Posts comment telling user to remove the label to re-trigger. Works for both merged and unmerged PRs. Closes #1054
myakove
force-pushed
the
fix/issue-1054-prevent-duplicate-cherry-picks
branch
from
9226052 to
0668b43
Compare
|
New container for ghcr.io/myk-org/github-webhook-server:latest published |
2 participants
Summary
cherry-pick-{branch}label is already presentTriggered by duplicate cherry-pick PRs on RedHatQE/mtv-api-tests#369
Closes #1054
Test plan
test_process_cherry_pick_command_skips_already_cherry_picked- partial skip (merged PR)test_process_cherry_pick_command_all_already_cherry_picked- full skip (merged PR)test_process_cherry_pick_command_skips_already_cherry_picked_unmerged- partial skip (unmerged PR)Summary by CodeRabbit
Bug Fixes
Tests