From ca64c69b4a257d703e68ff44f9b4e4f435eabcfb Mon Sep 17 00:00:00 2001 From: young010101 <93481273+young010101@users.noreply.github.com> Date: Tue, 19 Nov 2024 10:45:06 +0800 Subject: [PATCH 01/11] Docs update runtime link (#5117) --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index e69538d21e0b..847b6c469812 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -54,7 +54,7 @@ The agent needs a place to run code and commands. When you run OpenHands on your to do this by default. But there are other ways of creating a sandbox for the agent. If you work for a company that provides a cloud-based runtime, you could help us add support for that runtime -by implementing the [interface specified here](https://github.com/All-Hands-AI/OpenHands/blob/main/openhands/runtime/runtime.py). +by implementing the [interface specified here](https://github.com/All-Hands-AI/OpenHands/blob/main/openhands/runtime/base.py). #### Testing When you write code, it is also good to write tests. Please navigate to the `tests` folder to see existing test suites. From 2c580387c56273be95280ffb245a430a377cc339 Mon Sep 17 00:00:00 2001 From: Raymond Xu Date: Tue, 19 Nov 2024 04:16:29 -0800 Subject: [PATCH 02/11] Allow to merge to a specific target branch instead of main (#5109) --- openhands/resolver/send_pull_request.py | 32 +++++-- tests/unit/resolver/test_send_pull_request.py | 91 ++++++++++++++++--- 2 files changed, 101 insertions(+), 22 deletions(-) diff --git a/openhands/resolver/send_pull_request.py b/openhands/resolver/send_pull_request.py index eade7fcfc419..8a9d6118bedd 100644 --- a/openhands/resolver/send_pull_request.py +++ b/openhands/resolver/send_pull_request.py @@ -203,6 +203,7 @@ def send_pull_request( pr_type: str, fork_owner: str | None = None, additional_message: str | None = None, + target_branch: str | None = None, ) -> str: if pr_type not in ['branch', 'draft', 'ready']: raise ValueError(f'Invalid pr_type: {pr_type}') @@ -224,12 +225,19 @@ def send_pull_request( attempt += 1 branch_name = f'{base_branch_name}-try{attempt}' - # Get the default branch - print('Getting default branch...') - response = requests.get(f'{base_url}', headers=headers) - response.raise_for_status() - default_branch = response.json()['default_branch'] - print(f'Default branch: {default_branch}') + # Get the default branch or use specified target branch + print('Getting base branch...') + if target_branch: + base_branch = target_branch + # Verify the target branch exists + response = requests.get(f'{base_url}/branches/{target_branch}', headers=headers) + if response.status_code != 200: + raise ValueError(f'Target branch {target_branch} does not exist') + else: + response = requests.get(f'{base_url}', headers=headers) + response.raise_for_status() + base_branch = response.json()['default_branch'] + print(f'Base branch: {base_branch}') # Create and checkout the new branch print('Creating new branch...') @@ -279,7 +287,7 @@ def send_pull_request( 'title': pr_title, # No need to escape title for GitHub API 'body': pr_body, 'head': branch_name, - 'base': default_branch, + 'base': base_branch, 'draft': pr_type == 'draft', } response = requests.post(f'{base_url}/pulls', headers=headers, json=data) @@ -435,6 +443,7 @@ def process_single_issue( llm_config: LLMConfig, fork_owner: str | None, send_on_failure: bool, + target_branch: str | None = None, ) -> None: if not resolver_output.success and not send_on_failure: print( @@ -484,6 +493,7 @@ def process_single_issue( llm_config=llm_config, fork_owner=fork_owner, additional_message=resolver_output.success_explanation, + target_branch=target_branch, ) @@ -508,6 +518,7 @@ def process_all_successful_issues( llm_config, fork_owner, False, + None, ) @@ -573,6 +584,12 @@ def main(): default=None, help='Base URL for the LLM model.', ) + parser.add_argument( + '--target-branch', + type=str, + default=None, + help='Target branch to create the pull request against (defaults to repository default branch)', + ) my_args = parser.parse_args() github_token = ( @@ -625,6 +642,7 @@ def main(): llm_config, my_args.fork_owner, my_args.send_on_failure, + my_args.target_branch, ) diff --git a/tests/unit/resolver/test_send_pull_request.py b/tests/unit/resolver/test_send_pull_request.py index 951be1af006c..f83e2e97ec2f 100644 --- a/tests/unit/resolver/test_send_pull_request.py +++ b/tests/unit/resolver/test_send_pull_request.py @@ -322,7 +322,17 @@ def test_update_existing_pull_request( ) -@pytest.mark.parametrize('pr_type', ['branch', 'draft', 'ready']) +@pytest.mark.parametrize( + 'pr_type,target_branch', + [ + ('branch', None), + ('draft', None), + ('ready', None), + ('branch', 'feature'), + ('draft', 'develop'), + ('ready', 'staging'), + ], +) @patch('subprocess.run') @patch('requests.post') @patch('requests.get') @@ -334,14 +344,22 @@ def test_send_pull_request( mock_output_dir, mock_llm_config, pr_type, + target_branch, ): repo_path = os.path.join(mock_output_dir, 'repo') - # Mock API responses - mock_get.side_effect = [ - MagicMock(status_code=404), # Branch doesn't exist - MagicMock(json=lambda: {'default_branch': 'main'}), - ] + # Mock API responses based on whether target_branch is specified + if target_branch: + mock_get.side_effect = [ + MagicMock(status_code=404), # Branch doesn't exist + MagicMock(status_code=200), # Target branch exists + ] + else: + mock_get.side_effect = [ + MagicMock(status_code=404), # 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' } @@ -360,10 +378,12 @@ def test_send_pull_request( patch_dir=repo_path, pr_type=pr_type, llm_config=mock_llm_config, + target_branch=target_branch, ) # Assert API calls - assert mock_get.call_count == 2 + expected_get_calls = 2 + assert mock_get.call_count == expected_get_calls # Check branch creation and push assert mock_run.call_count == 2 @@ -401,10 +421,41 @@ 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 if target_branch else 'main') assert post_data['draft'] == (pr_type == 'draft') +@patch('requests.get') +def test_send_pull_request_invalid_target_branch( + mock_get, mock_github_issue, mock_output_dir, mock_llm_config +): + """Test that an error is raised when specifying a non-existent target branch""" + repo_path = os.path.join(mock_output_dir, 'repo') + + # Mock API response for non-existent branch + mock_get.side_effect = [ + MagicMock(status_code=404), # Branch doesn't exist + MagicMock(status_code=404), # Target branch doesn't exist + ] + + # Test that ValueError is raised when target branch doesn't exist + with pytest.raises( + ValueError, match='Target branch nonexistent-branch does not exist' + ): + send_pull_request( + github_issue=mock_github_issue, + github_token='test-token', + github_username='test-user', + patch_dir=repo_path, + pr_type='ready', + llm_config=mock_llm_config, + target_branch='nonexistent-branch', + ) + + # Verify API calls + assert mock_get.call_count == 2 + + @patch('subprocess.run') @patch('requests.post') @patch('requests.get') @@ -616,6 +667,7 @@ def test_process_single_pr_update( mock_llm_config, None, False, + None, ) mock_initialize_repo.assert_called_once_with(mock_output_dir, 1, 'pr', 'branch 1') @@ -688,6 +740,7 @@ def test_process_single_issue( mock_llm_config, None, False, + None, ) # Assert that the mocked functions were called with correct arguments @@ -704,9 +757,10 @@ def test_process_single_issue( github_username=github_username, patch_dir=f'{mock_output_dir}/patches/issue_1', pr_type=pr_type, + llm_config=mock_llm_config, fork_owner=None, additional_message=resolver_output.success_explanation, - llm_config=mock_llm_config, + target_branch=None, ) @@ -757,6 +811,7 @@ def test_process_single_issue_unsuccessful( mock_llm_config, None, False, + None, ) # Assert that none of the mocked functions were called @@ -863,6 +918,7 @@ def test_process_all_successful_issues( mock_llm_config, None, False, + None, ), call( 'output_dir', @@ -873,6 +929,7 @@ def test_process_all_successful_issues( mock_llm_config, None, False, + None, ), ] ) @@ -971,6 +1028,7 @@ def test_main( mock_args.llm_model = 'mock_model' mock_args.llm_base_url = 'mock_url' mock_args.llm_api_key = 'mock_key' + mock_args.target_branch = None mock_parser.return_value.parse_args.return_value = mock_args # Setup environment variables @@ -994,12 +1052,8 @@ def test_main( 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( + # Use any_call instead of assert_called_with for more flexible matching + assert mock_process_single_issue.call_args == call( '/mock/output', mock_resolver_output, 'mock_token', @@ -1008,8 +1062,15 @@ def test_main( llm_config, None, False, + mock_args.target_branch, ) + # Other assertions + 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) + # Test for 'all_successful' issue number mock_args.issue_number = 'all_successful' main() From 1f723293db928ce665e7081da20603bd8e0469ef Mon Sep 17 00:00:00 2001 From: Rohit Malhotra Date: Tue, 19 Nov 2024 08:34:25 -0500 Subject: [PATCH 03/11] Add macro invocations to example workflow (#5121) --- .../resolver/examples/openhands-resolver.yml | 24 ++++++++++++++----- 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/openhands/resolver/examples/openhands-resolver.yml b/openhands/resolver/examples/openhands-resolver.yml index 6555e15057c7..fb40dde8d97e 100644 --- a/openhands/resolver/examples/openhands-resolver.yml +++ b/openhands/resolver/examples/openhands-resolver.yml @@ -7,6 +7,10 @@ on: types: [labeled] issue_comment: types: [created] + pull_request_review_comment: + types: [created] + pull_request_review: + types: [submitted] permissions: contents: write @@ -16,12 +20,20 @@ permissions: jobs: call-openhands-resolver: if: | - ${{ - github.event.label.name == 'fix-me' || - (github.event_name == 'issue_comment' && - startsWith(github.event.comment.body, vars.OPENHANDS_MACRO || '@openhands-agent') && - (github.event.comment.author_association == 'OWNER' || github.event.comment.author_association == 'COLLABORATOR' || github.event.comment.author_association == 'MEMBER')) - }} + github.event.label.name == 'fix-me' || + + ( + ((github.event_name == 'issue_comment' || github.event_name == 'pull_request_review_comment') && + (startsWith(github.event.comment.body, inputs.macro || '@openhands-agent') || startsWith(github.event.comment.body, inputs.macro || vars.OPENHANDS_MACRO)) && + (github.event.comment.author_association == 'OWNER' || github.event.comment.author_association == 'COLLABORATOR' || github.event.comment.author_association == 'MEMBER') + ) || + + (github.event_name == 'pull_request_review' && + (startsWith(github.event.review.body, inputs.macro || '@openhands-agent') || startsWith(github.event.review.body, inputs.macro || vars.OPENHANDS_MACRO)) && + (github.event.review.author_association == 'OWNER' || github.event.review.author_association == 'COLLABORATOR' || github.event.review.author_association == 'MEMBER') + ) + ) + uses: All-Hands-AI/OpenHands/.github/workflows/openhands-resolver.yml@main with: macro: ${{ vars.OPENHANDS_MACRO || '@openhands-agent' }} From ff84a3eede3d2f4e52e25822d525d676193abc78 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Tue, 19 Nov 2024 10:41:27 -0600 Subject: [PATCH 04/11] chore: remove specified sid (#5127) --- evaluation/discoverybench/run_infer.py | 5 +---- openhands/core/main.py | 5 ++++- openhands/resolver/resolve_issue.py | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/evaluation/discoverybench/run_infer.py b/evaluation/discoverybench/run_infer.py index 1b3a98c37e14..7cfd2dbac7ad 100644 --- a/evaluation/discoverybench/run_infer.py +++ b/evaluation/discoverybench/run_infer.py @@ -250,9 +250,6 @@ def process_instance( config = get_config(metadata) - # use a session id for concurrent evaluation - sid = 'ID_' + str(instance.instance_id) - # Setup the logger properly, so you can run # multi-processing to parallelize the evaluation if reset_logger: @@ -284,7 +281,7 @@ def process_instance( instruction += AGENT_CLS_TO_INST_SUFFIX[metadata.agent_class] # Here's how you can run the agent (similar to the `main` function) and get the final task state - runtime = create_runtime(config, sid=sid) + runtime = create_runtime(config) call_async_from_sync(runtime.connect) initialize_runtime(runtime, instance.data_files) diff --git a/openhands/core/main.py b/openhands/core/main.py index 06dede3d5d55..d55aa0175102 100644 --- a/openhands/core/main.py +++ b/openhands/core/main.py @@ -59,7 +59,8 @@ def create_runtime( """Create a runtime for the agent to run on. config: The app config. - sid: The session id. + sid: (optional) The session id. IMPORTANT: please don't set this unless you know what you're doing. + Set it to incompatible value will cause unexpected behavior on RemoteRuntime. headless_mode: Whether the agent is run in headless mode. `create_runtime` is typically called within evaluation scripts, where we don't want to have the VSCode UI open, so it defaults to True. """ @@ -105,6 +106,8 @@ async def run_controller( Args: config: The app config. initial_user_action: An Action object containing initial user input + sid: (optional) The session id. IMPORTANT: please don't set this unless you know what you're doing. + Set it to incompatible value will cause unexpected behavior on RemoteRuntime. runtime: (optional) A runtime for the agent to run on. agent: (optional) A agent to run. exit_on_message: quit if agent asks for a message from user (optional) diff --git a/openhands/resolver/resolve_issue.py b/openhands/resolver/resolve_issue.py index 67eb20bee1e0..cbe8cfa6df61 100644 --- a/openhands/resolver/resolve_issue.py +++ b/openhands/resolver/resolve_issue.py @@ -199,7 +199,7 @@ async def process_issue( ) config.set_llm_config(llm_config) - runtime = create_runtime(config, sid=f'{issue.number}') + runtime = create_runtime(config) await runtime.connect() async def on_event(evt): From de07fcfddcc0ceb28ac1d31e739f65191e075dd9 Mon Sep 17 00:00:00 2001 From: Rohit Malhotra Date: Tue, 19 Nov 2024 12:17:55 -0500 Subject: [PATCH 05/11] Moving resolver settings to repo variables (#5130) --- openhands/resolver/examples/openhands-resolver.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openhands/resolver/examples/openhands-resolver.yml b/openhands/resolver/examples/openhands-resolver.yml index fb40dde8d97e..2e2f42be0bac 100644 --- a/openhands/resolver/examples/openhands-resolver.yml +++ b/openhands/resolver/examples/openhands-resolver.yml @@ -37,7 +37,7 @@ jobs: uses: All-Hands-AI/OpenHands/.github/workflows/openhands-resolver.yml@main with: macro: ${{ vars.OPENHANDS_MACRO || '@openhands-agent' }} - max_iterations: 50 + max_iterations: ${{ vars.OPENHANDS_MAX_ITER || 50 }} secrets: PAT_TOKEN: ${{ secrets.PAT_TOKEN }} PAT_USERNAME: ${{ secrets.PAT_USERNAME }} From 7f5022c8fe9238af4d2818782601f4ea4452e827 Mon Sep 17 00:00:00 2001 From: Rohit Malhotra Date: Tue, 19 Nov 2024 12:23:42 -0500 Subject: [PATCH 06/11] Refactor issue filtering (#5129) --- openhands/resolver/issue_definitions.py | 35 +- openhands/resolver/resolve_all_issues.py | 7 +- openhands/resolver/resolve_issue.py | 7 +- tests/unit/resolver/test_issue_handler.py | 425 +++++++++++---------- tests/unit/resolver/test_resolve_issues.py | 10 +- 5 files changed, 256 insertions(+), 228 deletions(-) diff --git a/openhands/resolver/issue_definitions.py b/openhands/resolver/issue_definitions.py index c9e386178adf..a0d0eb570aa7 100644 --- a/openhands/resolver/issue_definitions.py +++ b/openhands/resolver/issue_definitions.py @@ -18,7 +18,9 @@ class IssueHandlerInterface(ABC): issue_type: ClassVar[str] @abstractmethod - def get_converted_issues(self, comment_id: int | None = None) -> list[GithubIssue]: + def get_converted_issues( + self, issue_numbers: list[int] | None = None, comment_id: int | None = None + ) -> list[GithubIssue]: """Download issues from GitHub.""" pass @@ -138,13 +140,29 @@ def _get_issue_comments( return all_comments if all_comments else None - def get_converted_issues(self, comment_id: int | None = None) -> list[GithubIssue]: + def get_converted_issues( + self, issue_numbers: list[int] | None = None, comment_id: int | None = None + ) -> list[GithubIssue]: """Download issues from Github. Returns: List of Github issues. """ + + if not issue_numbers: + raise ValueError('Unspecified issue number') + all_issues = self._download_issues_from_github() + logger.info(f'Limiting resolving to issues {issue_numbers}.') + all_issues = [ + issue + for issue in all_issues + if issue['number'] in issue_numbers and 'pull_request' not in issue + ] + + if len(issue_numbers) == 1 and not all_issues: + raise ValueError(f'Issue {issue_numbers[0]} not found') + converted_issues = [] for issue in all_issues: if any([issue.get(key) is None for key in ['number', 'title', 'body']]): @@ -153,9 +171,6 @@ def get_converted_issues(self, comment_id: int | None = None) -> list[GithubIssu ) continue - if 'pull_request' in issue: - continue - # Get issue thread comments thread_comments = self._get_issue_comments( issue['number'], comment_id=comment_id @@ -486,8 +501,16 @@ def __get_context_from_external_issues_references( return closing_issues - def get_converted_issues(self, comment_id: int | None = None) -> list[GithubIssue]: + def get_converted_issues( + self, issue_numbers: list[int] | None = None, comment_id: int | None = None + ) -> list[GithubIssue]: + if not issue_numbers: + raise ValueError('Unspecified issue numbers') + all_issues = self._download_issues_from_github() + logger.info(f'Limiting resolving to issues {issue_numbers}.') + all_issues = [issue for issue in all_issues if issue['number'] in issue_numbers] + converted_issues = [] for issue in all_issues: # For PRs, body can be None diff --git a/openhands/resolver/resolve_all_issues.py b/openhands/resolver/resolve_all_issues.py index a561b24a61a7..01d076446e97 100644 --- a/openhands/resolver/resolve_all_issues.py +++ b/openhands/resolver/resolve_all_issues.py @@ -83,11 +83,10 @@ async def resolve_issues( issue_handler = issue_handler_factory(issue_type, owner, repo, token) # Load dataset - issues: list[GithubIssue] = issue_handler.get_converted_issues() + issues: list[GithubIssue] = issue_handler.get_converted_issues( + issue_numbers=issue_numbers + ) - if issue_numbers is not None: - issues = [issue for issue in issues if issue.number in issue_numbers] - logger.info(f'Limiting resolving to issues {issue_numbers}.') if limit_issues is not None: issues = issues[:limit_issues] logger.info(f'Limiting resolving to first {limit_issues} issues.') diff --git a/openhands/resolver/resolve_issue.py b/openhands/resolver/resolve_issue.py index cbe8cfa6df61..b371d8160b20 100644 --- a/openhands/resolver/resolve_issue.py +++ b/openhands/resolver/resolve_issue.py @@ -339,13 +339,10 @@ async def resolve_issue( # Load dataset issues: list[GithubIssue] = issue_handler.get_converted_issues( - comment_id=comment_id + issue_numbers=[issue_number], comment_id=comment_id ) - # Find the specific issue - issue = next((i for i in issues if i.number == issue_number), None) - if not issue: - raise ValueError(f'Issue {issue_number} not found') + issue = issues[0] if comment_id is not None: if ( diff --git a/tests/unit/resolver/test_issue_handler.py b/tests/unit/resolver/test_issue_handler.py index 7df7f0fe4027..d0c17d9088e9 100644 --- a/tests/unit/resolver/test_issue_handler.py +++ b/tests/unit/resolver/test_issue_handler.py @@ -1,17 +1,18 @@ -from unittest.mock import patch, MagicMock -from openhands.resolver.issue_definitions import IssueHandler, PRHandler -from openhands.resolver.github_issue import GithubIssue, ReviewThread -from openhands.events.action.message import MessageAction +from unittest.mock import MagicMock, patch + from openhands.core.config import LLMConfig +from openhands.events.action.message import MessageAction +from openhands.resolver.github_issue import GithubIssue, ReviewThread +from openhands.resolver.issue_definitions import IssueHandler, PRHandler def test_get_converted_issues_initializes_review_comments(): # Mock the necessary dependencies - with patch("requests.get") as mock_get: + with patch('requests.get') as mock_get: # Mock the response for issues mock_issues_response = MagicMock() mock_issues_response.json.return_value = [ - {"number": 1, "title": "Test Issue", "body": "Test Body"} + {'number': 1, 'title': 'Test Issue', 'body': 'Test Body'} ] # Mock the response for comments mock_comments_response = MagicMock() @@ -26,10 +27,10 @@ def test_get_converted_issues_initializes_review_comments(): ] # Need two comment responses because we make two API calls # Create an instance of IssueHandler - handler = IssueHandler("test-owner", "test-repo", "test-token") + handler = IssueHandler('test-owner', 'test-repo', 'test-token') # Get converted issues - issues = handler.get_converted_issues() + issues = handler.get_converted_issues(issue_numbers=[1]) # Verify that we got exactly one issue assert len(issues) == 1 @@ -39,35 +40,35 @@ def test_get_converted_issues_initializes_review_comments(): # Verify other fields are set correctly assert issues[0].number == 1 - assert issues[0].title == "Test Issue" - assert issues[0].body == "Test Body" - assert issues[0].owner == "test-owner" - assert issues[0].repo == "test-repo" + assert issues[0].title == 'Test Issue' + assert issues[0].body == 'Test Body' + assert issues[0].owner == 'test-owner' + assert issues[0].repo == 'test-repo' def test_pr_handler_guess_success_with_thread_comments(): # Create a PR handler instance - handler = PRHandler("test-owner", "test-repo", "test-token") + handler = PRHandler('test-owner', 'test-repo', 'test-token') # Create a mock issue with thread comments but no review comments issue = GithubIssue( - owner="test-owner", - repo="test-repo", + owner='test-owner', + repo='test-repo', number=1, - title="Test PR", - body="Test Body", - thread_comments=["First comment", "Second comment"], - closing_issues=["Issue description"], + title='Test PR', + body='Test Body', + thread_comments=['First comment', 'Second comment'], + closing_issues=['Issue description'], review_comments=None, thread_ids=None, - head_branch="test-branch", + head_branch='test-branch', ) # Create mock history - history = [MessageAction(content="Fixed the issue by implementing X and Y")] + history = [MessageAction(content='Fixed the issue by implementing X and Y')] # Create mock LLM config - llm_config = LLMConfig(model="test-model", api_key="test-key") + llm_config = LLMConfig(model='test-model', api_key='test-key') # Mock the LLM response mock_response = MagicMock() @@ -84,7 +85,7 @@ def test_pr_handler_guess_success_with_thread_comments(): ] # Test the guess_success method - with patch("litellm.completion", return_value=mock_response): + with patch('litellm.completion', return_value=mock_response): success, success_list, explanation = handler.guess_success( issue, history, llm_config ) @@ -92,39 +93,39 @@ def test_pr_handler_guess_success_with_thread_comments(): # Verify the results assert success is True assert success_list == [True] - assert "successfully address" in explanation + assert 'successfully address' in explanation def test_pr_handler_get_converted_issues_with_comments(): # Mock the necessary dependencies - with patch("requests.get") as mock_get: + with patch('requests.get') as mock_get: # Mock the response for PRs mock_prs_response = MagicMock() mock_prs_response.json.return_value = [ { - "number": 1, - "title": "Test PR", - "body": "Test Body fixes #1", - "head": {"ref": "test-branch"}, + 'number': 1, + 'title': 'Test PR', + 'body': 'Test Body fixes #1', + 'head': {'ref': 'test-branch'}, } ] # Mock the response for PR comments mock_comments_response = MagicMock() mock_comments_response.json.return_value = [ - {"body": "First comment"}, - {"body": "Second comment"}, + {'body': 'First comment'}, + {'body': 'Second comment'}, ] # Mock the response for PR metadata (GraphQL) mock_graphql_response = MagicMock() mock_graphql_response.json.return_value = { - "data": { - "repository": { - "pullRequest": { - "closingIssuesReferences": {"edges": []}, - "reviews": {"nodes": []}, - "reviewThreads": {"edges": []}, + 'data': { + 'repository': { + 'pullRequest': { + 'closingIssuesReferences': {'edges': []}, + 'reviews': {'nodes': []}, + 'reviewThreads': {'edges': []}, } } } @@ -138,7 +139,7 @@ def test_pr_handler_get_converted_issues_with_comments(): # Mock the response for fetching the external issue referenced in PR body mock_external_issue_response = MagicMock() mock_external_issue_response.json.return_value = { - "body": "This is additional context from an externally referenced issue." + 'body': 'This is additional context from an externally referenced issue.' } mock_get.side_effect = [ @@ -150,56 +151,56 @@ def test_pr_handler_get_converted_issues_with_comments(): ] # Mock the post request for GraphQL - with patch("requests.post") as mock_post: + with patch('requests.post') as mock_post: mock_post.return_value = mock_graphql_response # Create an instance of PRHandler - handler = PRHandler("test-owner", "test-repo", "test-token") + handler = PRHandler('test-owner', 'test-repo', 'test-token') # Get converted issues - prs = handler.get_converted_issues() + prs = handler.get_converted_issues(issue_numbers=[1]) # Verify that we got exactly one PR assert len(prs) == 1 # Verify that thread_comments are set correctly - assert prs[0].thread_comments == ["First comment", "Second comment"] + assert prs[0].thread_comments == ['First comment', 'Second comment'] # Verify other fields are set correctly assert prs[0].number == 1 - assert prs[0].title == "Test PR" - assert prs[0].body == "Test Body fixes #1" - assert prs[0].owner == "test-owner" - assert prs[0].repo == "test-repo" - assert prs[0].head_branch == "test-branch" + assert prs[0].title == 'Test PR' + assert prs[0].body == 'Test Body fixes #1' + assert prs[0].owner == 'test-owner' + assert prs[0].repo == 'test-repo' + assert prs[0].head_branch == 'test-branch' assert prs[0].closing_issues == [ - "This is additional context from an externally referenced issue." + 'This is additional context from an externally referenced issue.' ] def test_pr_handler_guess_success_only_review_comments(): # Create a PR handler instance - handler = PRHandler("test-owner", "test-repo", "test-token") + handler = PRHandler('test-owner', 'test-repo', 'test-token') # Create a mock issue with only review comments issue = GithubIssue( - owner="test-owner", - repo="test-repo", + owner='test-owner', + repo='test-repo', number=1, - title="Test PR", - body="Test Body", + title='Test PR', + body='Test Body', thread_comments=None, - closing_issues=["Issue description"], - review_comments=["Please fix the formatting", "Add more tests"], + closing_issues=['Issue description'], + review_comments=['Please fix the formatting', 'Add more tests'], thread_ids=None, - head_branch="test-branch", + head_branch='test-branch', ) # Create mock history - history = [MessageAction(content="Fixed the formatting and added more tests")] + history = [MessageAction(content='Fixed the formatting and added more tests')] # Create mock LLM config - llm_config = LLMConfig(model="test-model", api_key="test-key") + llm_config = LLMConfig(model='test-model', api_key='test-key') # Mock the LLM response mock_response = MagicMock() @@ -216,7 +217,7 @@ def test_pr_handler_guess_success_only_review_comments(): ] # Test the guess_success method - with patch("litellm.completion", return_value=mock_response): + with patch('litellm.completion', return_value=mock_response): success, success_list, explanation = handler.guess_success( issue, history, llm_config ) @@ -224,32 +225,32 @@ def test_pr_handler_guess_success_only_review_comments(): # Verify the results assert success is True assert success_list == [True] - assert "successfully address" in explanation + assert 'successfully address' in explanation def test_pr_handler_guess_success_no_comments(): # Create a PR handler instance - handler = PRHandler("test-owner", "test-repo", "test-token") + handler = PRHandler('test-owner', 'test-repo', 'test-token') # Create a mock issue with no comments issue = GithubIssue( - owner="test-owner", - repo="test-repo", + owner='test-owner', + repo='test-repo', number=1, - title="Test PR", - body="Test Body", + title='Test PR', + body='Test Body', thread_comments=None, - closing_issues=["Issue description"], + closing_issues=['Issue description'], review_comments=None, thread_ids=None, - head_branch="test-branch", + head_branch='test-branch', ) # Create mock history - history = [MessageAction(content="Fixed the issue")] + history = [MessageAction(content='Fixed the issue')] # Create mock LLM config - llm_config = LLMConfig(model="test-model", api_key="test-key") + llm_config = LLMConfig(model='test-model', api_key='test-key') # Test that it returns appropriate message when no comments are present success, success_list, explanation = handler.guess_success( @@ -257,29 +258,29 @@ def test_pr_handler_guess_success_no_comments(): ) assert success is False assert success_list is None - assert explanation == "No feedback was found to process" + assert explanation == 'No feedback was found to process' def test_get_issue_comments_with_specific_comment_id(): # Mock the necessary dependencies - with patch("requests.get") as mock_get: + with patch('requests.get') as mock_get: # Mock the response for comments mock_comments_response = MagicMock() mock_comments_response.json.return_value = [ - {"id": 123, "body": "First comment"}, - {"id": 456, "body": "Second comment"}, + {'id': 123, 'body': 'First comment'}, + {'id': 456, 'body': 'Second comment'}, ] mock_get.return_value = mock_comments_response # Create an instance of IssueHandler - handler = IssueHandler("test-owner", "test-repo", "test-token") + handler = IssueHandler('test-owner', 'test-repo', 'test-token') # Get comments with a specific comment_id specific_comment = handler._get_issue_comments(issue_number=1, comment_id=123) # Verify only the specific comment is returned - assert specific_comment == ["First comment"] + assert specific_comment == ['First comment'] def test_pr_handler_get_converted_issues_with_specific_thread_comment(): @@ -287,50 +288,50 @@ def test_pr_handler_get_converted_issues_with_specific_thread_comment(): specific_comment_id = 123 # Mock GraphQL response for review threads - with patch("requests.get") as mock_get: + with patch('requests.get') as mock_get: # Mock the response for PRs mock_prs_response = MagicMock() mock_prs_response.json.return_value = [ { - "number": 1, - "title": "Test PR", - "body": "Test Body", - "head": {"ref": "test-branch"}, + 'number': 1, + 'title': 'Test PR', + 'body': 'Test Body', + 'head': {'ref': 'test-branch'}, } ] # Mock the response for PR comments mock_comments_response = MagicMock() mock_comments_response.json.return_value = [ - {"body": "First comment", "id": 123}, - {"body": "Second comment", "id": 124}, + {'body': 'First comment', 'id': 123}, + {'body': 'Second comment', 'id': 124}, ] # Mock the response for PR metadata (GraphQL) mock_graphql_response = MagicMock() mock_graphql_response.json.return_value = { - "data": { - "repository": { - "pullRequest": { - "closingIssuesReferences": {"edges": []}, - "reviews": {"nodes": []}, - "reviewThreads": { - "edges": [ + 'data': { + 'repository': { + 'pullRequest': { + 'closingIssuesReferences': {'edges': []}, + 'reviews': {'nodes': []}, + 'reviewThreads': { + 'edges': [ { - "node": { - "id": "review-thread-1", - "isResolved": False, - "comments": { - "nodes": [ + 'node': { + 'id': 'review-thread-1', + 'isResolved': False, + 'comments': { + 'nodes': [ { - "fullDatabaseId": 121, - "body": "Specific review comment", - "path": "file1.txt", + 'fullDatabaseId': 121, + 'body': 'Specific review comment', + 'path': 'file1.txt', }, { - "fullDatabaseId": 456, - "body": "Another review comment", - "path": "file2.txt", + 'fullDatabaseId': 456, + 'body': 'Another review comment', + 'path': 'file2.txt', }, ] }, @@ -356,30 +357,32 @@ def test_pr_handler_get_converted_issues_with_specific_thread_comment(): ] # Mock the post request for GraphQL - with patch("requests.post") as mock_post: + with patch('requests.post') as mock_post: mock_post.return_value = mock_graphql_response # Create an instance of PRHandler - handler = PRHandler("test-owner", "test-repo", "test-token") + handler = PRHandler('test-owner', 'test-repo', 'test-token') # Get converted issues - prs = handler.get_converted_issues(comment_id=specific_comment_id) + prs = handler.get_converted_issues( + issue_numbers=[1], comment_id=specific_comment_id + ) # Verify that we got exactly one PR assert len(prs) == 1 # Verify that thread_comments are set correctly - assert prs[0].thread_comments == ["First comment"] + assert prs[0].thread_comments == ['First comment'] assert prs[0].review_comments == [] assert prs[0].review_threads == [] # Verify other fields are set correctly assert prs[0].number == 1 - assert prs[0].title == "Test PR" - assert prs[0].body == "Test Body" - assert prs[0].owner == "test-owner" - assert prs[0].repo == "test-repo" - assert prs[0].head_branch == "test-branch" + assert prs[0].title == 'Test PR' + assert prs[0].body == 'Test Body' + assert prs[0].owner == 'test-owner' + assert prs[0].repo == 'test-repo' + assert prs[0].head_branch == 'test-branch' def test_pr_handler_get_converted_issues_with_specific_review_thread_comment(): @@ -387,50 +390,50 @@ def test_pr_handler_get_converted_issues_with_specific_review_thread_comment(): specific_comment_id = 123 # Mock GraphQL response for review threads - with patch("requests.get") as mock_get: + with patch('requests.get') as mock_get: # Mock the response for PRs mock_prs_response = MagicMock() mock_prs_response.json.return_value = [ { - "number": 1, - "title": "Test PR", - "body": "Test Body", - "head": {"ref": "test-branch"}, + 'number': 1, + 'title': 'Test PR', + 'body': 'Test Body', + 'head': {'ref': 'test-branch'}, } ] # Mock the response for PR comments mock_comments_response = MagicMock() mock_comments_response.json.return_value = [ - {"body": "First comment", "id": 120}, - {"body": "Second comment", "id": 124}, + {'body': 'First comment', 'id': 120}, + {'body': 'Second comment', 'id': 124}, ] # Mock the response for PR metadata (GraphQL) mock_graphql_response = MagicMock() mock_graphql_response.json.return_value = { - "data": { - "repository": { - "pullRequest": { - "closingIssuesReferences": {"edges": []}, - "reviews": {"nodes": []}, - "reviewThreads": { - "edges": [ + 'data': { + 'repository': { + 'pullRequest': { + 'closingIssuesReferences': {'edges': []}, + 'reviews': {'nodes': []}, + 'reviewThreads': { + 'edges': [ { - "node": { - "id": "review-thread-1", - "isResolved": False, - "comments": { - "nodes": [ + 'node': { + 'id': 'review-thread-1', + 'isResolved': False, + 'comments': { + 'nodes': [ { - "fullDatabaseId": specific_comment_id, - "body": "Specific review comment", - "path": "file1.txt", + 'fullDatabaseId': specific_comment_id, + 'body': 'Specific review comment', + 'path': 'file1.txt', }, { - "fullDatabaseId": 456, - "body": "Another review comment", - "path": "file1.txt", + 'fullDatabaseId': 456, + 'body': 'Another review comment', + 'path': 'file1.txt', }, ] }, @@ -456,14 +459,16 @@ def test_pr_handler_get_converted_issues_with_specific_review_thread_comment(): ] # Mock the post request for GraphQL - with patch("requests.post") as mock_post: + with patch('requests.post') as mock_post: mock_post.return_value = mock_graphql_response # Create an instance of PRHandler - handler = PRHandler("test-owner", "test-repo", "test-token") + handler = PRHandler('test-owner', 'test-repo', 'test-token') # Get converted issues - prs = handler.get_converted_issues(comment_id=specific_comment_id) + prs = handler.get_converted_issues( + issue_numbers=[1], comment_id=specific_comment_id + ) # Verify that we got exactly one PR assert len(prs) == 1 @@ -475,17 +480,17 @@ def test_pr_handler_get_converted_issues_with_specific_review_thread_comment(): assert isinstance(prs[0].review_threads[0], ReviewThread) assert ( prs[0].review_threads[0].comment - == "Specific review comment\n---\nlatest feedback:\nAnother review comment\n" + == 'Specific review comment\n---\nlatest feedback:\nAnother review comment\n' ) - assert prs[0].review_threads[0].files == ["file1.txt"] + assert prs[0].review_threads[0].files == ['file1.txt'] # Verify other fields are set correctly assert prs[0].number == 1 - assert prs[0].title == "Test PR" - assert prs[0].body == "Test Body" - assert prs[0].owner == "test-owner" - assert prs[0].repo == "test-repo" - assert prs[0].head_branch == "test-branch" + assert prs[0].title == 'Test PR' + assert prs[0].body == 'Test Body' + assert prs[0].owner == 'test-owner' + assert prs[0].repo == 'test-repo' + assert prs[0].head_branch == 'test-branch' def test_pr_handler_get_converted_issues_with_specific_comment_and_issue_refs(): @@ -493,50 +498,50 @@ def test_pr_handler_get_converted_issues_with_specific_comment_and_issue_refs(): specific_comment_id = 123 # Mock GraphQL response for review threads - with patch("requests.get") as mock_get: + with patch('requests.get') as mock_get: # Mock the response for PRs mock_prs_response = MagicMock() mock_prs_response.json.return_value = [ { - "number": 1, - "title": "Test PR fixes #3", - "body": "Test Body", - "head": {"ref": "test-branch"}, + 'number': 1, + 'title': 'Test PR fixes #3', + 'body': 'Test Body', + 'head': {'ref': 'test-branch'}, } ] # Mock the response for PR comments mock_comments_response = MagicMock() mock_comments_response.json.return_value = [ - {"body": "First comment", "id": 120}, - {"body": "Second comment", "id": 124}, + {'body': 'First comment', 'id': 120}, + {'body': 'Second comment', 'id': 124}, ] # Mock the response for PR metadata (GraphQL) mock_graphql_response = MagicMock() mock_graphql_response.json.return_value = { - "data": { - "repository": { - "pullRequest": { - "closingIssuesReferences": {"edges": []}, - "reviews": {"nodes": []}, - "reviewThreads": { - "edges": [ + 'data': { + 'repository': { + 'pullRequest': { + 'closingIssuesReferences': {'edges': []}, + 'reviews': {'nodes': []}, + 'reviewThreads': { + 'edges': [ { - "node": { - "id": "review-thread-1", - "isResolved": False, - "comments": { - "nodes": [ + 'node': { + 'id': 'review-thread-1', + 'isResolved': False, + 'comments': { + 'nodes': [ { - "fullDatabaseId": specific_comment_id, - "body": "Specific review comment that references #6", - "path": "file1.txt", + 'fullDatabaseId': specific_comment_id, + 'body': 'Specific review comment that references #6', + 'path': 'file1.txt', }, { - "fullDatabaseId": 456, - "body": "Another review comment referencing #7", - "path": "file2.txt", + 'fullDatabaseId': 456, + 'body': 'Another review comment referencing #7', + 'path': 'file2.txt', }, ] }, @@ -557,13 +562,13 @@ def test_pr_handler_get_converted_issues_with_specific_comment_and_issue_refs(): # Mock the response for fetching the external issue referenced in PR body mock_external_issue_response_in_body = MagicMock() mock_external_issue_response_in_body.json.return_value = { - "body": "External context #1." + 'body': 'External context #1.' } # Mock the response for fetching the external issue referenced in review thread mock_external_issue_response_review_thread = MagicMock() mock_external_issue_response_review_thread.json.return_value = { - "body": "External context #2." + 'body': 'External context #2.' } mock_get.side_effect = [ @@ -576,14 +581,16 @@ def test_pr_handler_get_converted_issues_with_specific_comment_and_issue_refs(): ] # Mock the post request for GraphQL - with patch("requests.post") as mock_post: + with patch('requests.post') as mock_post: mock_post.return_value = mock_graphql_response # Create an instance of PRHandler - handler = PRHandler("test-owner", "test-repo", "test-token") + handler = PRHandler('test-owner', 'test-repo', 'test-token') # Get converted issues - prs = handler.get_converted_issues(comment_id=specific_comment_id) + prs = handler.get_converted_issues( + issue_numbers=[1], comment_id=specific_comment_id + ) # Verify that we got exactly one PR assert len(prs) == 1 @@ -595,52 +602,52 @@ def test_pr_handler_get_converted_issues_with_specific_comment_and_issue_refs(): assert isinstance(prs[0].review_threads[0], ReviewThread) assert ( prs[0].review_threads[0].comment - == "Specific review comment that references #6\n---\nlatest feedback:\nAnother review comment referencing #7\n" + == 'Specific review comment that references #6\n---\nlatest feedback:\nAnother review comment referencing #7\n' ) assert prs[0].closing_issues == [ - "External context #1.", - "External context #2.", + 'External context #1.', + 'External context #2.', ] # Only includes references inside comment ID and body PR # Verify other fields are set correctly assert prs[0].number == 1 - assert prs[0].title == "Test PR fixes #3" - assert prs[0].body == "Test Body" - assert prs[0].owner == "test-owner" - assert prs[0].repo == "test-repo" - assert prs[0].head_branch == "test-branch" + assert prs[0].title == 'Test PR fixes #3' + assert prs[0].body == 'Test Body' + assert prs[0].owner == 'test-owner' + assert prs[0].repo == 'test-repo' + assert prs[0].head_branch == 'test-branch' def test_pr_handler_get_converted_issues_with_duplicate_issue_refs(): # Mock the necessary dependencies - with patch("requests.get") as mock_get: + with patch('requests.get') as mock_get: # Mock the response for PRs mock_prs_response = MagicMock() mock_prs_response.json.return_value = [ { - "number": 1, - "title": "Test PR", - "body": "Test Body fixes #1", - "head": {"ref": "test-branch"}, + 'number': 1, + 'title': 'Test PR', + 'body': 'Test Body fixes #1', + 'head': {'ref': 'test-branch'}, } ] # Mock the response for PR comments mock_comments_response = MagicMock() mock_comments_response.json.return_value = [ - {"body": "First comment addressing #1"}, - {"body": "Second comment addressing #2"}, + {'body': 'First comment addressing #1'}, + {'body': 'Second comment addressing #2'}, ] # Mock the response for PR metadata (GraphQL) mock_graphql_response = MagicMock() mock_graphql_response.json.return_value = { - "data": { - "repository": { - "pullRequest": { - "closingIssuesReferences": {"edges": []}, - "reviews": {"nodes": []}, - "reviewThreads": {"edges": []}, + 'data': { + 'repository': { + 'pullRequest': { + 'closingIssuesReferences': {'edges': []}, + 'reviews': {'nodes': []}, + 'reviewThreads': {'edges': []}, } } } @@ -654,13 +661,13 @@ def test_pr_handler_get_converted_issues_with_duplicate_issue_refs(): # Mock the response for fetching the external issue referenced in PR body mock_external_issue_response_in_body = MagicMock() mock_external_issue_response_in_body.json.return_value = { - "body": "External context #1." + 'body': 'External context #1.' } # Mock the response for fetching the external issue referenced in review thread mock_external_issue_response_in_comment = MagicMock() mock_external_issue_response_in_comment.json.return_value = { - "body": "External context #2." + 'body': 'External context #2.' } mock_get.side_effect = [ @@ -673,32 +680,32 @@ def test_pr_handler_get_converted_issues_with_duplicate_issue_refs(): ] # Mock the post request for GraphQL - with patch("requests.post") as mock_post: + with patch('requests.post') as mock_post: mock_post.return_value = mock_graphql_response # Create an instance of PRHandler - handler = PRHandler("test-owner", "test-repo", "test-token") + handler = PRHandler('test-owner', 'test-repo', 'test-token') # Get converted issues - prs = handler.get_converted_issues() + prs = handler.get_converted_issues(issue_numbers=[1]) # Verify that we got exactly one PR assert len(prs) == 1 # Verify that thread_comments are set correctly assert prs[0].thread_comments == [ - "First comment addressing #1", - "Second comment addressing #2", + 'First comment addressing #1', + 'Second comment addressing #2', ] # Verify other fields are set correctly assert prs[0].number == 1 - assert prs[0].title == "Test PR" - assert prs[0].body == "Test Body fixes #1" - assert prs[0].owner == "test-owner" - assert prs[0].repo == "test-repo" - assert prs[0].head_branch == "test-branch" + assert prs[0].title == 'Test PR' + assert prs[0].body == 'Test Body fixes #1' + assert prs[0].owner == 'test-owner' + assert prs[0].repo == 'test-repo' + assert prs[0].head_branch == 'test-branch' assert prs[0].closing_issues == [ - "External context #1.", - "External context #2.", + 'External context #1.', + 'External context #2.', ] diff --git a/tests/unit/resolver/test_resolve_issues.py b/tests/unit/resolver/test_resolve_issues.py index 6eb3bb9f2767..3f08db7e2bea 100644 --- a/tests/unit/resolver/test_resolve_issues.py +++ b/tests/unit/resolver/test_resolve_issues.py @@ -112,7 +112,7 @@ def get_mock_response(url, *args, **kwargs): return mock_issues_response with patch('requests.get', side_effect=get_mock_response): - issues = handler.get_converted_issues() + issues = handler.get_converted_issues(issue_numbers=[1, 3]) assert len(issues) == 2 assert handler.issue_type == 'issue' @@ -225,7 +225,7 @@ def get_mock_response(url, *args, **kwargs): with patch('requests.get', side_effect=get_mock_response): with patch('requests.post', return_value=mock_graphql_response): - issues = handler.get_converted_issues() + issues = handler.get_converted_issues(issue_numbers=[1, 2, 3]) assert len(issues) == 3 assert handler.issue_type == 'pr' @@ -811,7 +811,7 @@ def get_mock_response(url, *args, **kwargs): with patch('requests.get', side_effect=get_mock_response): with patch('requests.post', return_value=mock_graphql_response): - issues = handler.get_converted_issues() + issues = handler.get_converted_issues(issue_numbers=[1]) assert len(issues) == 1 assert handler.issue_type == 'pr' @@ -867,7 +867,9 @@ def get_mock_response(url, *args, **kwargs): return mock_issue_response with patch('requests.get', side_effect=get_mock_response): - issues = handler.get_converted_issues(comment_id=specific_comment_id) + issues = handler.get_converted_issues( + issue_numbers=[1], comment_id=specific_comment_id + ) assert len(issues) == 1 assert issues[0].number == 1 From f0ca45c59ef8586c5846703f86eb69dfbd265ef4 Mon Sep 17 00:00:00 2001 From: Rohit Malhotra Date: Tue, 19 Nov 2024 12:26:11 -0500 Subject: [PATCH 07/11] Add clarity for Openhands-resolver guide (#5124) --- docs/modules/usage/how-to/github-action.md | 35 ++++++++++++++++++++-- 1 file changed, 32 insertions(+), 3 deletions(-) diff --git a/docs/modules/usage/how-to/github-action.md b/docs/modules/usage/how-to/github-action.md index 70c43beea174..3b55ab4952a7 100644 --- a/docs/modules/usage/how-to/github-action.md +++ b/docs/modules/usage/how-to/github-action.md @@ -4,13 +4,42 @@ This guide explains how to use the OpenHands GitHub Action, both within the Open ## Using the Action in the OpenHands Repository -To use the OpenHands GitHub Action in the OpenHands repository, an OpenHands maintainer can: +To use the OpenHands GitHub Action in a repository, you can: 1. Create an issue in the repository. -2. Add the `fix-me` label to the issue. -3. The action will automatically trigger and attempt to resolve the issue. +2. Add the `fix-me` label to the issue or leave a comment on the issue starting with `@openhands-agent`. + +The action will automatically trigger and attempt to resolve the issue. ## Installing the Action in a New Repository To install the OpenHands GitHub Action in your own repository, follow the [README for the OpenHands Resolver](https://github.com/All-Hands-AI/OpenHands/blob/main/openhands/resolver/README.md). + +## Usage Tips + +### Iterative resolution + +1. Create an issue in the repository. +2. Add the `fix-me` label to the issue, or leave a comment starting with `@openhands-agent` +3. Review the attempt to resolve the issue by checking the pull request +4. Follow up with feedback through general comments, review comments, or inline thread comments +5. Add the `fix-me` label to the pull request, or address a specific comment by starting with `@openhands-agent` + +### Label versus Macro + +- Label (`fix-me`): Requests OpenHands to address the **entire** issue or pull request. +- Macro (`@openhands-agent`): Requests OpenHands to consider only the issue/pull request description and **the specific comment**. + +## Advanced Settings + +### Add custom repository settings + +You can provide custom directions for OpenHands by following the [README for the resolver](https://github.com/All-Hands-AI/OpenHands/blob/main/openhands/resolver/README.md#providing-custom-instructions). + +### Configure custom macro + +To customize the default macro (`@openhands-agent`): + +1. [Create a repository variable](https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/store-information-in-variables#creating-configuration-variables-for-a-repository) named `OPENHANDS_MACRO` +2. Assign the variable a custom value From e052c25572fb3f9001b47a627d86048a5c65b548 Mon Sep 17 00:00:00 2001 From: Robert Brennan Date: Tue, 19 Nov 2024 12:49:20 -0500 Subject: [PATCH 08/11] Fix GitHub prompt (#5123) --- openhands/agenthub/codeact_agent/micro/github.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/openhands/agenthub/codeact_agent/micro/github.md b/openhands/agenthub/codeact_agent/micro/github.md index abd48fa5c75c..c03bb546cd80 100644 --- a/openhands/agenthub/codeact_agent/micro/github.md +++ b/openhands/agenthub/codeact_agent/micro/github.md @@ -21,11 +21,9 @@ Here are some instructions for pushing, but ONLY do this if the user asks you to * After opening or updating a pull request, send the user a short message with a link to the pull request. * Do all of the above in as few steps as possible. E.g. you could open a PR with one step by running the following bash commands: ```bash -git checkout -b create-widget -git add . -git commit -m "Create widget" -git push origin create-widget -curl -X POST "https://api.github.com/repos/CodeActOrg/openhands/pulls" \ +git remote -v && git branch # to find the current org, repo and branch +git checkout -b create-widget && git add . && git commit -m "Create widget" && git push -u origin create-widget +curl -X POST "https://api.github.com/repos/$ORG_NAME/$REPO_NAME/pulls" \ -H "Authorization: Bearer $GITHUB_TOKEN" \ -d '{"title":"Create widget","head":"create-widget","base":"openhands-workspace"}' ``` From c9ed9b166be89d448c01a2ada5ef4ee525eb74b3 Mon Sep 17 00:00:00 2001 From: Robert Brennan Date: Tue, 19 Nov 2024 13:46:03 -0500 Subject: [PATCH 09/11] handle exceptions more explicitly (#4971) --- openhands/controller/agent_controller.py | 4 ++-- openhands/core/logger.py | 9 ++++++-- openhands/runtime/base.py | 12 +++++++++-- .../impl/eventstream/eventstream_runtime.py | 20 ++++++++++++++++-- .../runtime/impl/remote/remote_runtime.py | 5 ++++- openhands/server/listen.py | 21 ++++++++++++++++--- openhands/server/session/agent_session.py | 6 +++--- openhands/server/session/manager.py | 7 ++++++- 8 files changed, 68 insertions(+), 16 deletions(-) diff --git a/openhands/controller/agent_controller.py b/openhands/controller/agent_controller.py index f9e4a8edb555..e0fa0dab0384 100644 --- a/openhands/controller/agent_controller.py +++ b/openhands/controller/agent_controller.py @@ -18,6 +18,7 @@ LLMNoActionError, LLMResponseError, ) +from openhands.core.logger import LOG_ALL_EVENTS from openhands.core.logger import openhands_logger as logger from openhands.core.schema import AgentState from openhands.events import EventSource, EventStream, EventStreamSubscriber @@ -528,8 +529,7 @@ async def _step(self) -> None: await self.update_state_after_step() - # Use info level if LOG_ALL_EVENTS is set - log_level = 'info' if os.getenv('LOG_ALL_EVENTS') in ('true', '1') else 'debug' + log_level = 'info' if LOG_ALL_EVENTS else 'debug' self.log(log_level, str(action), extra={'msg_type': 'ACTION'}) async def _delegate_step(self): diff --git a/openhands/core/logger.py b/openhands/core/logger.py index 1450e503515e..238b4c39435c 100644 --- a/openhands/core/logger.py +++ b/openhands/core/logger.py @@ -17,6 +17,8 @@ LOG_TO_FILE = os.getenv('LOG_TO_FILE', 'False').lower() in ['true', '1', 'yes'] DISABLE_COLOR_PRINTING = False +LOG_ALL_EVENTS = os.getenv('LOG_ALL_EVENTS', 'False').lower() in ['true', '1', 'yes'] + ColorType = Literal[ 'red', 'green', @@ -89,8 +91,11 @@ def format(self, record): return f'{time_str} - {name_str}:{level_str}: {record.filename}:{record.lineno}\n{msg_type_color}\n{msg}' return f'{time_str} - {msg_type_color}\n{msg}' elif msg_type == 'STEP': - msg = '\n\n==============\n' + record.msg + '\n' - return f'{msg}' + if LOG_ALL_EVENTS: + msg = '\n\n==============\n' + record.msg + '\n' + return f'{msg}' + else: + return record.msg return super().format(record) diff --git a/openhands/runtime/base.py b/openhands/runtime/base.py index b12c501c19f3..74891a7d52b0 100644 --- a/openhands/runtime/base.py +++ b/openhands/runtime/base.py @@ -47,11 +47,19 @@ } -class RuntimeNotReadyError(Exception): +class RuntimeUnavailableError(Exception): pass -class RuntimeDisconnectedError(Exception): +class RuntimeNotReadyError(RuntimeUnavailableError): + pass + + +class RuntimeDisconnectedError(RuntimeUnavailableError): + pass + + +class RuntimeNotFoundError(RuntimeUnavailableError): pass diff --git a/openhands/runtime/impl/eventstream/eventstream_runtime.py b/openhands/runtime/impl/eventstream/eventstream_runtime.py index e65c36cc67c6..fe8f67f29552 100644 --- a/openhands/runtime/impl/eventstream/eventstream_runtime.py +++ b/openhands/runtime/impl/eventstream/eventstream_runtime.py @@ -34,7 +34,11 @@ ) from openhands.events.serialization import event_to_dict, observation_from_dict from openhands.events.serialization.action import ACTION_TYPE_TO_CLASS -from openhands.runtime.base import Runtime +from openhands.runtime.base import ( + Runtime, + RuntimeDisconnectedError, + RuntimeNotFoundError, +) from openhands.runtime.builder import DockerRuntimeBuilder from openhands.runtime.impl.eventstream.containers import remove_all_containers from openhands.runtime.plugins import PluginRequirement @@ -424,10 +428,22 @@ def _refresh_logs(self): @tenacity.retry( stop=tenacity.stop_after_delay(120) | stop_if_should_exit(), - reraise=(ConnectionRefusedError,), + retry=tenacity.retry_if_exception_type( + (ConnectionError, requests.exceptions.ConnectionError) + ), + reraise=True, wait=tenacity.wait_fixed(2), ) def _wait_until_alive(self): + try: + container = self.docker_client.containers.get(self.container_name) + if container.status == 'exited': + raise RuntimeDisconnectedError( + f'Container {self.container_name} has exited.' + ) + except docker.errors.NotFound: + raise RuntimeNotFoundError(f'Container {self.container_name} not found.') + self._refresh_logs() if not self.log_buffer: raise RuntimeError('Runtime client is not ready.') diff --git a/openhands/runtime/impl/remote/remote_runtime.py b/openhands/runtime/impl/remote/remote_runtime.py index 4191a047b1c2..d996441b66c9 100644 --- a/openhands/runtime/impl/remote/remote_runtime.py +++ b/openhands/runtime/impl/remote/remote_runtime.py @@ -31,6 +31,7 @@ from openhands.runtime.base import ( Runtime, RuntimeDisconnectedError, + RuntimeNotFoundError, RuntimeNotReadyError, ) from openhands.runtime.builder.remote import RemoteRuntimeBuilder @@ -109,7 +110,9 @@ def _start_or_attach_to_runtime(self): if existing_runtime: self.log('debug', f'Using existing runtime with ID: {self.runtime_id}') elif self.attach_to_existing: - raise RuntimeError('Could not find existing runtime to attach to.') + raise RuntimeNotFoundError( + f'Could not find existing runtime for SID: {self.sid}' + ) else: self.send_status_message('STATUS$STARTING_CONTAINER') if self.config.sandbox.runtime_container_image is None: diff --git a/openhands/server/listen.py b/openhands/server/listen.py index 8724daf1905f..929a26ec987d 100644 --- a/openhands/server/listen.py +++ b/openhands/server/listen.py @@ -34,6 +34,7 @@ Request, UploadFile, WebSocket, + WebSocketDisconnect, status, ) from fastapi.responses import FileResponse, JSONResponse @@ -238,7 +239,8 @@ async def attach_session(request: Request, call_next): request.state.conversation = await session_manager.attach_to_conversation( request.state.sid ) - if request.state.conversation is None: + if not request.state.conversation: + logger.error(f'Runtime not found for session: {request.state.sid}') return JSONResponse( status_code=status.HTTP_404_NOT_FOUND, content={'error': 'Session not found'}, @@ -344,7 +346,13 @@ async def websocket_endpoint(websocket: WebSocket): latest_event_id = -1 if websocket.query_params.get('latest_event_id'): - latest_event_id = int(websocket.query_params.get('latest_event_id')) + try: + latest_event_id = int(websocket.query_params.get('latest_event_id')) + except ValueError: + logger.warning( + f'Invalid latest_event_id: {websocket.query_params.get("latest_event_id")}' + ) + pass async_stream = AsyncEventStreamWrapper( session.agent_session.event_stream, latest_event_id + 1 @@ -361,7 +369,14 @@ async def websocket_endpoint(websocket: WebSocket): ), ): continue - await websocket.send_json(event_to_dict(event)) + try: + await websocket.send_json(event_to_dict(event)) + except WebSocketDisconnect: + logger.warning( + 'Websocket disconnected while sending event history, before loop started' + ) + session.close() + return await session.loop_recv() diff --git a/openhands/server/session/agent_session.py b/openhands/server/session/agent_session.py index 8f9d20a5dc6e..76f6e2aa8bcb 100644 --- a/openhands/server/session/agent_session.py +++ b/openhands/server/session/agent_session.py @@ -11,7 +11,7 @@ from openhands.events.event import EventSource from openhands.events.stream import EventStream from openhands.runtime import get_runtime_cls -from openhands.runtime.base import Runtime +from openhands.runtime.base import Runtime, RuntimeUnavailableError from openhands.security import SecurityAnalyzer, options from openhands.storage.files import FileStore @@ -194,13 +194,13 @@ async def _create_runtime( try: await self.runtime.connect() - except Exception as e: + except RuntimeUnavailableError as e: logger.error(f'Runtime initialization failed: {e}', exc_info=True) if self._status_callback: self._status_callback( 'error', 'STATUS$ERROR_RUNTIME_DISCONNECTED', str(e) ) - raise + return if self.runtime is not None: logger.debug( diff --git a/openhands/server/session/manager.py b/openhands/server/session/manager.py index f746b3676e29..790b7c4bd1eb 100644 --- a/openhands/server/session/manager.py +++ b/openhands/server/session/manager.py @@ -6,6 +6,7 @@ from openhands.core.config import AppConfig from openhands.core.logger import openhands_logger as logger from openhands.events.stream import session_exists +from openhands.runtime.base import RuntimeUnavailableError from openhands.server.session.conversation import Conversation from openhands.server.session.session import Session from openhands.storage.files import FileStore @@ -26,7 +27,11 @@ async def attach_to_conversation(self, sid: str) -> Conversation | None: if not await session_exists(sid, self.file_store): return None c = Conversation(sid, file_store=self.file_store, config=self.config) - await c.connect() + try: + await c.connect() + except RuntimeUnavailableError as e: + logger.error(f'Error connecting to conversation {c.sid}: {e}') + return None end_time = time.time() logger.info( f'Conversation {c.sid} connected in {end_time - start_time} seconds' From 3c61a9521b5718754e34600f86a7397b4f8d1856 Mon Sep 17 00:00:00 2001 From: Robert Brennan Date: Tue, 19 Nov 2024 13:46:14 -0500 Subject: [PATCH 10/11] Simple initial rate limiting implementation (#4976) --- openhands/server/listen.py | 11 ++++++- openhands/server/middleware.py | 57 ++++++++++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 1 deletion(-) diff --git a/openhands/server/listen.py b/openhands/server/listen.py index 929a26ec987d..433b13bde208 100644 --- a/openhands/server/listen.py +++ b/openhands/server/listen.py @@ -64,7 +64,12 @@ from openhands.llm import bedrock from openhands.runtime.base import Runtime from openhands.server.auth.auth import get_sid_from_token, sign_token -from openhands.server.middleware import LocalhostCORSMiddleware, NoCacheMiddleware +from openhands.server.middleware import ( + InMemoryRateLimiter, + LocalhostCORSMiddleware, + NoCacheMiddleware, + RateLimitMiddleware, +) from openhands.server.session import SessionManager load_dotenv() @@ -84,6 +89,10 @@ app.add_middleware(NoCacheMiddleware) +app.add_middleware( + RateLimitMiddleware, rate_limiter=InMemoryRateLimiter(requests=2, seconds=1) +) + security_scheme = HTTPBearer() diff --git a/openhands/server/middleware.py b/openhands/server/middleware.py index 218a949fca58..872241fc865f 100644 --- a/openhands/server/middleware.py +++ b/openhands/server/middleware.py @@ -1,6 +1,11 @@ +import asyncio +from collections import defaultdict +from datetime import datetime, timedelta from urllib.parse import urlparse +from fastapi import Request from fastapi.middleware.cors import CORSMiddleware +from fastapi.responses import JSONResponse from starlette.middleware.base import BaseHTTPMiddleware from starlette.types import ASGIApp @@ -41,3 +46,55 @@ async def dispatch(self, request, call_next): response.headers['Pragma'] = 'no-cache' response.headers['Expires'] = '0' return response + + +class InMemoryRateLimiter: + history: dict + requests: int + seconds: int + sleep_seconds: int + + def __init__(self, requests: int = 2, seconds: int = 1, sleep_seconds: int = 1): + self.requests = requests + self.seconds = seconds + self.history = defaultdict(list) + + def _clean_old_requests(self, key: str) -> None: + now = datetime.now() + cutoff = now - timedelta(seconds=self.seconds) + self.history[key] = [ts for ts in self.history[key] if ts > cutoff] + + async def __call__(self, request: Request) -> bool: + key = request.client.host + now = datetime.now() + + self._clean_old_requests(key) + + self.history[key].append(now) + + if len(self.history[key]) > self.requests * 2: + return False + elif len(self.history[key]) > self.requests: + if self.sleep_seconds > 0: + await asyncio.sleep(self.sleep_seconds) + return True + else: + return False + + return True + + +class RateLimitMiddleware(BaseHTTPMiddleware): + def __init__(self, app: ASGIApp, rate_limiter: InMemoryRateLimiter): + super().__init__(app) + self.rate_limiter = rate_limiter + + async def dispatch(self, request, call_next): + ok = await self.rate_limiter(request) + if not ok: + return JSONResponse( + status_code=429, + content={'message': 'Too many requests'}, + headers={'Retry-After': '1'}, + ) + return await call_next(request) From 302e41d7bb3d5b2b319f1ce2d15e5925dda069a2 Mon Sep 17 00:00:00 2001 From: mamoodi Date: Tue, 19 Nov 2024 14:53:24 -0500 Subject: [PATCH 11/11] Release 0.14.1 (#5133) --- frontend/package-lock.json | 2 +- frontend/package.json | 2 +- pyproject.toml | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/frontend/package-lock.json b/frontend/package-lock.json index 2d45a56c6c11..cfd8519e1adb 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -1,6 +1,6 @@ { "name": "openhands-frontend", - "version": "0.14.0", + "version": "0.14.1", "lockfileVersion": 3, "requires": true, "packages": { diff --git a/frontend/package.json b/frontend/package.json index c1415b02bd5a..1757adbe8ac3 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "openhands-frontend", - "version": "0.14.0", + "version": "0.14.1", "private": true, "type": "module", "engines": { diff --git a/pyproject.toml b/pyproject.toml index 1caad8bf9fc7..53648ae7d8e8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [tool.poetry] name = "openhands-ai" -version = "0.14.0" +version = "0.14.1" description = "OpenHands: Code Less, Make More" authors = ["OpenHands"] license = "MIT"