From 2f8f4b6a58fcf7b64dfc43ad8749651cb42e15bd Mon Sep 17 00:00:00 2001 From: Raymond Xu Date: Sat, 16 Nov 2024 00:08:53 +0000 Subject: [PATCH 1/7] ability to select a branch --- .github/workflows/openhands-resolver.yml | 23 ++++++++++++- openhands/resolver/resolve_issue.py | 42 ++++++++++++++++++++++-- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/.github/workflows/openhands-resolver.yml b/.github/workflows/openhands-resolver.yml index 6b04b2dc02a7..11e2b5933911 100644 --- a/.github/workflows/openhands-resolver.yml +++ b/.github/workflows/openhands-resolver.yml @@ -11,6 +11,16 @@ on: required: false type: string default: "@openhands-agent" + source_branch: + required: false + type: string + default: "main" + description: 'Source branch to pull from (for PRs)' + target_branch: + required: false + type: string + default: "main" + description: 'Target branch to create PR against (for PRs)' secrets: LLM_MODEL: required: true @@ -124,6 +134,15 @@ jobs: echo "MAX_ITERATIONS=${{ inputs.max_iterations || 50 }}" >> $GITHUB_ENV echo "SANDBOX_ENV_GITHUB_TOKEN=${{ secrets.GITHUB_TOKEN }}" >> $GITHUB_ENV + + # Set branch variables + echo "SOURCE_BRANCH=${{ inputs.source_branch }}" >> $GITHUB_ENV + # If source_branch is specified but target_branch isn't, use source_branch for both + if [ -n "${{ inputs.source_branch }}" ] && [ "${{ inputs.target_branch }}" == "main" ]; then + echo "TARGET_BRANCH=${{ inputs.source_branch }}" >> $GITHUB_ENV + else + echo "TARGET_BRANCH=${{ inputs.target_branch }}" >> $GITHUB_ENV + fi - name: Comment on issue with start message uses: actions/github-script@v7 @@ -161,7 +180,9 @@ jobs: --issue-number ${{ env.ISSUE_NUMBER }} \ --issue-type ${{ env.ISSUE_TYPE }} \ --max-iterations ${{ env.MAX_ITERATIONS }} \ - --comment-id ${{ env.COMMENT_ID }} + --comment-id ${{ env.COMMENT_ID }} \ + --source-branch ${{ env.SOURCE_BRANCH }} \ + --target-branch ${{ env.TARGET_BRANCH }} - name: Check resolution result id: check_result diff --git a/openhands/resolver/resolve_issue.py b/openhands/resolver/resolve_issue.py index 67eb20bee1e0..afda8b735b92 100644 --- a/openhands/resolver/resolve_issue.py +++ b/openhands/resolver/resolve_issue.py @@ -161,6 +161,8 @@ async def process_issue( issue_handler: IssueHandlerInterface, repo_instruction: str | None = None, reset_logger: bool = False, + source_branch: str | None = None, + target_branch: str | None = None, ) -> ResolverOutput: # Setup the logger properly, so you can run multi-processing to parallelize processing if reset_logger: @@ -315,6 +317,8 @@ async def resolve_issue( repo_instruction: str | None, issue_number: int, comment_id: int | None, + source_branch: str | None = None, + target_branch: str | None = None, reset_logger: bool = False, ) -> None: """Resolve a single github issue. @@ -333,6 +337,8 @@ async def resolve_issue( repo_instruction: Repository instruction to use. issue_number: Issue number to resolve. comment_id: Optional ID of a specific comment to focus on. + source_branch: Optional source branch to pull from (for PRs). + target_branch: Optional target branch to create PR against (for PRs). reset_logger: Whether to reset the logger for multiprocessing. """ issue_handler = issue_handler_factory(issue_type, owner, repo, token) @@ -425,15 +431,31 @@ async def resolve_issue( try: # checkout to pr branch if needed if issue_type == 'pr': + branch_to_use = source_branch if source_branch else issue.head_branch logger.info( - f'Checking out to PR branch {issue.head_branch} for issue {issue.number}' + f'Checking out to PR branch {branch_to_use} for issue {issue.number}' ) + # Fetch the branch first to ensure it exists locally subprocess.check_output( - ['git', 'checkout', f'{issue.head_branch}'], + ['git', 'fetch', 'origin', branch_to_use], cwd=repo_dir, ) + # Checkout the branch + subprocess.check_output( + ['git', 'checkout', branch_to_use], + cwd=repo_dir, + ) + + # Update issue's head_branch if using custom source branch + if source_branch: + issue.head_branch = source_branch + + # Update issue's base_branch if using custom target branch + if target_branch: + issue.base_branch = target_branch + base_commit = ( subprocess.check_output(['git', 'rev-parse', 'HEAD'], cwd=repo_dir) .decode('utf-8') @@ -451,6 +473,8 @@ async def resolve_issue( issue_handler, repo_instruction, reset_logger, + source_branch, + target_branch, ) output_fp.write(output.model_dump_json() + '\n') output_fp.flush() @@ -556,6 +580,18 @@ def int_or_none(value): choices=['issue', 'pr'], help='Type of issue to resolve, either open issue or pr comments.', ) + parser.add_argument( + '--source-branch', + type=str, + default=None, + help='Source branch to pull from (for PRs). If not specified, uses the PR\'s head branch.', + ) + parser.add_argument( + '--target-branch', + type=str, + default=None, + help='Target branch to create PR against (for PRs). If not specified, uses the PR\'s base branch.', + ) my_args = parser.parse_args() @@ -616,6 +652,8 @@ def int_or_none(value): repo_instruction=repo_instruction, issue_number=my_args.issue_number, comment_id=my_args.comment_id, + source_branch=my_args.source_branch, + target_branch=my_args.target_branch, ) ) From dd12bb0ac9fe42fda3bf9afe30c58f9ca65bb5dd Mon Sep 17 00:00:00 2001 From: Raymond Xu Date: Sat, 16 Nov 2024 00:20:26 +0000 Subject: [PATCH 2/7] Nov 15, 2024, 4:20 PM --- tests/unit/resolver/test_send_pull_request.py | 426 ++---------------- 1 file changed, 43 insertions(+), 383 deletions(-) diff --git a/tests/unit/resolver/test_send_pull_request.py b/tests/unit/resolver/test_send_pull_request.py index 951be1af006c..e76d6a72b084 100644 --- a/tests/unit/resolver/test_send_pull_request.py +++ b/tests/unit/resolver/test_send_pull_request.py @@ -326,7 +326,7 @@ def test_update_existing_pull_request( @patch('subprocess.run') @patch('requests.post') @patch('requests.get') -def test_send_pull_request( +def test_send_pull_request_with_custom_branches( mock_get, mock_post, mock_run, @@ -336,11 +336,17 @@ def test_send_pull_request( pr_type, ): repo_path = os.path.join(mock_output_dir, 'repo') + source_branch = 'feature-branch' + target_branch = 'development' + + # Update issue with custom branches + mock_github_issue.head_branch = source_branch + mock_github_issue.base_branch = target_branch # Mock API responses mock_get.side_effect = [ MagicMock(status_code=404), # Branch doesn't exist - MagicMock(json=lambda: {'default_branch': 'main'}), + MagicMock(json=lambda: {'default_branch': target_branch}), ] mock_post.return_value.json.return_value = { 'html_url': 'https://github.com/test-owner/test-repo/pull/1' @@ -401,15 +407,16 @@ def test_send_pull_request( assert post_data['title'] == 'Fix issue #42: Test Issue' assert post_data['body'].startswith('This pull request fixes #42.') assert post_data['head'] == 'openhands-fix-issue-42' - assert post_data['base'] == 'main' + assert post_data['base'] == target_branch # Should use target_branch instead of 'main' assert post_data['draft'] == (pr_type == 'draft') +@pytest.mark.parametrize('pr_type', ['branch', 'draft', 'ready']) @patch('subprocess.run') @patch('requests.post') @patch('requests.get') def test_send_pull_request_git_push_failure( - mock_get, mock_post, mock_run, mock_github_issue, mock_output_dir, mock_llm_config + mock_get, mock_post, mock_run, mock_github_issue, mock_output_dir, mock_llm_config, pr_type ): repo_path = os.path.join(mock_output_dir, 'repo') @@ -721,179 +728,23 @@ def test_process_single_issue_unsuccessful( mock_initialize_repo, mock_output_dir, mock_llm_config, -): - # Initialize test data - github_token = 'test_token' - github_username = 'test_user' - pr_type = 'draft' - - resolver_output = ResolverOutput( - issue=GithubIssue( - owner='test-owner', - repo='test-repo', - number=1, - title='Issue 1', - body='Body 1', - ), - issue_type='issue', - instruction='Test instruction 1', - base_commit='def456', - git_patch='Test patch 1', - history=[], - metrics={}, - success=False, - comment_success=None, - success_explanation='', - error='Test error', - ) - - # Call the function - process_single_issue( - mock_output_dir, - resolver_output, - github_token, - github_username, - pr_type, - mock_llm_config, - None, - False, - ) - - # Assert that none of the mocked functions were called - mock_initialize_repo.assert_not_called() - mock_apply_patch.assert_not_called() - mock_make_commit.assert_not_called() - mock_send_pull_request.assert_not_called() - - -@patch('openhands.resolver.send_pull_request.load_all_resolver_outputs') -@patch('openhands.resolver.send_pull_request.process_single_issue') -def test_process_all_successful_issues( - mock_process_single_issue, mock_load_all_resolver_outputs, mock_llm_config -): - # Create ResolverOutput objects with properly initialized GithubIssue instances - resolver_output_1 = ResolverOutput( - issue=GithubIssue( - owner='test-owner', - repo='test-repo', - number=1, - title='Issue 1', - body='Body 1', - ), - issue_type='issue', - instruction='Test instruction 1', - base_commit='def456', - git_patch='Test patch 1', - history=[], - metrics={}, - success=True, - comment_success=None, - success_explanation='Test success 1', - error=None, - ) - - resolver_output_2 = ResolverOutput( - issue=GithubIssue( - owner='test-owner', - repo='test-repo', - number=2, - title='Issue 2', - body='Body 2', - ), - issue_type='issue', - instruction='Test instruction 2', - base_commit='ghi789', - git_patch='Test patch 2', - history=[], - metrics={}, - success=False, - comment_success=None, - success_explanation='', - error='Test error 2', - ) - - resolver_output_3 = ResolverOutput( - issue=GithubIssue( - owner='test-owner', - repo='test-repo', - number=3, - title='Issue 3', - body='Body 3', - ), - issue_type='issue', - instruction='Test instruction 3', - base_commit='jkl012', - git_patch='Test patch 3', - history=[], - metrics={}, - success=True, - comment_success=None, - success_explanation='Test success 3', - error=None, - ) - - mock_load_all_resolver_outputs.return_value = [ - resolver_output_1, - resolver_output_2, - resolver_output_3, - ] - - # Call the function - process_all_successful_issues( - 'output_dir', - 'github_token', - 'github_username', - 'draft', - mock_llm_config, # llm_config - None, # fork_owner - ) - - # Assert that process_single_issue was called for successful issues only - assert mock_process_single_issue.call_count == 2 - - # Check that the function was called with the correct arguments for successful issues - mock_process_single_issue.assert_has_calls( - [ - call( - 'output_dir', - resolver_output_1, - 'github_token', - 'github_username', - 'draft', - mock_llm_config, - None, - False, - ), - call( - 'output_dir', - resolver_output_3, - 'github_token', - 'github_username', - 'draft', - mock_llm_config, - None, - False, - ), - ] - ) - - # Add more assertions as needed to verify the behavior of the function - - -@patch('requests.get') -@patch('subprocess.run') -def test_send_pull_request_branch_naming( - mock_run, mock_get, mock_github_issue, mock_output_dir, mock_llm_config + pr_type, ): repo_path = os.path.join(mock_output_dir, 'repo') + source_branch = 'feature-branch' + + # Update issue with source branch only + mock_github_issue.head_branch = source_branch + mock_github_issue.base_branch = source_branch # Target should match source when only source is specified # Mock API responses mock_get.side_effect = [ - MagicMock(status_code=200), # First branch exists - MagicMock(status_code=200), # Second branch exists - MagicMock(status_code=404), # Third branch doesn't exist - MagicMock(json=lambda: {'default_branch': 'main'}), # Get default branch + MagicMock(status_code=404), # Branch doesn't exist + MagicMock(json=lambda: {'default_branch': 'main'}), ] + mock_post.return_value.json.return_value = { + 'html_url': 'https://github.com/test-owner/test-repo/pull/1' + } # Mock subprocess.run calls mock_run.side_effect = [ @@ -907,19 +758,19 @@ def test_send_pull_request_branch_naming( github_token='test-token', github_username='test-user', patch_dir=repo_path, - pr_type='branch', + pr_type=pr_type, llm_config=mock_llm_config, ) # Assert API calls - assert mock_get.call_count == 4 + assert mock_get.call_count == 2 # Check branch creation and push assert mock_run.call_count == 2 checkout_call, push_call = mock_run.call_args_list assert checkout_call == call( - ['git', '-C', repo_path, 'checkout', '-b', 'openhands-fix-issue-42-try3'], + ['git', '-C', repo_path, 'checkout', '-b', 'openhands-fix-issue-42'], capture_output=True, text=True, ) @@ -930,218 +781,27 @@ def test_send_pull_request_branch_naming( repo_path, 'push', 'https://test-user:test-token@github.com/test-owner/test-repo.git', - 'openhands-fix-issue-42-try3', + 'openhands-fix-issue-42', ], capture_output=True, text=True, ) - # Check the result - assert ( - result - == 'https://github.com/test-owner/test-repo/compare/openhands-fix-issue-42-try3?expand=1' - ) - - -@patch('openhands.resolver.send_pull_request.argparse.ArgumentParser') -@patch('openhands.resolver.send_pull_request.process_all_successful_issues') -@patch('openhands.resolver.send_pull_request.process_single_issue') -@patch('openhands.resolver.send_pull_request.load_single_resolver_output') -@patch('os.path.exists') -@patch('os.getenv') -def test_main( - mock_getenv, - mock_path_exists, - mock_load_single_resolver_output, - mock_process_single_issue, - mock_process_all_successful_issues, - mock_parser, -): - from openhands.resolver.send_pull_request import main - - # Setup mock parser - mock_args = MagicMock() - mock_args.github_token = None - mock_args.github_username = 'mock_username' - mock_args.output_dir = '/mock/output' - mock_args.pr_type = 'draft' - mock_args.issue_number = '42' - mock_args.fork_owner = None - mock_args.send_on_failure = False - mock_args.llm_model = 'mock_model' - mock_args.llm_base_url = 'mock_url' - mock_args.llm_api_key = 'mock_key' - mock_parser.return_value.parse_args.return_value = mock_args - - # Setup environment variables - mock_getenv.side_effect = ( - lambda key, default=None: 'mock_token' if key == 'GITHUB_TOKEN' else default - ) - - # Setup path exists - mock_path_exists.return_value = True - - # Setup mock resolver output - mock_resolver_output = MagicMock() - mock_load_single_resolver_output.return_value = mock_resolver_output - - # Run main function - main() - - llm_config = LLMConfig( - model=mock_args.llm_model, - base_url=mock_args.llm_base_url, - api_key=mock_args.llm_api_key, - ) - - # Assert function calls - mock_parser.assert_called_once() - mock_getenv.assert_any_call('GITHUB_TOKEN') - mock_path_exists.assert_called_with('/mock/output') - mock_load_single_resolver_output.assert_called_with('/mock/output/output.jsonl', 42) - mock_process_single_issue.assert_called_with( - '/mock/output', - mock_resolver_output, - 'mock_token', - 'mock_username', - 'draft', - llm_config, - None, - False, - ) - - # Test for 'all_successful' issue number - mock_args.issue_number = 'all_successful' - main() - mock_process_all_successful_issues.assert_called_with( - '/mock/output', - 'mock_token', - 'mock_username', - 'draft', - llm_config, - None, - ) - - # Test for invalid issue number - mock_args.issue_number = 'invalid' - with pytest.raises(ValueError): - main() - - -@patch('subprocess.run') -def test_make_commit_escapes_issue_title(mock_subprocess_run): - # Setup - repo_dir = '/path/to/repo' - issue = GithubIssue( - owner='test-owner', - repo='test-repo', - number=42, - title='Issue with "quotes" and $pecial characters', - body='Test body', - ) - - # Mock subprocess.run to return success for all calls - mock_subprocess_run.return_value = MagicMock( - returncode=0, stdout='sample output', stderr='' - ) - - # Call the function - issue_type = 'issue' - make_commit(repo_dir, issue, issue_type) - - # Assert that subprocess.run was called with the correct arguments - calls = mock_subprocess_run.call_args_list - assert len(calls) == 4 # git config check, git add, git commit - - # Check the git commit call - git_commit_call = calls[3][0][0] - expected_commit_message = ( - 'Fix issue #42: Issue with "quotes" and $pecial characters' - ) - assert [ - 'git', - '-C', - '/path/to/repo', - 'commit', - '-m', - expected_commit_message, - ] == git_commit_call - - -@patch('subprocess.run') -def test_make_commit_no_changes(mock_subprocess_run): - # Setup - repo_dir = '/path/to/repo' - issue = GithubIssue( - owner='test-owner', - repo='test-repo', - number=42, - title='Issue with no changes', - body='Test body', - ) - - # Mock subprocess.run to simulate no changes in the repo - mock_subprocess_run.side_effect = [ - MagicMock(returncode=0), - MagicMock(returncode=0), - MagicMock(returncode=1, stdout=''), # git status --porcelain (no changes) - ] - - with pytest.raises( - RuntimeError, match='ERROR: Openhands failed to make code changes.' - ): - make_commit(repo_dir, issue, 'issue') - - # Check that subprocess.run was called for checking git status and add, but not commit - assert mock_subprocess_run.call_count == 3 - git_status_call = mock_subprocess_run.call_args_list[2][0][0] - assert f'git -C {repo_dir} status --porcelain' in git_status_call - - -def test_apply_patch_rename_directory(mock_output_dir): - # Create a sample directory structure - old_dir = os.path.join(mock_output_dir, 'prompts', 'resolve') - os.makedirs(old_dir) - - # Create test files - test_files = [ - 'issue-success-check.jinja', - 'pr-feedback-check.jinja', - 'pr-thread-check.jinja', - ] - for filename in test_files: - file_path = os.path.join(old_dir, filename) - with open(file_path, 'w') as f: - f.write(f'Content of {filename}') - - # Create a patch that renames the directory - patch_content = """diff --git a/prompts/resolve/issue-success-check.jinja b/prompts/guess_success/issue-success-check.jinja -similarity index 100% -rename from prompts/resolve/issue-success-check.jinja -rename to prompts/guess_success/issue-success-check.jinja -diff --git a/prompts/resolve/pr-feedback-check.jinja b/prompts/guess_success/pr-feedback-check.jinja -similarity index 100% -rename from prompts/resolve/pr-feedback-check.jinja -rename to prompts/guess_success/pr-feedback-check.jinja -diff --git a/prompts/resolve/pr-thread-check.jinja b/prompts/guess_success/pr-thread-check.jinja -similarity index 100% -rename from prompts/resolve/pr-thread-check.jinja -rename to prompts/guess_success/pr-thread-check.jinja""" - - # Apply the patch - apply_patch(mock_output_dir, patch_content) + # Check PR creation based on pr_type + if pr_type == 'branch': + assert ( + result + == 'https://github.com/test-owner/test-repo/compare/openhands-fix-issue-42?expand=1' + ) + mock_post.assert_not_called() + else: + assert result == 'https://github.com/test-owner/test-repo/pull/1' + mock_post.assert_called_once() + post_data = mock_post.call_args[1]['json'] + assert post_data['title'] == 'Fix issue #42: Test Issue' + assert post_data['body'].startswith('This pull request fixes #42.') + assert post_data['head'] == 'openhands-fix-issue-42' + assert post_data['base'] == source_branch # Should use source_branch as target + assert post_data['draft'] == (pr_type == 'draft') - # Check if files were moved correctly - new_dir = os.path.join(mock_output_dir, 'prompts', 'guess_success') - assert not os.path.exists(old_dir), 'Old directory still exists' - assert os.path.exists(new_dir), 'New directory was not created' - - # Check if all files were moved and content preserved - for filename in test_files: - old_path = os.path.join(old_dir, filename) - new_path = os.path.join(new_dir, filename) - assert not os.path.exists(old_path), f'Old file {filename} still exists' - assert os.path.exists(new_path), f'New file {filename} was not created' - with open(new_path, 'r') as f: - content = f.read() - assert content == f'Content of {filename}', f'Content mismatch for {filename}' +# Rest of the file remains unchanged From 8de65ab37ddb3f0ecf3757f851d2d23d2b180f1e Mon Sep 17 00:00:00 2001 From: Raymond Xu Date: Sat, 16 Nov 2024 00:21:37 +0000 Subject: [PATCH 3/7] Nov 15, 2024, 4:21 PM --- tests/unit/resolver/test_send_pull_request.py | 165 +++++++++++++++++- 1 file changed, 164 insertions(+), 1 deletion(-) diff --git a/tests/unit/resolver/test_send_pull_request.py b/tests/unit/resolver/test_send_pull_request.py index e76d6a72b084..5345ecf50bf0 100644 --- a/tests/unit/resolver/test_send_pull_request.py +++ b/tests/unit/resolver/test_send_pull_request.py @@ -653,6 +653,7 @@ def test_process_single_issue( mock_initialize_repo, mock_output_dir, mock_llm_config, + pr_type, ): # Initialize test data github_token = 'test_token' @@ -728,7 +729,169 @@ def test_process_single_issue_unsuccessful( mock_initialize_repo, mock_output_dir, mock_llm_config, - pr_type, +): + # Initialize test data + github_token = 'test_token' + github_username = 'test_user' + pr_type = 'draft' + + resolver_output = ResolverOutput( + issue=GithubIssue( + owner='test-owner', + repo='test-repo', + number=1, + title='Issue 1', + body='Body 1', + ), + issue_type='issue', + instruction='Test instruction 1', + base_commit='def456', + git_patch='Test patch 1', + history=[], + metrics={}, + success=False, + comment_success=None, + success_explanation='', + error='Test error', + ) + + # Call the function + process_single_issue( + mock_output_dir, + resolver_output, + github_token, + github_username, + pr_type, + mock_llm_config, + None, + False, + ) + + # Assert that none of the mocked functions were called + mock_initialize_repo.assert_not_called() + mock_apply_patch.assert_not_called() + mock_make_commit.assert_not_called() + mock_send_pull_request.assert_not_called() + + +@patch('openhands.resolver.send_pull_request.load_all_resolver_outputs') +@patch('openhands.resolver.send_pull_request.process_single_issue') +def test_process_all_successful_issues( + mock_process_single_issue, mock_load_all_resolver_outputs, mock_llm_config +): + # Create ResolverOutput objects with properly initialized GithubIssue instances + resolver_output_1 = ResolverOutput( + issue=GithubIssue( + owner='test-owner', + repo='test-repo', + number=1, + title='Issue 1', + body='Body 1', + ), + issue_type='issue', + instruction='Test instruction 1', + base_commit='def456', + git_patch='Test patch 1', + history=[], + metrics={}, + success=True, + comment_success=None, + success_explanation='Test success 1', + error=None, + ) + + resolver_output_2 = ResolverOutput( + issue=GithubIssue( + owner='test-owner', + repo='test-repo', + number=2, + title='Issue 2', + body='Body 2', + ), + issue_type='issue', + instruction='Test instruction 2', + base_commit='ghi789', + git_patch='Test patch 2', + history=[], + metrics={}, + success=False, + comment_success=None, + success_explanation='', + error='Test error 2', + ) + + resolver_output_3 = ResolverOutput( + issue=GithubIssue( + owner='test-owner', + repo='test-repo', + number=3, + title='Issue 3', + body='Body 3', + ), + issue_type='issue', + instruction='Test instruction 3', + base_commit='jkl012', + git_patch='Test patch 3', + history=[], + metrics={}, + success=True, + comment_success=None, + success_explanation='Test success 3', + error=None, + ) + + mock_load_all_resolver_outputs.return_value = [ + resolver_output_1, + resolver_output_2, + resolver_output_3, + ] + + # Call the function + process_all_successful_issues( + 'output_dir', + 'github_token', + 'github_username', + 'draft', + mock_llm_config, # llm_config + None, # fork_owner + ) + + # Assert that process_single_issue was called for successful issues only + assert mock_process_single_issue.call_count == 2 + + # Check that the function was called with the correct arguments for successful issues + mock_process_single_issue.assert_has_calls( + [ + call( + 'output_dir', + resolver_output_1, + 'github_token', + 'github_username', + 'draft', + mock_llm_config, + None, + False, + ), + call( + 'output_dir', + resolver_output_3, + 'github_token', + 'github_username', + 'draft', + mock_llm_config, + None, + False, + ), + ] + ) + + # Add more assertions as needed to verify the behavior of the function + + +@patch('requests.get') +@patch('subprocess.run') +def test_send_pull_request_branch_naming( + mock_run, mock_get, mock_github_issue, mock_output_dir, mock_llm_config ): repo_path = os.path.join(mock_output_dir, 'repo') source_branch = 'feature-branch' From 3ae2b0c24f97dde4b981d4769e7df5abb713e309 Mon Sep 17 00:00:00 2001 From: Raymond Xu Date: Fri, 15 Nov 2024 16:24:13 -0800 Subject: [PATCH 4/7] back to orig --- tests/unit/resolver/test_send_pull_request.py | 263 +++++++++++++++--- 1 file changed, 220 insertions(+), 43 deletions(-) diff --git a/tests/unit/resolver/test_send_pull_request.py b/tests/unit/resolver/test_send_pull_request.py index 5345ecf50bf0..951be1af006c 100644 --- a/tests/unit/resolver/test_send_pull_request.py +++ b/tests/unit/resolver/test_send_pull_request.py @@ -326,7 +326,7 @@ def test_update_existing_pull_request( @patch('subprocess.run') @patch('requests.post') @patch('requests.get') -def test_send_pull_request_with_custom_branches( +def test_send_pull_request( mock_get, mock_post, mock_run, @@ -336,17 +336,11 @@ def test_send_pull_request_with_custom_branches( pr_type, ): repo_path = os.path.join(mock_output_dir, 'repo') - source_branch = 'feature-branch' - target_branch = 'development' - - # Update issue with custom branches - mock_github_issue.head_branch = source_branch - mock_github_issue.base_branch = target_branch # Mock API responses mock_get.side_effect = [ MagicMock(status_code=404), # Branch doesn't exist - MagicMock(json=lambda: {'default_branch': target_branch}), + MagicMock(json=lambda: {'default_branch': 'main'}), ] mock_post.return_value.json.return_value = { 'html_url': 'https://github.com/test-owner/test-repo/pull/1' @@ -407,16 +401,15 @@ def test_send_pull_request_with_custom_branches( assert post_data['title'] == 'Fix issue #42: Test Issue' assert post_data['body'].startswith('This pull request fixes #42.') assert post_data['head'] == 'openhands-fix-issue-42' - assert post_data['base'] == target_branch # Should use target_branch instead of 'main' + assert post_data['base'] == 'main' assert post_data['draft'] == (pr_type == 'draft') -@pytest.mark.parametrize('pr_type', ['branch', 'draft', 'ready']) @patch('subprocess.run') @patch('requests.post') @patch('requests.get') def test_send_pull_request_git_push_failure( - mock_get, mock_post, mock_run, mock_github_issue, mock_output_dir, mock_llm_config, pr_type + mock_get, mock_post, mock_run, mock_github_issue, mock_output_dir, mock_llm_config ): repo_path = os.path.join(mock_output_dir, 'repo') @@ -653,7 +646,6 @@ def test_process_single_issue( mock_initialize_repo, mock_output_dir, mock_llm_config, - pr_type, ): # Initialize test data github_token = 'test_token' @@ -894,20 +886,14 @@ def test_send_pull_request_branch_naming( mock_run, mock_get, mock_github_issue, mock_output_dir, mock_llm_config ): repo_path = os.path.join(mock_output_dir, 'repo') - source_branch = 'feature-branch' - - # Update issue with source branch only - mock_github_issue.head_branch = source_branch - mock_github_issue.base_branch = source_branch # Target should match source when only source is specified # Mock API responses mock_get.side_effect = [ - MagicMock(status_code=404), # Branch doesn't exist - MagicMock(json=lambda: {'default_branch': 'main'}), + MagicMock(status_code=200), # First branch exists + MagicMock(status_code=200), # Second branch exists + MagicMock(status_code=404), # Third branch doesn't exist + MagicMock(json=lambda: {'default_branch': 'main'}), # Get default branch ] - mock_post.return_value.json.return_value = { - 'html_url': 'https://github.com/test-owner/test-repo/pull/1' - } # Mock subprocess.run calls mock_run.side_effect = [ @@ -921,19 +907,19 @@ def test_send_pull_request_branch_naming( github_token='test-token', github_username='test-user', patch_dir=repo_path, - pr_type=pr_type, + pr_type='branch', llm_config=mock_llm_config, ) # Assert API calls - assert mock_get.call_count == 2 + assert mock_get.call_count == 4 # Check branch creation and push assert mock_run.call_count == 2 checkout_call, push_call = mock_run.call_args_list assert checkout_call == call( - ['git', '-C', repo_path, 'checkout', '-b', 'openhands-fix-issue-42'], + ['git', '-C', repo_path, 'checkout', '-b', 'openhands-fix-issue-42-try3'], capture_output=True, text=True, ) @@ -944,27 +930,218 @@ def test_send_pull_request_branch_naming( repo_path, 'push', 'https://test-user:test-token@github.com/test-owner/test-repo.git', - 'openhands-fix-issue-42', + 'openhands-fix-issue-42-try3', ], capture_output=True, text=True, ) - # Check PR creation based on pr_type - if pr_type == 'branch': - assert ( - result - == 'https://github.com/test-owner/test-repo/compare/openhands-fix-issue-42?expand=1' - ) - mock_post.assert_not_called() - else: - assert result == 'https://github.com/test-owner/test-repo/pull/1' - mock_post.assert_called_once() - post_data = mock_post.call_args[1]['json'] - assert post_data['title'] == 'Fix issue #42: Test Issue' - assert post_data['body'].startswith('This pull request fixes #42.') - assert post_data['head'] == 'openhands-fix-issue-42' - assert post_data['base'] == source_branch # Should use source_branch as target - assert post_data['draft'] == (pr_type == 'draft') + # Check the result + assert ( + result + == 'https://github.com/test-owner/test-repo/compare/openhands-fix-issue-42-try3?expand=1' + ) + + +@patch('openhands.resolver.send_pull_request.argparse.ArgumentParser') +@patch('openhands.resolver.send_pull_request.process_all_successful_issues') +@patch('openhands.resolver.send_pull_request.process_single_issue') +@patch('openhands.resolver.send_pull_request.load_single_resolver_output') +@patch('os.path.exists') +@patch('os.getenv') +def test_main( + mock_getenv, + mock_path_exists, + mock_load_single_resolver_output, + mock_process_single_issue, + mock_process_all_successful_issues, + mock_parser, +): + from openhands.resolver.send_pull_request import main + + # Setup mock parser + mock_args = MagicMock() + mock_args.github_token = None + mock_args.github_username = 'mock_username' + mock_args.output_dir = '/mock/output' + mock_args.pr_type = 'draft' + mock_args.issue_number = '42' + mock_args.fork_owner = None + mock_args.send_on_failure = False + mock_args.llm_model = 'mock_model' + mock_args.llm_base_url = 'mock_url' + mock_args.llm_api_key = 'mock_key' + mock_parser.return_value.parse_args.return_value = mock_args + + # Setup environment variables + mock_getenv.side_effect = ( + lambda key, default=None: 'mock_token' if key == 'GITHUB_TOKEN' else default + ) + + # Setup path exists + mock_path_exists.return_value = True + + # Setup mock resolver output + mock_resolver_output = MagicMock() + mock_load_single_resolver_output.return_value = mock_resolver_output + + # Run main function + main() + + llm_config = LLMConfig( + model=mock_args.llm_model, + base_url=mock_args.llm_base_url, + api_key=mock_args.llm_api_key, + ) + + # Assert function calls + mock_parser.assert_called_once() + mock_getenv.assert_any_call('GITHUB_TOKEN') + mock_path_exists.assert_called_with('/mock/output') + mock_load_single_resolver_output.assert_called_with('/mock/output/output.jsonl', 42) + mock_process_single_issue.assert_called_with( + '/mock/output', + mock_resolver_output, + 'mock_token', + 'mock_username', + 'draft', + llm_config, + None, + False, + ) + + # Test for 'all_successful' issue number + mock_args.issue_number = 'all_successful' + main() + mock_process_all_successful_issues.assert_called_with( + '/mock/output', + 'mock_token', + 'mock_username', + 'draft', + llm_config, + None, + ) + + # Test for invalid issue number + mock_args.issue_number = 'invalid' + with pytest.raises(ValueError): + main() + + +@patch('subprocess.run') +def test_make_commit_escapes_issue_title(mock_subprocess_run): + # Setup + repo_dir = '/path/to/repo' + issue = GithubIssue( + owner='test-owner', + repo='test-repo', + number=42, + title='Issue with "quotes" and $pecial characters', + body='Test body', + ) + + # Mock subprocess.run to return success for all calls + mock_subprocess_run.return_value = MagicMock( + returncode=0, stdout='sample output', stderr='' + ) + + # Call the function + issue_type = 'issue' + make_commit(repo_dir, issue, issue_type) + + # Assert that subprocess.run was called with the correct arguments + calls = mock_subprocess_run.call_args_list + assert len(calls) == 4 # git config check, git add, git commit + + # Check the git commit call + git_commit_call = calls[3][0][0] + expected_commit_message = ( + 'Fix issue #42: Issue with "quotes" and $pecial characters' + ) + assert [ + 'git', + '-C', + '/path/to/repo', + 'commit', + '-m', + expected_commit_message, + ] == git_commit_call + + +@patch('subprocess.run') +def test_make_commit_no_changes(mock_subprocess_run): + # Setup + repo_dir = '/path/to/repo' + issue = GithubIssue( + owner='test-owner', + repo='test-repo', + number=42, + title='Issue with no changes', + body='Test body', + ) + + # Mock subprocess.run to simulate no changes in the repo + mock_subprocess_run.side_effect = [ + MagicMock(returncode=0), + MagicMock(returncode=0), + MagicMock(returncode=1, stdout=''), # git status --porcelain (no changes) + ] + + with pytest.raises( + RuntimeError, match='ERROR: Openhands failed to make code changes.' + ): + make_commit(repo_dir, issue, 'issue') + + # Check that subprocess.run was called for checking git status and add, but not commit + assert mock_subprocess_run.call_count == 3 + git_status_call = mock_subprocess_run.call_args_list[2][0][0] + assert f'git -C {repo_dir} status --porcelain' in git_status_call + + +def test_apply_patch_rename_directory(mock_output_dir): + # Create a sample directory structure + old_dir = os.path.join(mock_output_dir, 'prompts', 'resolve') + os.makedirs(old_dir) + + # Create test files + test_files = [ + 'issue-success-check.jinja', + 'pr-feedback-check.jinja', + 'pr-thread-check.jinja', + ] + for filename in test_files: + file_path = os.path.join(old_dir, filename) + with open(file_path, 'w') as f: + f.write(f'Content of {filename}') + + # Create a patch that renames the directory + patch_content = """diff --git a/prompts/resolve/issue-success-check.jinja b/prompts/guess_success/issue-success-check.jinja +similarity index 100% +rename from prompts/resolve/issue-success-check.jinja +rename to prompts/guess_success/issue-success-check.jinja +diff --git a/prompts/resolve/pr-feedback-check.jinja b/prompts/guess_success/pr-feedback-check.jinja +similarity index 100% +rename from prompts/resolve/pr-feedback-check.jinja +rename to prompts/guess_success/pr-feedback-check.jinja +diff --git a/prompts/resolve/pr-thread-check.jinja b/prompts/guess_success/pr-thread-check.jinja +similarity index 100% +rename from prompts/resolve/pr-thread-check.jinja +rename to prompts/guess_success/pr-thread-check.jinja""" + + # Apply the patch + apply_patch(mock_output_dir, patch_content) -# Rest of the file remains unchanged + # Check if files were moved correctly + new_dir = os.path.join(mock_output_dir, 'prompts', 'guess_success') + assert not os.path.exists(old_dir), 'Old directory still exists' + assert os.path.exists(new_dir), 'New directory was not created' + + # Check if all files were moved and content preserved + for filename in test_files: + old_path = os.path.join(old_dir, filename) + new_path = os.path.join(new_dir, filename) + assert not os.path.exists(old_path), f'Old file {filename} still exists' + assert os.path.exists(new_path), f'New file {filename} was not created' + with open(new_path, 'r') as f: + content = f.read() + assert content == f'Content of {filename}', f'Content mismatch for {filename}' From 8d08dde5b60b813eda7aa0b7dcccff9f67819607 Mon Sep 17 00:00:00 2001 From: Raymond Xu Date: Tue, 19 Nov 2024 14:57:47 -0800 Subject: [PATCH 5/7] fix mypy lint error? --- openhands/resolver/resolve_issue.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/openhands/resolver/resolve_issue.py b/openhands/resolver/resolve_issue.py index afda8b735b92..0c659a15b770 100644 --- a/openhands/resolver/resolve_issue.py +++ b/openhands/resolver/resolve_issue.py @@ -436,15 +436,20 @@ async def resolve_issue( f'Checking out to PR branch {branch_to_use} for issue {issue.number}' ) + if not branch_to_use: + raise ValueError('Branch name cannot be None') + # Fetch the branch first to ensure it exists locally + fetch_cmd = ['git', 'fetch', 'origin', branch_to_use] subprocess.check_output( - ['git', 'fetch', 'origin', branch_to_use], + fetch_cmd, cwd=repo_dir, ) # Checkout the branch + checkout_cmd = ['git', 'checkout', branch_to_use] subprocess.check_output( - ['git', 'checkout', branch_to_use], + checkout_cmd, cwd=repo_dir, ) @@ -584,13 +589,13 @@ def int_or_none(value): '--source-branch', type=str, default=None, - help='Source branch to pull from (for PRs). If not specified, uses the PR\'s head branch.', + help="Source branch to pull from (for PRs). If not specified, uses the PR's head branch.", ) parser.add_argument( '--target-branch', type=str, default=None, - help='Target branch to create PR against (for PRs). If not specified, uses the PR\'s base branch.', + help="Target branch to create PR against (for PRs). If not specified, uses the PR's base branch.", ) my_args = parser.parse_args() From 47149682cc9d91ba46dfa84371283bac4c9f5653 Mon Sep 17 00:00:00 2001 From: Raymond Xu Date: Wed, 20 Nov 2024 01:53:59 -0800 Subject: [PATCH 6/7] only have a single branch; target branch --- .github/workflows/openhands-resolver.yml | 18 +++--------------- openhands/resolver/resolve_issue.py | 23 +++-------------------- 2 files changed, 6 insertions(+), 35 deletions(-) diff --git a/.github/workflows/openhands-resolver.yml b/.github/workflows/openhands-resolver.yml index 67242ad55e22..7c98ba506643 100644 --- a/.github/workflows/openhands-resolver.yml +++ b/.github/workflows/openhands-resolver.yml @@ -11,16 +11,11 @@ on: required: false type: string default: "@openhands-agent" - source_branch: - required: false - type: string - default: "main" - description: 'Source branch to pull from (for PRs)' target_branch: required: false type: string default: "main" - description: 'Target branch to create PR against (for PRs)' + description: 'Target branch to pull and create PR against' secrets: LLM_MODEL: required: true @@ -144,15 +139,9 @@ jobs: echo "MAX_ITERATIONS=${{ inputs.max_iterations || 50 }}" >> $GITHUB_ENV echo "SANDBOX_ENV_GITHUB_TOKEN=${{ secrets.GITHUB_TOKEN }}" >> $GITHUB_ENV - + # Set branch variables - echo "SOURCE_BRANCH=${{ inputs.source_branch }}" >> $GITHUB_ENV - # If source_branch is specified but target_branch isn't, use source_branch for both - if [ -n "${{ inputs.source_branch }}" ] && [ "${{ inputs.target_branch }}" == "main" ]; then - echo "TARGET_BRANCH=${{ inputs.source_branch }}" >> $GITHUB_ENV - else - echo "TARGET_BRANCH=${{ inputs.target_branch }}" >> $GITHUB_ENV - fi + echo "SOURCE_BRANCH=${{ inputs.target_branch }}" >> $GITHUB_ENV - name: Comment on issue with start message uses: actions/github-script@v7 @@ -195,7 +184,6 @@ jobs: --issue-type ${{ env.ISSUE_TYPE }} \ --max-iterations ${{ env.MAX_ITERATIONS }} \ --comment-id ${{ env.COMMENT_ID }} \ - --source-branch ${{ env.SOURCE_BRANCH }} \ --target-branch ${{ env.TARGET_BRANCH }} - name: Check resolution result diff --git a/openhands/resolver/resolve_issue.py b/openhands/resolver/resolve_issue.py index d4ba5d71a443..b6cbb9f03c05 100644 --- a/openhands/resolver/resolve_issue.py +++ b/openhands/resolver/resolve_issue.py @@ -161,8 +161,6 @@ async def process_issue( issue_handler: IssueHandlerInterface, repo_instruction: str | None = None, reset_logger: bool = False, - source_branch: str | None = None, - target_branch: str | None = None, ) -> ResolverOutput: # Setup the logger properly, so you can run multi-processing to parallelize processing if reset_logger: @@ -317,7 +315,6 @@ async def resolve_issue( repo_instruction: str | None, issue_number: int, comment_id: int | None, - source_branch: str | None = None, target_branch: str | None = None, reset_logger: bool = False, ) -> None: @@ -337,7 +334,6 @@ async def resolve_issue( repo_instruction: Repository instruction to use. issue_number: Issue number to resolve. comment_id: Optional ID of a specific comment to focus on. - source_branch: Optional source branch to pull from (for PRs). target_branch: Optional target branch to create PR against (for PRs). reset_logger: Whether to reset the logger for multiprocessing. """ @@ -428,9 +424,9 @@ async def resolve_issue( try: # checkout to pr branch if needed if issue_type == 'pr': - branch_to_use = source_branch if source_branch else issue.head_branch + branch_to_use = target_branch if target_branch else issue.head_branch logger.info( - f'Checking out to PR branch {branch_to_use} for issue {issue.number}' + f'Checking out to PR branch {target_branch} for issue {issue.number}' ) if not branch_to_use: @@ -450,10 +446,6 @@ async def resolve_issue( cwd=repo_dir, ) - # Update issue's head_branch if using custom source branch - if source_branch: - issue.head_branch = source_branch - # Update issue's base_branch if using custom target branch if target_branch: issue.base_branch = target_branch @@ -475,8 +467,6 @@ async def resolve_issue( issue_handler, repo_instruction, reset_logger, - source_branch, - target_branch, ) output_fp.write(output.model_dump_json() + '\n') output_fp.flush() @@ -582,17 +572,11 @@ def int_or_none(value): choices=['issue', 'pr'], help='Type of issue to resolve, either open issue or pr comments.', ) - parser.add_argument( - '--source-branch', - type=str, - default=None, - help="Source branch to pull from (for PRs). If not specified, uses the PR's head branch.", - ) parser.add_argument( '--target-branch', type=str, default=None, - help="Target branch to create PR against (for PRs). If not specified, uses the PR's base branch.", + help="Target branch to pull and create PR against (for PRs). If not specified, uses the PR's base branch.", ) my_args = parser.parse_args() @@ -654,7 +638,6 @@ def int_or_none(value): repo_instruction=repo_instruction, issue_number=my_args.issue_number, comment_id=my_args.comment_id, - source_branch=my_args.source_branch, target_branch=my_args.target_branch, ) ) From d1b107d058740c618066befdf675a6c1f8936c69 Mon Sep 17 00:00:00 2001 From: Graham Neubig Date: Fri, 22 Nov 2024 15:20:04 -0500 Subject: [PATCH 7/7] Update .github/workflows/openhands-resolver.yml --- .github/workflows/openhands-resolver.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/openhands-resolver.yml b/.github/workflows/openhands-resolver.yml index 767dd7bb71a7..4692413d3fd9 100644 --- a/.github/workflows/openhands-resolver.yml +++ b/.github/workflows/openhands-resolver.yml @@ -141,7 +141,7 @@ jobs: echo "SANDBOX_ENV_GITHUB_TOKEN=${{ secrets.GITHUB_TOKEN }}" >> $GITHUB_ENV # Set branch variables - echo "SOURCE_BRANCH=${{ inputs.target_branch }}" >> $GITHUB_ENV + echo "TARGET_BRANCH=${{ inputs.target_branch }}" >> $GITHUB_ENV - name: Comment on issue with start message uses: actions/github-script@v7