feat(cherry-pick): add PR author attribution and optional assignee for cherry-pick PRs#994
Open
feat(cherry-pick): add PR author attribution and optional assignee for cherry-pick PRs#994
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a new global and per-repository config flag to optionally assign cherry-pick PRs to the original PR author, threads the original author through label-triggered cherry-pick flows, updates the cherry-pick implementation to support by-label formatting and optional assignee, and updates tests accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant PR as Pull Request
participant PRH as PullRequestHandler
participant RH as RunnerHandler
participant GH as GitHub
PR->>PRH: merged with `cherry-pick-<branch>` label(s)
PRH->>PRH: filter labels\nextract original PR author
PRH->>RH: cherry_pick(pr, target_branch,\nreviewed_user=pr_author, by_label=true)
RH->>RH: build requested_by ("by {author} with target-branch label")
alt repo config cherry_pick_assign_to_pr_author = true
RH->>RH: set assignee_flag ("-a {author_login}")
else
RH->>RH: no assignee_flag
end
alt target branch exists
RH->>GH: execute hub create pull-request\n(include assignee_flag & labels)
GH-->>RH: success (cherry-pick PR created)
else
RH->>RH: log error
RH->>GH: post comment on original PR about failure
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Review Summary by QodoAdd PR author attribution and optional assignee for cherry-pick PRs
WalkthroughsDescription• Add PR author attribution to cherry-pick descriptions with new by_label parameter • Implement optional assignee feature via cherry-pick-assign-to-pr-author config option • Extract PR author login when cherry-pick triggered by label on merge • Add comprehensive test coverage for assignee and attribution functionality File Changes1. webhook_server/libs/github_api.py
|
Collaborator
|
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. |
Code Review by Qodo
✅ 1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
myakove-bot
added
commented-qodo-code-review[bot]
commented-rnetser
and removed
commented-rnetser
commented-qodo-code-review[bot]
labels
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/runner_handler.py`:
- Around line 539-555: In cherry_pick, avoid producing "by with target-branch
label" when by_label is True but reviewed_user is empty: update the requested_by
assignment to check reviewed_user and use a sensible fallback (e.g. "by
target-branch label" or "by unknown") or format conditionally so you only
include "by {reviewed_user} with target-branch label" when reviewed_user is
non-empty; reference the cherry_pick method and the requested_by / reviewed_user
/ by_label variables when making the change.
In `@webhook_server/tests/test_runner_handler.py`:
- Around line 965-1003: Rename the test function
test_cherry_pick_assigns_to_pr_author_fallback to reflect actual behavior (e.g.,
test_cherry_pick_always_assigns_to_pr_author_when_flag_set) and update its
docstring to state that when
runner_handler.github_webhook.cherry_pick_assign_to_pr_author is True the
assignee is always pull_request.user.login regardless of reviewed_user; also
update the inline comment on Line 996 to match this invariant (referencing
runner_handler.cherry_pick, reviewed_user, and cherry_pick_assign_to_pr_author
to locate the code).
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/tests/test_runner_handler.py`:
- Around line 1008-1046: Add a new async test that exercises
runner_handler.cherry_pick with by_label=True and reviewed_user="" to cover the
fallback branch that sets requested_by to "by target-branch label";
specifically, create a test named like
test_cherry_pick_by_label_empty_reviewed_user_requested_by_format that mirrors
test_cherry_pick_by_label_requested_by_format's setup (patch is_branch_exists,
check_run_handler methods, _checkout_worktree, run_command, and
PR.create_issue_comment) but calls runner_handler.cherry_pick(mock_pull_request,
"main", reviewed_user="", by_label=True) and asserts the last run_command
invocation's hub command contains "requested-by by target-branch label" and does
not contain the double-space pattern "by with".
- Around line 999-1000: The test's assertion using mock_to_thread.call_count >=
1 is too weak; when cherry_pick_assign_to_pr_author=True the success path should
make exactly two asyncio.to_thread calls (one for accessing user.login and one
for create_issue_comment). Update the assertion in the test_runner_handler to
assert mock_to_thread.call_count == 2 (or otherwise verify the two specific
calls exist), and/or add an explicit check that one of the mock_to_thread calls
invoked user.login (e.g., by inspecting mock_to_thread.call_args_list) so the
test guarantees user.login was called via asyncio.to_thread.
… call count checks
- Capture asyncio.to_thread mock in test_cherry_pick_by_label_requested_by_format and add call_count == 2 assertion for consistency with sibling tests - Remove dead-code mock_pull_request.user setup in test_cherry_pick_by_label_empty_reviewed_user_requested_by_format since cherry_pick_assign_to_pr_author=False means user.login is never accessed
rnetser
force-pushed
the
feat/issue-993-cherry-pick-author-attribution
branch
from
2556e41 to
9f51fd3
Compare
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/config/schema.yaml`:
- Around line 82-85: Add the new schema key "cherry-pick-assign-to-pr-author" to
the config schema tests: update the valid_full_config fixture in
webhook_server/tests/test_config_schema.py to include
"cherry-pick-assign-to-pr-author" at both the global and per-repo levels, and
add "cherry-pick-assign-to-pr-author" to the boolean_fields lists in
webhook_server/tests/test_schema_validator.py (the global list near
"auto-verify-cherry-picked-prs" and the repo-level list near "verified-job") so
the schema validator tests cover this boolean option.
In `@webhook_server/libs/handlers/runner_handler.py`:
- Around line 561-565: The code is directly accessing pull_request.user.login;
replace that with a call through the unified API on the webhook to fetch the PR
author (e.g., use self.github_webhook.unified_api to get the pull request author
login) when cherry_pick_assign_to_pr_author is true, then build assignee_flag
from that value (keep the existing shlex.quote usage and flag format); update
the block that references cherry_pick_assign_to_pr_author, pull_request and
assignee_flag to use the unified_api helper instead of pull_request.user.login.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
webhook_server/config/schema.yamlwebhook_server/libs/github_api.pywebhook_server/libs/handlers/pull_request_handler.pywebhook_server/libs/handlers/runner_handler.pywebhook_server/tests/test_pull_request_handler.pywebhook_server/tests/test_runner_handler.py
…educe test nesting - Reuse reviewed_user as pr_author when by_label=True to avoid redundant asyncio.to_thread call for pull_request.user.login - Fix misleading fallback message: use "unknown requester" instead of "by target-branch label" for comment-triggered cherry-picks - Extract CherryPickMocks dataclass and cherry_pick_setup async context manager to reduce 7-8 levels of nesting to 1 in cherry-pick tests
…feat/issue-993-cherry-pick-author-attribution
Always use pull_request.user.login as the cherry-pick PR assignee rather than the user who requested the cherry-pick. Wrap the user.login access with asyncio.to_thread to avoid blocking. Collapse redundant conditional in by_label branch. Update tests to assert PR author as assignee and correct to_thread call counts.
…ering Combine list comprehension assignment and truthiness check into a single walrus operator expression.
Collaborator
Author
|
/build-and-push-container |
Collaborator
|
New container for ghcr.io/myk-org/github-webhook-server:pr-994 published |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
requested-by by <user> with target-branch labelinstead of the anonymousrequested-by by target-branch labelcherry-pick-assign-to-pr-author(boolean, default: false) to assign cherry-pick PRs to the original PR author via hub's-aflagTest plan
Closes #993
Summary by CodeRabbit
New Features
Improvements
Tests