diff --git a/.github/workflows/lint-fix.yml b/.github/workflows/lint-fix.yml index e3021b887c53..b877de6bb4d2 100644 --- a/.github/workflows/lint-fix.yml +++ b/.github/workflows/lint-fix.yml @@ -44,7 +44,11 @@ jobs: run: pip install pre-commit==3.7.0 - name: Fix python lint issues run: | - pre-commit run --files openhands/**/* evaluation/**/* tests/**/* --config ./dev_config/python/.pre-commit-config.yaml + pre-commit run trailing-whitespace --files openhands/**/* evaluation/**/* tests/**/* --config ./dev_config/python/.pre-commit-config.yaml + pre-commit run end-of-file-fixer --files openhands/**/* evaluation/**/* tests/**/* --config ./dev_config/python/.pre-commit-config.yaml + pre-commit run pyproject-fmt --files openhands/**/* evaluation/**/* tests/**/* --config ./dev_config/python/.pre-commit-config.yaml + pre-commit run ruff --files openhands/**/* evaluation/**/* tests/**/* --config ./dev_config/python/.pre-commit-config.yaml + pre-commit run ruff-format --files openhands/**/* evaluation/**/* tests/**/* --config ./dev_config/python/.pre-commit-config.yaml # Commit and push changes if any - name: Check for changes 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. 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 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/evaluation/swe_bench/run_infer.py b/evaluation/swe_bench/run_infer.py index 386c0dd19238..9cb9dd77f498 100644 --- a/evaluation/swe_bench/run_infer.py +++ b/evaluation/swe_bench/run_infer.py @@ -145,7 +145,7 @@ def get_config( platform='linux/amd64', api_key=os.environ.get('ALLHANDS_API_KEY', None), remote_runtime_api_url=os.environ.get('SANDBOX_REMOTE_RUNTIME_API_URL'), - keep_remote_runtime_alive=False, + keep_runtime_alive=False, remote_runtime_init_timeout=3600, ), # do not mount workspace diff --git a/evaluation/utils/shared.py b/evaluation/utils/shared.py index 847eb16bb32e..517ecc523581 100644 --- a/evaluation/utils/shared.py +++ b/evaluation/utils/shared.py @@ -3,9 +3,11 @@ import multiprocessing as mp import os import pathlib +import signal import subprocess import time import traceback +from contextlib import contextmanager from typing import Any, Awaitable, Callable, TextIO import pandas as pd @@ -92,6 +94,27 @@ class EvalException(Exception): pass +class EvalTimeoutException(Exception): + pass + + +@contextmanager +def timeout(seconds: int): + def timeout_handler(signum, frame): + raise EvalTimeoutException(f'Function timed out after {seconds} seconds') + + # Set up the signal handler + original_handler = signal.signal(signal.SIGALRM, timeout_handler) + signal.alarm(seconds) + + try: + yield + finally: + # Restore the original handler and disable the alarm + signal.alarm(0) + signal.signal(signal.SIGALRM, original_handler) + + def codeact_user_response( state: State, encapsulate_solution: bool = False, @@ -280,15 +303,33 @@ def _process_instance_wrapper( metadata: EvalMetadata, use_mp: bool, max_retries: int = 5, + timeout_seconds: int | None = None, ) -> EvalOutput: - """Wrap the process_instance_func to handle retries and errors. - - Retry an instance up to max_retries times if it fails (e.g., due to transient network/runtime issues). - """ + """Wrap the process_instance_func to handle retries and errors.""" for attempt in range(max_retries + 1): try: - result = process_instance_func(instance, metadata, use_mp) + if timeout_seconds is not None: + with timeout(timeout_seconds): + result = process_instance_func(instance, metadata, use_mp) + else: + result = process_instance_func(instance, metadata, use_mp) return result + except EvalTimeoutException as e: + error = f'Timeout after {timeout_seconds} seconds' + stacktrace = traceback.format_exc() + msg = ( + '-' * 10 + + '\n' + + f'Timeout ({timeout_seconds} seconds) in instance [{instance.instance_id}], Stopped evaluation for this instance.' + + '\n' + + '-' * 10 + ) + logger.exception(e) + return EvalOutput( + instance_id=instance.instance_id, + test_result={}, + error=error, + ) except Exception as e: error = str(e) stacktrace = traceback.format_exc() @@ -337,6 +378,7 @@ def run_evaluation( [pd.Series, EvalMetadata, bool], Awaitable[EvalOutput] ], max_retries: int = 5, # number of retries for each instance + timeout_seconds: int | None = None, ): use_multiprocessing = num_workers > 1 @@ -357,7 +399,14 @@ def run_evaluation( if use_multiprocessing: with mp.Pool(num_workers) as pool: args_iter = ( - (process_instance_func, instance, metadata, True, max_retries) + ( + process_instance_func, + instance, + metadata, + True, + max_retries, + timeout_seconds, + ) for _, instance in dataset.iterrows() ) results = pool.imap_unordered(_process_instance_wrapper_mp, args_iter) diff --git a/frontend/src/api/open-hands.ts b/frontend/src/api/open-hands.ts index 072588ce476e..33f08d94f21e 100644 --- a/frontend/src/api/open-hands.ts +++ b/frontend/src/api/open-hands.ts @@ -185,8 +185,7 @@ class OpenHands { } static async getRuntimeId(): Promise<{ runtime_id: string }> { - const response = await request("/api/config"); - const data = await response.json(); + const data = await request("/api/conversation"); return data; } diff --git a/openhands/agenthub/codeact_agent/function_calling.py b/openhands/agenthub/codeact_agent/function_calling.py index 42ce1f98db87..a4ee35ff7b59 100644 --- a/openhands/agenthub/codeact_agent/function_calling.py +++ b/openhands/agenthub/codeact_agent/function_calling.py @@ -12,6 +12,7 @@ ModelResponse, ) +from openhands.core.exceptions import FunctionCallNotExistsError from openhands.core.logger import openhands_logger as logger from openhands.events.action import ( Action, @@ -484,7 +485,9 @@ def response_to_actions(response: ModelResponse) -> list[Action]: elif tool_call.function.name == 'browser': action = BrowseInteractiveAction(browser_actions=arguments['code']) else: - raise RuntimeError(f'Unknown tool call: {tool_call.function.name}') + raise FunctionCallNotExistsError( + f'Tool {tool_call.function.name} is not registered. (arguments: {arguments}). Please check the tool name and retry with an existing tool.' + ) # We only add thought to the first action if i == 0: diff --git a/openhands/controller/agent_controller.py b/openhands/controller/agent_controller.py index 9ba8c3d0a154..f9e4a8edb555 100644 --- a/openhands/controller/agent_controller.py +++ b/openhands/controller/agent_controller.py @@ -12,6 +12,7 @@ from openhands.controller.stuck import StuckDetector from openhands.core.config import AgentConfig, LLMConfig from openhands.core.exceptions import ( + FunctionCallNotExistsError, FunctionCallValidationError, LLMMalformedActionError, LLMNoActionError, @@ -488,6 +489,7 @@ async def _step(self) -> None: LLMNoActionError, LLMResponseError, FunctionCallValidationError, + FunctionCallNotExistsError, ) as e: self.event_stream.add_event( ErrorObservation( diff --git a/openhands/core/exceptions.py b/openhands/core/exceptions.py index 0c0a771191b4..bf5a29f60752 100644 --- a/openhands/core/exceptions.py +++ b/openhands/core/exceptions.py @@ -114,3 +114,10 @@ class FunctionCallValidationError(Exception): def __init__(self, message): super().__init__(message) + + +class FunctionCallNotExistsError(Exception): + """Exception raised when an LLM call a tool that is not registered.""" + + def __init__(self, message): + super().__init__(message) 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/examples/openhands-resolver.yml b/openhands/resolver/examples/openhands-resolver.yml index 6555e15057c7..2e2f42be0bac 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,16 +20,24 @@ 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' }} - max_iterations: 50 + max_iterations: ${{ vars.OPENHANDS_MAX_ITER || 50 }} secrets: PAT_TOKEN: ${{ secrets.PAT_TOKEN }} PAT_USERNAME: ${{ secrets.PAT_USERNAME }} diff --git a/openhands/resolver/issue_definitions.py b/openhands/resolver/issue_definitions.py index 10a134b812cf..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 @@ -576,9 +599,7 @@ def get_instruction( # Format thread comments if they exist thread_context = '' if issue.thread_comments: - thread_context = '\n\nPR Thread Comments:\n' + '\n---\n'.join( - issue.thread_comments - ) + thread_context = '\n---\n'.join(issue.thread_comments) images.extend(self._extract_image_urls(thread_context)) instruction = template.render( diff --git a/openhands/resolver/prompts/resolve/basic-followup.jinja b/openhands/resolver/prompts/resolve/basic-followup.jinja index cf26d80b3bdd..b7bdc2ae9e18 100644 --- a/openhands/resolver/prompts/resolve/basic-followup.jinja +++ b/openhands/resolver/prompts/resolve/basic-followup.jinja @@ -3,7 +3,7 @@ The feedback may be addressed to specific code files. In this case the file loca Please update the code based on the feedback for the repository in /workspace. An environment has been set up for you to start working. You may assume all necessary tools are installed. -# Issues addressed +# Issues addressed {{ issues }} # Review comments @@ -15,10 +15,13 @@ An environment has been set up for you to start working. You may assume all nece # Review thread files {{ files }} +# PR Thread Comments +{{ thread_context }} + IMPORTANT: You should ONLY interact with the environment provided to you AND NEVER ASK FOR HUMAN HELP. You SHOULD INCLUDE PROPER INDENTATION in your edit commands.{% if repo_instruction %} Some basic information about this repository: {{ repo_instruction }}{% endif %} -When you think you have fixed the issue through code changes, please finish the interaction. \ No newline at end of file +When you think you have fixed the issue through code changes, please finish the interaction. 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 67eb20bee1e0..b371d8160b20 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): @@ -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/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/openhands/server/listen.py b/openhands/server/listen.py index c1b178b9c2b7..8724daf1905f 100644 --- a/openhands/server/listen.py +++ b/openhands/server/listen.py @@ -11,7 +11,6 @@ from pathspec import PathSpec from pathspec.patterns import GitWildMatchPattern -from openhands.runtime.impl.remote.remote_runtime import RemoteRuntime from openhands.security.options import SecurityAnalyzers from openhands.server.data_models.feedback import FeedbackDataModel, store_feedback from openhands.server.github import ( @@ -565,27 +564,21 @@ def sanitize_filename(filename): return filename -@app.get('/api/config') +@app.get('/api/conversation') async def get_remote_runtime_config(request: Request): """Retrieve the remote runtime configuration. Currently, this is the runtime ID. """ - try: - runtime = request.state.conversation.runtime - if isinstance(runtime, RemoteRuntime): - return JSONResponse(content={'runtime_id': runtime.runtime_id}) - else: - return JSONResponse( - status_code=status.HTTP_404_NOT_FOUND, - content={'error': 'Runtime ID not available in this environment'}, - ) - except Exception as e: - logger.error(e) - return JSONResponse( - status_code=status.HTTP_500_INTERNAL_SERVER_ERROR, - content={'error': 'Something went wrong'}, - ) + runtime = request.state.conversation.runtime + runtime_id = runtime.runtime_id if hasattr(runtime, 'runtime_id') else None + session_id = runtime.sid if hasattr(runtime, 'sid') else None + return JSONResponse( + content={ + 'runtime_id': runtime_id, + 'session_id': session_id, + } + ) @app.post('/api/upload-files') 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 bcf5f36481e7..3f08db7e2bea 100644 --- a/tests/unit/resolver/test_resolve_issues.py +++ b/tests/unit/resolver/test_resolve_issues.py @@ -1,57 +1,57 @@ import os import tempfile -import pytest +from unittest.mock import AsyncMock, MagicMock, patch +import pytest -from unittest.mock import AsyncMock, patch, MagicMock +from openhands.core.config import LLMConfig +from openhands.events.action import CmdRunAction +from openhands.events.observation import CmdOutputObservation, NullObservation +from openhands.resolver.github_issue import GithubIssue, ReviewThread from openhands.resolver.issue_definitions import IssueHandler, PRHandler from openhands.resolver.resolve_issue import ( - initialize_runtime, complete_runtime, + initialize_runtime, process_issue, ) -from openhands.resolver.github_issue import GithubIssue, ReviewThread -from openhands.events.action import CmdRunAction -from openhands.events.observation import CmdOutputObservation, NullObservation from openhands.resolver.resolver_output import ResolverOutput -from openhands.core.config import LLMConfig @pytest.fixture def mock_output_dir(): with tempfile.TemporaryDirectory() as temp_dir: - repo_path = os.path.join(temp_dir, "repo") + repo_path = os.path.join(temp_dir, 'repo') # Initialize a GitHub repo in "repo" and add a commit with "README.md" os.makedirs(repo_path) - os.system(f"git init {repo_path}") - readme_path = os.path.join(repo_path, "README.md") - with open(readme_path, "w") as f: - f.write("hello world") - os.system(f"git -C {repo_path} add README.md") + os.system(f'git init {repo_path}') + readme_path = os.path.join(repo_path, 'README.md') + with open(readme_path, 'w') as f: + f.write('hello world') + os.system(f'git -C {repo_path} add README.md') os.system(f"git -C {repo_path} commit -m 'Initial commit'") yield temp_dir @pytest.fixture def mock_subprocess(): - with patch("subprocess.check_output") as mock_check_output: + with patch('subprocess.check_output') as mock_check_output: yield mock_check_output @pytest.fixture def mock_os(): - with patch("os.system") as mock_system, patch("os.path.join") as mock_join: + with patch('os.system') as mock_system, patch('os.path.join') as mock_join: yield mock_system, mock_join @pytest.fixture def mock_prompt_template(): - return "Issue: {{ body }}\n\nPlease fix this issue." + return 'Issue: {{ body }}\n\nPlease fix this issue.' @pytest.fixture def mock_followup_prompt_template(): - return "Issue context: {{ issues }}\n\nReview comments: {{ review_comments }}\n\nReview threads: {{ review_threads }}\n\nFiles: {{ files }}\n\nPlease fix this issue." + return 'Issue context: {{ issues }}\n\nReview comments: {{ review_comments }}\n\nReview threads: {{ review_threads }}\n\nFiles: {{ files }}\n\nThread comments: {{ thread_context }}\n\nPlease fix this issue.' def create_cmd_output(exit_code: int, content: str, command_id: int, command: str): @@ -64,11 +64,11 @@ def test_initialize_runtime(): mock_runtime = MagicMock() mock_runtime.run_action.side_effect = [ create_cmd_output( - exit_code=0, content="", command_id=1, command="cd /workspace" + exit_code=0, content='', command_id=1, command='cd /workspace' ), create_cmd_output( exit_code=0, - content="", + content='', command_id=2, command='git config --global core.pager ""', ), @@ -77,26 +77,26 @@ def test_initialize_runtime(): initialize_runtime(mock_runtime) assert mock_runtime.run_action.call_count == 2 - mock_runtime.run_action.assert_any_call(CmdRunAction(command="cd /workspace")) + mock_runtime.run_action.assert_any_call(CmdRunAction(command='cd /workspace')) mock_runtime.run_action.assert_any_call( CmdRunAction(command='git config --global core.pager ""') ) def test_download_issues_from_github(): - handler = IssueHandler("owner", "repo", "token") + handler = IssueHandler('owner', 'repo', 'token') mock_issues_response = MagicMock() mock_issues_response.json.side_effect = [ [ - {"number": 1, "title": "Issue 1", "body": "This is an issue"}, + {'number': 1, 'title': 'Issue 1', 'body': 'This is an issue'}, { - "number": 2, - "title": "PR 1", - "body": "This is a pull request", - "pull_request": {}, + 'number': 2, + 'title': 'PR 1', + 'body': 'This is a pull request', + 'pull_request': {}, }, - {"number": 3, "title": "Issue 2", "body": "This is another issue"}, + {'number': 3, 'title': 'Issue 2', 'body': 'This is another issue'}, ], None, ] @@ -107,41 +107,41 @@ def test_download_issues_from_github(): mock_comments_response.raise_for_status = MagicMock() def get_mock_response(url, *args, **kwargs): - if "/comments" in url: + if '/comments' in url: return mock_comments_response return mock_issues_response - with patch("requests.get", side_effect=get_mock_response): - issues = handler.get_converted_issues() + with patch('requests.get', side_effect=get_mock_response): + issues = handler.get_converted_issues(issue_numbers=[1, 3]) assert len(issues) == 2 - assert handler.issue_type == "issue" + assert handler.issue_type == 'issue' assert all(isinstance(issue, GithubIssue) for issue in issues) assert [issue.number for issue in issues] == [1, 3] - assert [issue.title for issue in issues] == ["Issue 1", "Issue 2"] + assert [issue.title for issue in issues] == ['Issue 1', 'Issue 2'] assert [issue.review_comments for issue in issues] == [None, None] assert [issue.closing_issues for issue in issues] == [None, None] assert [issue.thread_ids for issue in issues] == [None, None] def test_download_pr_from_github(): - handler = PRHandler("owner", "repo", "token") + handler = PRHandler('owner', 'repo', 'token') mock_pr_response = MagicMock() mock_pr_response.json.side_effect = [ [ { - "number": 1, - "title": "PR 1", - "body": "This is a pull request", - "head": {"ref": "b1"}, + 'number': 1, + 'title': 'PR 1', + 'body': 'This is a pull request', + 'head': {'ref': 'b1'}, }, { - "number": 2, - "title": "My PR", - "body": "This is another pull request", - "head": {"ref": "b2"}, + 'number': 2, + 'title': 'My PR', + 'body': 'This is another pull request', + 'head': {'ref': 'b2'}, }, - {"number": 3, "title": "PR 3", "body": "Final PR", "head": {"ref": "b3"}}, + {'number': 3, 'title': 'PR 3', 'body': 'Final PR', 'head': {'ref': 'b3'}}, ], None, ] @@ -155,55 +155,55 @@ def test_download_pr_from_github(): # Mock for GraphQL request (for download_pr_metadata) mock_graphql_response = MagicMock() mock_graphql_response.json.side_effect = lambda: { - "data": { - "repository": { - "pullRequest": { - "closingIssuesReferences": { - "edges": [ - {"node": {"body": "Issue 1 body", "number": 1}}, - {"node": {"body": "Issue 2 body", "number": 2}}, + 'data': { + 'repository': { + 'pullRequest': { + 'closingIssuesReferences': { + 'edges': [ + {'node': {'body': 'Issue 1 body', 'number': 1}}, + {'node': {'body': 'Issue 2 body', 'number': 2}}, ] }, - "reviewThreads": { - "edges": [ + 'reviewThreads': { + 'edges': [ { - "node": { - "isResolved": False, - "id": "1", - "comments": { - "nodes": [ + 'node': { + 'isResolved': False, + 'id': '1', + 'comments': { + 'nodes': [ { - "body": "Unresolved comment 1", - "path": "/frontend/header.tsx", + 'body': 'Unresolved comment 1', + 'path': '/frontend/header.tsx', }, - {"body": "Follow up thread"}, + {'body': 'Follow up thread'}, ] }, } }, { - "node": { - "isResolved": True, - "id": "2", - "comments": { - "nodes": [ + 'node': { + 'isResolved': True, + 'id': '2', + 'comments': { + 'nodes': [ { - "body": "Resolved comment 1", - "path": "/some/file.py", + 'body': 'Resolved comment 1', + 'path': '/some/file.py', } ] }, } }, { - "node": { - "isResolved": False, - "id": "3", - "comments": { - "nodes": [ + 'node': { + 'isResolved': False, + 'id': '3', + 'comments': { + 'nodes': [ { - "body": "Unresolved comment 3", - "path": "/another/file.py", + 'body': 'Unresolved comment 3', + 'path': '/another/file.py', } ] }, @@ -219,34 +219,34 @@ def test_download_pr_from_github(): mock_graphql_response.raise_for_status = MagicMock() def get_mock_response(url, *args, **kwargs): - if "/comments" in url: + if '/comments' in url: return mock_comments_response return mock_pr_response - with patch("requests.get", side_effect=get_mock_response): - with patch("requests.post", return_value=mock_graphql_response): - issues = handler.get_converted_issues() + with patch('requests.get', side_effect=get_mock_response): + with patch('requests.post', return_value=mock_graphql_response): + issues = handler.get_converted_issues(issue_numbers=[1, 2, 3]) assert len(issues) == 3 - assert handler.issue_type == "pr" + assert handler.issue_type == 'pr' assert all(isinstance(issue, GithubIssue) for issue in issues) assert [issue.number for issue in issues] == [1, 2, 3] - assert [issue.title for issue in issues] == ["PR 1", "My PR", "PR 3"] - assert [issue.head_branch for issue in issues] == ["b1", "b2", "b3"] + assert [issue.title for issue in issues] == ['PR 1', 'My PR', 'PR 3'] + assert [issue.head_branch for issue in issues] == ['b1', 'b2', 'b3'] assert len(issues[0].review_threads) == 2 # Only unresolved threads assert ( issues[0].review_threads[0].comment - == "Unresolved comment 1\n---\nlatest feedback:\nFollow up thread\n" + == 'Unresolved comment 1\n---\nlatest feedback:\nFollow up thread\n' ) - assert issues[0].review_threads[0].files == ["/frontend/header.tsx"] + assert issues[0].review_threads[0].files == ['/frontend/header.tsx'] assert ( issues[0].review_threads[1].comment - == "latest feedback:\nUnresolved comment 3\n" + == 'latest feedback:\nUnresolved comment 3\n' ) - assert issues[0].review_threads[1].files == ["/another/file.py"] - assert issues[0].closing_issues == ["Issue 1 body", "Issue 2 body"] - assert issues[0].thread_ids == ["1", "3"] + assert issues[0].review_threads[1].files == ['/another/file.py'] + assert issues[0].closing_issues == ['Issue 1 body', 'Issue 2 body'] + assert issues[0].thread_ids == ['1', '3'] @pytest.mark.asyncio @@ -254,34 +254,34 @@ async def test_complete_runtime(): mock_runtime = MagicMock() mock_runtime.run_action.side_effect = [ create_cmd_output( - exit_code=0, content="", command_id=1, command="cd /workspace" + exit_code=0, content='', command_id=1, command='cd /workspace' ), create_cmd_output( exit_code=0, - content="", + content='', command_id=2, command='git config --global core.pager ""', ), create_cmd_output( exit_code=0, - content="", + content='', command_id=3, - command="git config --global --add safe.directory /workspace", + command='git config --global --add safe.directory /workspace', ), create_cmd_output( exit_code=0, - content="", + content='', command_id=4, - command="git diff base_commit_hash fix", + command='git diff base_commit_hash fix', ), create_cmd_output( - exit_code=0, content="git diff content", command_id=5, command="git apply" + exit_code=0, content='git diff content', command_id=5, command='git apply' ), ] - result = await complete_runtime(mock_runtime, "base_commit_hash") + result = await complete_runtime(mock_runtime, 'base_commit_hash') - assert result == {"git_patch": "git diff content"} + assert result == {'git_patch': 'git diff content'} assert mock_runtime.run_action.call_count == 5 @@ -296,65 +296,65 @@ async def test_process_issue(mock_output_dir, mock_prompt_template): # Set up test data issue = GithubIssue( - owner="test_owner", - repo="test_repo", + owner='test_owner', + repo='test_repo', number=1, - title="Test Issue", - body="This is a test issue", + title='Test Issue', + body='This is a test issue', ) - base_commit = "abcdef1234567890" - repo_instruction = "Resolve this repo" + base_commit = 'abcdef1234567890' + repo_instruction = 'Resolve this repo' max_iterations = 5 - llm_config = LLMConfig(model="test_model", api_key="test_api_key") - runtime_container_image = "test_image:latest" + llm_config = LLMConfig(model='test_model', api_key='test_api_key') + runtime_container_image = 'test_image:latest' # Test cases for different scenarios test_cases = [ { - "name": "successful_run", - "run_controller_return": MagicMock( - history=[NullObservation(content="")], + 'name': 'successful_run', + 'run_controller_return': MagicMock( + history=[NullObservation(content='')], metrics=MagicMock( - get=MagicMock(return_value={"test_result": "passed"}) + get=MagicMock(return_value={'test_result': 'passed'}) ), last_error=None, ), - "run_controller_raises": None, - "expected_success": True, - "expected_error": None, - "expected_explanation": "Issue resolved successfully", + 'run_controller_raises': None, + 'expected_success': True, + 'expected_error': None, + 'expected_explanation': 'Issue resolved successfully', }, { - "name": "value_error", - "run_controller_return": None, - "run_controller_raises": ValueError("Test value error"), - "expected_success": False, - "expected_error": "Agent failed to run or crashed", - "expected_explanation": "Agent failed to run", + 'name': 'value_error', + 'run_controller_return': None, + 'run_controller_raises': ValueError('Test value error'), + 'expected_success': False, + 'expected_error': 'Agent failed to run or crashed', + 'expected_explanation': 'Agent failed to run', }, { - "name": "runtime_error", - "run_controller_return": None, - "run_controller_raises": RuntimeError("Test runtime error"), - "expected_success": False, - "expected_error": "Agent failed to run or crashed", - "expected_explanation": "Agent failed to run", + 'name': 'runtime_error', + 'run_controller_return': None, + 'run_controller_raises': RuntimeError('Test runtime error'), + 'expected_success': False, + 'expected_error': 'Agent failed to run or crashed', + 'expected_explanation': 'Agent failed to run', }, { - "name": "json_decode_error", - "run_controller_return": MagicMock( - history=[NullObservation(content="")], + 'name': 'json_decode_error', + 'run_controller_return': MagicMock( + history=[NullObservation(content='')], metrics=MagicMock( - get=MagicMock(return_value={"test_result": "passed"}) + get=MagicMock(return_value={'test_result': 'passed'}) ), last_error=None, ), - "run_controller_raises": None, - "expected_success": True, - "expected_error": None, - "expected_explanation": "Non-JSON explanation", - "is_pr": True, - "comment_success": [ + 'run_controller_raises': None, + 'expected_success': True, + 'expected_error': None, + 'expected_explanation': 'Non-JSON explanation', + 'is_pr': True, + 'comment_success': [ True, False, ], # To trigger the PR success logging code path @@ -371,31 +371,31 @@ async def test_process_issue(mock_output_dir, mock_prompt_template): # Mock return values mock_create_runtime.return_value = MagicMock(connect=AsyncMock()) - if test_case["run_controller_raises"]: - mock_run_controller.side_effect = test_case["run_controller_raises"] + if test_case['run_controller_raises']: + mock_run_controller.side_effect = test_case['run_controller_raises'] else: - mock_run_controller.return_value = test_case["run_controller_return"] + mock_run_controller.return_value = test_case['run_controller_return'] mock_run_controller.side_effect = None - mock_complete_runtime.return_value = {"git_patch": "test patch"} + mock_complete_runtime.return_value = {'git_patch': 'test patch'} handler_instance.guess_success.return_value = ( - test_case["expected_success"], - test_case.get("comment_success", None), - test_case["expected_explanation"], + test_case['expected_success'], + test_case.get('comment_success', None), + test_case['expected_explanation'], ) - handler_instance.get_instruction.return_value = ("Test instruction", []) - handler_instance.issue_type = "pr" if test_case.get("is_pr", False) else "issue" + handler_instance.get_instruction.return_value = ('Test instruction', []) + handler_instance.issue_type = 'pr' if test_case.get('is_pr', False) else 'issue' with patch( - "openhands.resolver.resolve_issue.create_runtime", mock_create_runtime + 'openhands.resolver.resolve_issue.create_runtime', mock_create_runtime ), patch( - "openhands.resolver.resolve_issue.initialize_runtime", + 'openhands.resolver.resolve_issue.initialize_runtime', mock_initialize_runtime, ), patch( - "openhands.resolver.resolve_issue.run_controller", mock_run_controller + 'openhands.resolver.resolve_issue.run_controller', mock_run_controller ), patch( - "openhands.resolver.resolve_issue.complete_runtime", mock_complete_runtime - ), patch("openhands.resolver.resolve_issue.logger"): + 'openhands.resolver.resolve_issue.complete_runtime', mock_complete_runtime + ), patch('openhands.resolver.resolve_issue.logger'): # Call the function result = await process_issue( issue, @@ -411,15 +411,15 @@ async def test_process_issue(mock_output_dir, mock_prompt_template): ) # Assert the result - expected_issue_type = "pr" if test_case.get("is_pr", False) else "issue" + expected_issue_type = 'pr' if test_case.get('is_pr', False) else 'issue' assert handler_instance.issue_type == expected_issue_type assert isinstance(result, ResolverOutput) assert result.issue == issue assert result.base_commit == base_commit - assert result.git_patch == "test patch" - assert result.success == test_case["expected_success"] - assert result.success_explanation == test_case["expected_explanation"] - assert result.error == test_case["expected_error"] + assert result.git_patch == 'test patch' + assert result.success == test_case['expected_success'] + assert result.success_explanation == test_case['expected_explanation'] + assert result.error == test_case['expected_error'] # Assert that the mocked functions were called mock_create_runtime.assert_called_once() @@ -428,7 +428,7 @@ async def test_process_issue(mock_output_dir, mock_prompt_template): mock_complete_runtime.assert_called_once() # Assert that guess_success was called only for successful runs - if test_case["expected_success"]: + if test_case['expected_success']: handler_instance.guess_success.assert_called_once() else: handler_instance.guess_success.assert_not_called() @@ -436,60 +436,64 @@ async def test_process_issue(mock_output_dir, mock_prompt_template): def test_get_instruction(mock_prompt_template, mock_followup_prompt_template): issue = GithubIssue( - owner="test_owner", - repo="test_repo", + owner='test_owner', + repo='test_repo', number=123, - title="Test Issue", - body="This is a test issue refer to image ![First Image](https://sampleimage.com/image1.png)", + title='Test Issue', + body='This is a test issue refer to image ![First Image](https://sampleimage.com/image1.png)', ) - issue_handler = IssueHandler("owner", "repo", "token") + issue_handler = IssueHandler('owner', 'repo', 'token') instruction, images_urls = issue_handler.get_instruction( issue, mock_prompt_template, None ) - expected_instruction = "Issue: Test Issue\n\nThis is a test issue refer to image ![First Image](https://sampleimage.com/image1.png)\n\nPlease fix this issue." + expected_instruction = 'Issue: Test Issue\n\nThis is a test issue refer to image ![First Image](https://sampleimage.com/image1.png)\n\nPlease fix this issue.' - assert images_urls == ["https://sampleimage.com/image1.png"] - assert issue_handler.issue_type == "issue" + assert images_urls == ['https://sampleimage.com/image1.png'] + assert issue_handler.issue_type == 'issue' assert instruction == expected_instruction issue = GithubIssue( - owner="test_owner", - repo="test_repo", + owner='test_owner', + repo='test_repo', number=123, - title="Test Issue", - body="This is a test issue", - closing_issues=["Issue 1 fix the type"], + title='Test Issue', + body='This is a test issue', + closing_issues=['Issue 1 fix the type'], review_threads=[ ReviewThread( comment="There is still a typo 'pthon' instead of 'python'", files=[] ) ], + thread_comments=[ + "I've left review comments, please address them", + 'This is a valid concern.', + ], ) - pr_handler = PRHandler("owner", "repo", "token") + pr_handler = PRHandler('owner', 'repo', 'token') instruction, images_urls = pr_handler.get_instruction( issue, mock_followup_prompt_template, None ) - expected_instruction = "Issue context: [\n \"Issue 1 fix the type\"\n]\n\nReview comments: None\n\nReview threads: [\n \"There is still a typo 'pthon' instead of 'python'\"\n]\n\nFiles: []\n\nPlease fix this issue." + expected_instruction = "Issue context: [\n \"Issue 1 fix the type\"\n]\n\nReview comments: None\n\nReview threads: [\n \"There is still a typo 'pthon' instead of 'python'\"\n]\n\nFiles: []\n\nThread comments: I've left review comments, please address them\n---\nThis is a valid concern.\n\nPlease fix this issue." assert images_urls == [] - assert pr_handler.issue_type == "pr" + assert pr_handler.issue_type == 'pr' assert instruction == expected_instruction def test_file_instruction(): issue = GithubIssue( - owner="test_owner", - repo="test_repo", + owner='test_owner', + repo='test_repo', number=123, - title="Test Issue", - body="This is a test issue ![image](https://sampleimage.com/sample.png)", + title='Test Issue', + body='This is a test issue ![image](https://sampleimage.com/sample.png)', ) # load prompt from openhands/resolver/prompts/resolve/basic.jinja - with open("openhands/resolver/prompts/resolve/basic.jinja", "r") as f: + with open('openhands/resolver/prompts/resolve/basic.jinja', 'r') as f: prompt = f.read() # Test without thread comments - issue_handler = IssueHandler("owner", "repo", "token") + issue_handler = IssueHandler('owner', 'repo', 'token') instruction, images_urls = issue_handler.get_instruction(issue, prompt, None) expected_instruction = """Please fix the following issue for the repository in /workspace. An environment has been set up for you to start working. You may assume all necessary tools are installed. @@ -505,28 +509,28 @@ def test_file_instruction(): When you think you have fixed the issue through code changes, please finish the interaction.""" assert instruction == expected_instruction - assert images_urls == ["https://sampleimage.com/sample.png"] + assert images_urls == ['https://sampleimage.com/sample.png'] def test_file_instruction_with_repo_instruction(): issue = GithubIssue( - owner="test_owner", - repo="test_repo", + owner='test_owner', + repo='test_repo', number=123, - title="Test Issue", - body="This is a test issue", + title='Test Issue', + body='This is a test issue', ) # load prompt from openhands/resolver/prompts/resolve/basic.jinja - with open("openhands/resolver/prompts/resolve/basic.jinja", "r") as f: + with open('openhands/resolver/prompts/resolve/basic.jinja', 'r') as f: prompt = f.read() # load repo instruction from openhands/resolver/prompts/repo_instructions/all-hands-ai___openhands-resolver.txt with open( - "openhands/resolver/prompts/repo_instructions/all-hands-ai___openhands-resolver.txt", - "r", + 'openhands/resolver/prompts/repo_instructions/all-hands-ai___openhands-resolver.txt', + 'r', ) as f: repo_instruction = f.read() - issue_handler = IssueHandler("owner", "repo", "token") + issue_handler = IssueHandler('owner', 'repo', 'token') instruction, image_urls = issue_handler.get_instruction( issue, prompt, repo_instruction ) @@ -549,226 +553,226 @@ def test_file_instruction_with_repo_instruction(): When you think you have fixed the issue through code changes, please finish the interaction.""" assert instruction == expected_instruction - assert issue_handler.issue_type == "issue" + assert issue_handler.issue_type == 'issue' assert image_urls == [] def test_guess_success(): mock_issue = GithubIssue( - owner="test_owner", - repo="test_repo", + owner='test_owner', + repo='test_repo', number=1, - title="Test Issue", - body="This is a test issue", + title='Test Issue', + body='This is a test issue', ) mock_history = [ create_cmd_output( - exit_code=0, content="", command_id=1, command="cd /workspace" + exit_code=0, content='', command_id=1, command='cd /workspace' ) ] - mock_llm_config = LLMConfig(model="test_model", api_key="test_api_key") + mock_llm_config = LLMConfig(model='test_model', api_key='test_api_key') mock_completion_response = MagicMock() mock_completion_response.choices = [ MagicMock( message=MagicMock( - content="--- success\ntrue\n--- explanation\nIssue resolved successfully" + content='--- success\ntrue\n--- explanation\nIssue resolved successfully' ) ) ] - issue_handler = IssueHandler("owner", "repo", "token") + issue_handler = IssueHandler('owner', 'repo', 'token') - with patch("litellm.completion", MagicMock(return_value=mock_completion_response)): + with patch('litellm.completion', MagicMock(return_value=mock_completion_response)): success, comment_success, explanation = issue_handler.guess_success( mock_issue, mock_history, mock_llm_config ) - assert issue_handler.issue_type == "issue" + assert issue_handler.issue_type == 'issue' assert comment_success is None assert success - assert explanation == "Issue resolved successfully" + assert explanation == 'Issue resolved successfully' def test_guess_success_with_thread_comments(): mock_issue = GithubIssue( - owner="test_owner", - repo="test_repo", + owner='test_owner', + repo='test_repo', number=1, - title="Test Issue", - body="This is a test issue", + title='Test Issue', + body='This is a test issue', thread_comments=[ - "First comment", - "Second comment", - "latest feedback:\nPlease add tests", + 'First comment', + 'Second comment', + 'latest feedback:\nPlease add tests', ], ) - mock_history = [MagicMock(message="I have added tests for this case")] - mock_llm_config = LLMConfig(model="test_model", api_key="test_api_key") + mock_history = [MagicMock(message='I have added tests for this case')] + mock_llm_config = LLMConfig(model='test_model', api_key='test_api_key') mock_completion_response = MagicMock() mock_completion_response.choices = [ MagicMock( message=MagicMock( - content="--- success\ntrue\n--- explanation\nTests have been added to verify thread comments handling" + content='--- success\ntrue\n--- explanation\nTests have been added to verify thread comments handling' ) ) ] - issue_handler = IssueHandler("owner", "repo", "token") + issue_handler = IssueHandler('owner', 'repo', 'token') - with patch("litellm.completion", MagicMock(return_value=mock_completion_response)): + with patch('litellm.completion', MagicMock(return_value=mock_completion_response)): success, comment_success, explanation = issue_handler.guess_success( mock_issue, mock_history, mock_llm_config ) - assert issue_handler.issue_type == "issue" + assert issue_handler.issue_type == 'issue' assert comment_success is None assert success - assert "Tests have been added" in explanation + assert 'Tests have been added' in explanation def test_instruction_with_thread_comments(): # Create an issue with thread comments issue = GithubIssue( - owner="test_owner", - repo="test_repo", + owner='test_owner', + repo='test_repo', number=123, - title="Test Issue", - body="This is a test issue", + title='Test Issue', + body='This is a test issue', thread_comments=[ - "First comment", - "Second comment", - "latest feedback:\nPlease add tests", + 'First comment', + 'Second comment', + 'latest feedback:\nPlease add tests', ], ) # Load the basic prompt template - with open("openhands/resolver/prompts/resolve/basic.jinja", "r") as f: + with open('openhands/resolver/prompts/resolve/basic.jinja', 'r') as f: prompt = f.read() - issue_handler = IssueHandler("owner", "repo", "token") + issue_handler = IssueHandler('owner', 'repo', 'token') instruction, images_urls = issue_handler.get_instruction(issue, prompt, None) # Verify that thread comments are included in the instruction - assert "First comment" in instruction - assert "Second comment" in instruction - assert "Please add tests" in instruction - assert "Issue Thread Comments:" in instruction + assert 'First comment' in instruction + assert 'Second comment' in instruction + assert 'Please add tests' in instruction + assert 'Issue Thread Comments:' in instruction assert images_urls == [] def test_guess_success_failure(): mock_issue = GithubIssue( - owner="test_owner", - repo="test_repo", + owner='test_owner', + repo='test_repo', number=1, - title="Test Issue", - body="This is a test issue", + title='Test Issue', + body='This is a test issue', thread_comments=[ - "First comment", - "Second comment", - "latest feedback:\nPlease add tests", + 'First comment', + 'Second comment', + 'latest feedback:\nPlease add tests', ], ) - mock_history = [MagicMock(message="I have added tests for this case")] - mock_llm_config = LLMConfig(model="test_model", api_key="test_api_key") + mock_history = [MagicMock(message='I have added tests for this case')] + mock_llm_config = LLMConfig(model='test_model', api_key='test_api_key') mock_completion_response = MagicMock() mock_completion_response.choices = [ MagicMock( message=MagicMock( - content="--- success\ntrue\n--- explanation\nTests have been added to verify thread comments handling" + content='--- success\ntrue\n--- explanation\nTests have been added to verify thread comments handling' ) ) ] - issue_handler = IssueHandler("owner", "repo", "token") + issue_handler = IssueHandler('owner', 'repo', 'token') - with patch("litellm.completion", MagicMock(return_value=mock_completion_response)): + with patch('litellm.completion', MagicMock(return_value=mock_completion_response)): success, comment_success, explanation = issue_handler.guess_success( mock_issue, mock_history, mock_llm_config ) - assert issue_handler.issue_type == "issue" + assert issue_handler.issue_type == 'issue' assert comment_success is None assert success - assert "Tests have been added" in explanation + assert 'Tests have been added' in explanation def test_guess_success_negative_case(): mock_issue = GithubIssue( - owner="test_owner", - repo="test_repo", + owner='test_owner', + repo='test_repo', number=1, - title="Test Issue", - body="This is a test issue", + title='Test Issue', + body='This is a test issue', ) mock_history = [ create_cmd_output( - exit_code=0, content="", command_id=1, command="cd /workspace" + exit_code=0, content='', command_id=1, command='cd /workspace' ) ] - mock_llm_config = LLMConfig(model="test_model", api_key="test_api_key") + mock_llm_config = LLMConfig(model='test_model', api_key='test_api_key') mock_completion_response = MagicMock() mock_completion_response.choices = [ MagicMock( message=MagicMock( - content="--- success\nfalse\n--- explanation\nIssue not resolved" + content='--- success\nfalse\n--- explanation\nIssue not resolved' ) ) ] - issue_handler = IssueHandler("owner", "repo", "token") + issue_handler = IssueHandler('owner', 'repo', 'token') - with patch("litellm.completion", MagicMock(return_value=mock_completion_response)): + with patch('litellm.completion', MagicMock(return_value=mock_completion_response)): success, comment_success, explanation = issue_handler.guess_success( mock_issue, mock_history, mock_llm_config ) - assert issue_handler.issue_type == "issue" + assert issue_handler.issue_type == 'issue' assert comment_success is None assert not success - assert explanation == "Issue not resolved" + assert explanation == 'Issue not resolved' def test_guess_success_invalid_output(): mock_issue = GithubIssue( - owner="test_owner", - repo="test_repo", + owner='test_owner', + repo='test_repo', number=1, - title="Test Issue", - body="This is a test issue", + title='Test Issue', + body='This is a test issue', ) mock_history = [ create_cmd_output( - exit_code=0, content="", command_id=1, command="cd /workspace" + exit_code=0, content='', command_id=1, command='cd /workspace' ) ] - mock_llm_config = LLMConfig(model="test_model", api_key="test_api_key") + mock_llm_config = LLMConfig(model='test_model', api_key='test_api_key') mock_completion_response = MagicMock() mock_completion_response.choices = [ - MagicMock(message=MagicMock(content="This is not a valid output")) + MagicMock(message=MagicMock(content='This is not a valid output')) ] - issue_handler = IssueHandler("owner", "repo", "token") + issue_handler = IssueHandler('owner', 'repo', 'token') - with patch("litellm.completion", MagicMock(return_value=mock_completion_response)): + with patch('litellm.completion', MagicMock(return_value=mock_completion_response)): success, comment_success, explanation = issue_handler.guess_success( mock_issue, mock_history, mock_llm_config ) - assert issue_handler.issue_type == "issue" + assert issue_handler.issue_type == 'issue' assert comment_success is None assert not success assert ( explanation - == "Failed to decode answer from LLM response: This is not a valid output" + == 'Failed to decode answer from LLM response: This is not a valid output' ) def test_download_pr_with_review_comments(): - handler = PRHandler("owner", "repo", "token") + handler = PRHandler('owner', 'repo', 'token') mock_pr_response = MagicMock() mock_pr_response.json.side_effect = [ [ { - "number": 1, - "title": "PR 1", - "body": "This is a pull request", - "head": {"ref": "b1"}, + 'number': 1, + 'title': 'PR 1', + 'body': 'This is a pull request', + 'head': {'ref': 'b1'}, }, ], None, @@ -783,14 +787,14 @@ def test_download_pr_with_review_comments(): # Mock for GraphQL request with review comments but no threads mock_graphql_response = MagicMock() mock_graphql_response.json.side_effect = lambda: { - "data": { - "repository": { - "pullRequest": { - "closingIssuesReferences": {"edges": []}, - "reviews": { - "nodes": [ - {"body": "Please fix this typo"}, - {"body": "Add more tests"}, + 'data': { + 'repository': { + 'pullRequest': { + 'closingIssuesReferences': {'edges': []}, + 'reviews': { + 'nodes': [ + {'body': 'Please fix this typo'}, + {'body': 'Add more tests'}, ] }, } @@ -801,32 +805,32 @@ def test_download_pr_with_review_comments(): mock_graphql_response.raise_for_status = MagicMock() def get_mock_response(url, *args, **kwargs): - if "/comments" in url: + if '/comments' in url: return mock_comments_response return mock_pr_response - with patch("requests.get", side_effect=get_mock_response): - with patch("requests.post", return_value=mock_graphql_response): - issues = handler.get_converted_issues() + with patch('requests.get', side_effect=get_mock_response): + with patch('requests.post', return_value=mock_graphql_response): + issues = handler.get_converted_issues(issue_numbers=[1]) assert len(issues) == 1 - assert handler.issue_type == "pr" + assert handler.issue_type == 'pr' assert isinstance(issues[0], GithubIssue) assert issues[0].number == 1 - assert issues[0].title == "PR 1" - assert issues[0].head_branch == "b1" + assert issues[0].title == 'PR 1' + assert issues[0].head_branch == 'b1' # Verify review comments are set but threads are empty assert len(issues[0].review_comments) == 2 - assert issues[0].review_comments[0] == "Please fix this typo" - assert issues[0].review_comments[1] == "Add more tests" + assert issues[0].review_comments[0] == 'Please fix this typo' + assert issues[0].review_comments[1] == 'Add more tests' assert not issues[0].review_threads assert not issues[0].closing_issues assert not issues[0].thread_ids def test_download_issue_with_specific_comment(): - handler = IssueHandler("owner", "repo", "token") + handler = IssueHandler('owner', 'repo', 'token') # Define the specific comment_id to filter specific_comment_id = 101 @@ -835,7 +839,7 @@ def test_download_issue_with_specific_comment(): mock_issue_response = MagicMock() mock_issue_response.json.side_effect = [ [ - {"number": 1, "title": "Issue 1", "body": "This is an issue"}, + {'number': 1, 'title': 'Issue 1', 'body': 'This is an issue'}, ], None, ] @@ -844,32 +848,34 @@ def test_download_issue_with_specific_comment(): mock_comments_response = MagicMock() mock_comments_response.json.return_value = [ { - "id": specific_comment_id, - "body": "Specific comment body", - "issue_url": "https://api.github.com/repos/owner/repo/issues/1", + 'id': specific_comment_id, + 'body': 'Specific comment body', + 'issue_url': 'https://api.github.com/repos/owner/repo/issues/1', }, { - "id": 102, - "body": "Another comment body", - "issue_url": "https://api.github.com/repos/owner/repo/issues/2", + 'id': 102, + 'body': 'Another comment body', + 'issue_url': 'https://api.github.com/repos/owner/repo/issues/2', }, ] mock_comments_response.raise_for_status = MagicMock() def get_mock_response(url, *args, **kwargs): - if "/comments" in url: + if '/comments' in url: return mock_comments_response return mock_issue_response - with patch("requests.get", side_effect=get_mock_response): - issues = handler.get_converted_issues(comment_id=specific_comment_id) + with patch('requests.get', side_effect=get_mock_response): + issues = handler.get_converted_issues( + issue_numbers=[1], comment_id=specific_comment_id + ) assert len(issues) == 1 assert issues[0].number == 1 - assert issues[0].title == "Issue 1" - assert issues[0].thread_comments == ["Specific comment body"] + assert issues[0].title == 'Issue 1' + assert issues[0].thread_comments == ['Specific comment body'] -if __name__ == "__main__": +if __name__ == '__main__': pytest.main() 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()