From 6de177521f287d898d087d17f371c8a8ca4e87db Mon Sep 17 00:00:00 2001 From: OpenHands Date: Tue, 10 Dec 2024 12:19:55 -0500 Subject: [PATCH] Fix issue #5450: In openhands-resolver.yml, request code review from the person who initiated the workflow (#5451) Co-authored-by: Graham Neubig --- .github/workflows/openhands-resolver.yml | 3 +- openhands/resolver/send_pull_request.py | 23 +++++++ tests/unit/resolver/test_send_pull_request.py | 66 +++++++++++++++++++ 3 files changed, 91 insertions(+), 1 deletion(-) diff --git a/.github/workflows/openhands-resolver.yml b/.github/workflows/openhands-resolver.yml index 1348d24806..b68627f41e 100644 --- a/.github/workflows/openhands-resolver.yml +++ b/.github/workflows/openhands-resolver.yml @@ -239,7 +239,8 @@ jobs: if [ "${{ steps.check_result.outputs.RESOLUTION_SUCCESS }}" == "true" ]; then cd /tmp && python -m openhands.resolver.send_pull_request \ --issue-number ${{ env.ISSUE_NUMBER }} \ - --pr-type draft | tee pr_result.txt && \ + --pr-type draft \ + --reviewer ${{ github.actor }} | tee pr_result.txt && \ grep "draft created" pr_result.txt | sed 's/.*\///g' > pr_number.txt else cd /tmp && python -m openhands.resolver.send_pull_request \ diff --git a/openhands/resolver/send_pull_request.py b/openhands/resolver/send_pull_request.py index 9cfe0ce8d3..d888691e58 100644 --- a/openhands/resolver/send_pull_request.py +++ b/openhands/resolver/send_pull_request.py @@ -238,6 +238,7 @@ def send_pull_request( fork_owner: str | None = None, additional_message: str | None = None, target_branch: str | None = None, + reviewer: str | None = None, ) -> str: """Send a pull request to a GitHub repository. @@ -350,6 +351,19 @@ def send_pull_request( response.raise_for_status() pr_data = response.json() + # Request review if a reviewer was specified + if reviewer and pr_type != 'branch': + review_data = {'reviewers': [reviewer]} + review_response = requests.post( + f'{base_url}/pulls/{pr_data["number"]}/requested_reviewers', + headers=headers, + json=review_data, + ) + if review_response.status_code != 201: + print( + f'Warning: Failed to request review from {reviewer}: {review_response.text}' + ) + url = pr_data['html_url'] print(f'{pr_type} created: {url}\n\n--- Title: {pr_title}\n\n--- Body:\n{pr_body}') @@ -520,6 +534,7 @@ def process_single_issue( fork_owner: str | None, send_on_failure: bool, target_branch: str | None = None, + reviewer: str | None = None, ) -> None: if not resolver_output.success and not send_on_failure: print( @@ -569,6 +584,7 @@ def process_single_issue( fork_owner=fork_owner, additional_message=resolver_output.success_explanation, target_branch=target_branch, + reviewer=reviewer, ) @@ -665,6 +681,12 @@ def main(): default=None, help='Target branch to create the pull request against (defaults to repository default branch)', ) + parser.add_argument( + '--reviewer', + type=str, + help='GitHub username of the person to request review from', + default=None, + ) my_args = parser.parse_args() github_token = ( @@ -718,6 +740,7 @@ def main(): my_args.fork_owner, my_args.send_on_failure, my_args.target_branch, + my_args.reviewer, ) diff --git a/tests/unit/resolver/test_send_pull_request.py b/tests/unit/resolver/test_send_pull_request.py index c31d88cbae..c83b8a892c 100644 --- a/tests/unit/resolver/test_send_pull_request.py +++ b/tests/unit/resolver/test_send_pull_request.py @@ -432,6 +432,69 @@ def test_send_pull_request( assert post_data['draft'] == (pr_type == 'draft') +@patch('subprocess.run') +@patch('requests.post') +@patch('requests.get') +def test_send_pull_request_with_reviewer( + mock_get, mock_post, mock_run, mock_github_issue, mock_output_dir +): + repo_path = os.path.join(mock_output_dir, 'repo') + reviewer = 'test-reviewer' + + # Mock API responses + mock_get.side_effect = [ + MagicMock(status_code=404), # Branch doesn't exist + MagicMock(json=lambda: {'default_branch': 'main'}), # Get default branch + ] + + # Mock PR creation response + mock_post.side_effect = [ + MagicMock( + status_code=201, + json=lambda: { + 'html_url': 'https://github.com/test-owner/test-repo/pull/1', + 'number': 1, + }, + ), # PR creation + MagicMock(status_code=201), # Reviewer request + ] + + # Mock subprocess.run calls + mock_run.side_effect = [ + MagicMock(returncode=0), # git checkout -b + MagicMock(returncode=0), # git push + ] + + # Call the function with reviewer + result = send_pull_request( + github_issue=mock_github_issue, + github_token='test-token', + github_username='test-user', + patch_dir=repo_path, + pr_type='ready', + reviewer=reviewer, + ) + + # Assert API calls + assert mock_get.call_count == 2 + assert mock_post.call_count == 2 + + # Check PR creation + pr_create_call = mock_post.call_args_list[0] + assert pr_create_call[1]['json']['title'] == 'Fix issue #42: Test Issue' + + # Check reviewer request + reviewer_request_call = mock_post.call_args_list[1] + assert ( + reviewer_request_call[0][0] + == 'https://api.github.com/repos/test-owner/test-repo/pulls/1/requested_reviewers' + ) + assert reviewer_request_call[1]['json'] == {'reviewers': ['test-reviewer']} + + # Check the result URL + assert result == 'https://github.com/test-owner/test-repo/pull/1' + + @patch('requests.get') def test_send_pull_request_invalid_target_branch( mock_get, mock_github_issue, mock_output_dir @@ -764,6 +827,7 @@ def test_process_single_issue( fork_owner=None, additional_message=resolver_output.success_explanation, target_branch=None, + reviewer=None, ) @@ -1031,6 +1095,7 @@ def test_main( mock_args.llm_base_url = 'mock_url' mock_args.llm_api_key = 'mock_key' mock_args.target_branch = None + mock_args.reviewer = None mock_parser.return_value.parse_args.return_value = mock_args # Setup environment variables @@ -1065,6 +1130,7 @@ def test_main( None, False, mock_args.target_branch, + mock_args.reviewer, ) # Other assertions