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/frontend/package-lock.json b/frontend/package-lock.json index 2d45a56c6c11..cfd8519e1adb 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -1,6 +1,6 @@ { "name": "openhands-frontend", - "version": "0.14.0", + "version": "0.14.1", "lockfileVersion": 3, "requires": true, "packages": { diff --git a/frontend/package.json b/frontend/package.json index c1415b02bd5a..1757adbe8ac3 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,6 +1,6 @@ { "name": "openhands-frontend", - "version": "0.14.0", + "version": "0.14.1", "private": true, "type": "module", "engines": { diff --git a/openhands/agenthub/codeact_agent/micro/github.md b/openhands/agenthub/codeact_agent/micro/github.md index abd48fa5c75c..c03bb546cd80 100644 --- a/openhands/agenthub/codeact_agent/micro/github.md +++ b/openhands/agenthub/codeact_agent/micro/github.md @@ -21,11 +21,9 @@ Here are some instructions for pushing, but ONLY do this if the user asks you to * After opening or updating a pull request, send the user a short message with a link to the pull request. * Do all of the above in as few steps as possible. E.g. you could open a PR with one step by running the following bash commands: ```bash -git checkout -b create-widget -git add . -git commit -m "Create widget" -git push origin create-widget -curl -X POST "https://api.github.com/repos/CodeActOrg/openhands/pulls" \ +git remote -v && git branch # to find the current org, repo and branch +git checkout -b create-widget && git add . && git commit -m "Create widget" && git push -u origin create-widget +curl -X POST "https://api.github.com/repos/$ORG_NAME/$REPO_NAME/pulls" \ -H "Authorization: Bearer $GITHUB_TOKEN" \ -d '{"title":"Create widget","head":"create-widget","base":"openhands-workspace"}' ``` diff --git a/openhands/controller/agent_controller.py b/openhands/controller/agent_controller.py index f9e4a8edb555..e0fa0dab0384 100644 --- a/openhands/controller/agent_controller.py +++ b/openhands/controller/agent_controller.py @@ -18,6 +18,7 @@ LLMNoActionError, LLMResponseError, ) +from openhands.core.logger import LOG_ALL_EVENTS from openhands.core.logger import openhands_logger as logger from openhands.core.schema import AgentState from openhands.events import EventSource, EventStream, EventStreamSubscriber @@ -528,8 +529,7 @@ async def _step(self) -> None: await self.update_state_after_step() - # Use info level if LOG_ALL_EVENTS is set - log_level = 'info' if os.getenv('LOG_ALL_EVENTS') in ('true', '1') else 'debug' + log_level = 'info' if LOG_ALL_EVENTS else 'debug' self.log(log_level, str(action), extra={'msg_type': 'ACTION'}) async def _delegate_step(self): diff --git a/openhands/core/logger.py b/openhands/core/logger.py index 1450e503515e..238b4c39435c 100644 --- a/openhands/core/logger.py +++ b/openhands/core/logger.py @@ -17,6 +17,8 @@ LOG_TO_FILE = os.getenv('LOG_TO_FILE', 'False').lower() in ['true', '1', 'yes'] DISABLE_COLOR_PRINTING = False +LOG_ALL_EVENTS = os.getenv('LOG_ALL_EVENTS', 'False').lower() in ['true', '1', 'yes'] + ColorType = Literal[ 'red', 'green', @@ -89,8 +91,11 @@ def format(self, record): return f'{time_str} - {name_str}:{level_str}: {record.filename}:{record.lineno}\n{msg_type_color}\n{msg}' return f'{time_str} - {msg_type_color}\n{msg}' elif msg_type == 'STEP': - msg = '\n\n==============\n' + record.msg + '\n' - return f'{msg}' + if LOG_ALL_EVENTS: + msg = '\n\n==============\n' + record.msg + '\n' + return f'{msg}' + else: + return record.msg return super().format(record) diff --git a/openhands/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 c9e386178adf..a0d0eb570aa7 100644 --- a/openhands/resolver/issue_definitions.py +++ b/openhands/resolver/issue_definitions.py @@ -18,7 +18,9 @@ class IssueHandlerInterface(ABC): issue_type: ClassVar[str] @abstractmethod - def get_converted_issues(self, comment_id: int | None = None) -> list[GithubIssue]: + def get_converted_issues( + self, issue_numbers: list[int] | None = None, comment_id: int | None = None + ) -> list[GithubIssue]: """Download issues from GitHub.""" pass @@ -138,13 +140,29 @@ def _get_issue_comments( return all_comments if all_comments else None - def get_converted_issues(self, comment_id: int | None = None) -> list[GithubIssue]: + def get_converted_issues( + self, issue_numbers: list[int] | None = None, comment_id: int | None = None + ) -> list[GithubIssue]: """Download issues from Github. Returns: List of Github issues. """ + + if not issue_numbers: + raise ValueError('Unspecified issue number') + all_issues = self._download_issues_from_github() + logger.info(f'Limiting resolving to issues {issue_numbers}.') + all_issues = [ + issue + for issue in all_issues + if issue['number'] in issue_numbers and 'pull_request' not in issue + ] + + if len(issue_numbers) == 1 and not all_issues: + raise ValueError(f'Issue {issue_numbers[0]} not found') + converted_issues = [] for issue in all_issues: if any([issue.get(key) is None for key in ['number', 'title', 'body']]): @@ -153,9 +171,6 @@ def get_converted_issues(self, comment_id: int | None = None) -> list[GithubIssu ) continue - if 'pull_request' in issue: - continue - # Get issue thread comments thread_comments = self._get_issue_comments( issue['number'], comment_id=comment_id @@ -486,8 +501,16 @@ def __get_context_from_external_issues_references( return closing_issues - def get_converted_issues(self, comment_id: int | None = None) -> list[GithubIssue]: + def get_converted_issues( + self, issue_numbers: list[int] | None = None, comment_id: int | None = None + ) -> list[GithubIssue]: + if not issue_numbers: + raise ValueError('Unspecified issue numbers') + all_issues = self._download_issues_from_github() + logger.info(f'Limiting resolving to issues {issue_numbers}.') + all_issues = [issue for issue in all_issues if issue['number'] in issue_numbers] + converted_issues = [] for issue in all_issues: # For PRs, body can be None diff --git a/openhands/resolver/resolve_all_issues.py b/openhands/resolver/resolve_all_issues.py index a561b24a61a7..01d076446e97 100644 --- a/openhands/resolver/resolve_all_issues.py +++ b/openhands/resolver/resolve_all_issues.py @@ -83,11 +83,10 @@ async def resolve_issues( issue_handler = issue_handler_factory(issue_type, owner, repo, token) # Load dataset - issues: list[GithubIssue] = issue_handler.get_converted_issues() + issues: list[GithubIssue] = issue_handler.get_converted_issues( + issue_numbers=issue_numbers + ) - if issue_numbers is not None: - issues = [issue for issue in issues if issue.number in issue_numbers] - logger.info(f'Limiting resolving to issues {issue_numbers}.') if limit_issues is not None: issues = issues[:limit_issues] logger.info(f'Limiting resolving to first {limit_issues} issues.') diff --git a/openhands/resolver/resolve_issue.py b/openhands/resolver/resolve_issue.py index 67ce3b0c4fbf..9e9319033876 100644 --- a/openhands/resolver/resolve_issue.py +++ b/openhands/resolver/resolve_issue.py @@ -336,13 +336,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/runtime/base.py b/openhands/runtime/base.py index b12c501c19f3..74891a7d52b0 100644 --- a/openhands/runtime/base.py +++ b/openhands/runtime/base.py @@ -47,11 +47,19 @@ } -class RuntimeNotReadyError(Exception): +class RuntimeUnavailableError(Exception): pass -class RuntimeDisconnectedError(Exception): +class RuntimeNotReadyError(RuntimeUnavailableError): + pass + + +class RuntimeDisconnectedError(RuntimeUnavailableError): + pass + + +class RuntimeNotFoundError(RuntimeUnavailableError): pass diff --git a/openhands/runtime/impl/eventstream/eventstream_runtime.py b/openhands/runtime/impl/eventstream/eventstream_runtime.py index 2b0ed19ecee8..b55a66eb0b69 100644 --- a/openhands/runtime/impl/eventstream/eventstream_runtime.py +++ b/openhands/runtime/impl/eventstream/eventstream_runtime.py @@ -34,7 +34,11 @@ ) from openhands.events.serialization import event_to_dict, observation_from_dict from openhands.events.serialization.action import ACTION_TYPE_TO_CLASS -from openhands.runtime.base import Runtime +from openhands.runtime.base import ( + Runtime, + RuntimeDisconnectedError, + RuntimeNotFoundError, +) from openhands.runtime.builder import DockerRuntimeBuilder from openhands.runtime.impl.eventstream.containers import remove_all_containers from openhands.runtime.plugins import PluginRequirement @@ -429,10 +433,22 @@ def _refresh_logs(self): @tenacity.retry( stop=tenacity.stop_after_delay(120) | stop_if_should_exit(), - reraise=(ConnectionRefusedError,), + retry=tenacity.retry_if_exception_type( + (ConnectionError, requests.exceptions.ConnectionError) + ), + reraise=True, wait=tenacity.wait_fixed(2), ) def _wait_until_alive(self): + try: + container = self.docker_client.containers.get(self.container_name) + if container.status == 'exited': + raise RuntimeDisconnectedError( + f'Container {self.container_name} has exited.' + ) + except docker.errors.NotFound: + raise RuntimeNotFoundError(f'Container {self.container_name} not found.') + self._refresh_logs() if not self.log_buffer: raise RuntimeError('Runtime client is not ready.') diff --git a/openhands/runtime/impl/remote/remote_runtime.py b/openhands/runtime/impl/remote/remote_runtime.py index 4191a047b1c2..d996441b66c9 100644 --- a/openhands/runtime/impl/remote/remote_runtime.py +++ b/openhands/runtime/impl/remote/remote_runtime.py @@ -31,6 +31,7 @@ from openhands.runtime.base import ( Runtime, RuntimeDisconnectedError, + RuntimeNotFoundError, RuntimeNotReadyError, ) from openhands.runtime.builder.remote import RemoteRuntimeBuilder @@ -109,7 +110,9 @@ def _start_or_attach_to_runtime(self): if existing_runtime: self.log('debug', f'Using existing runtime with ID: {self.runtime_id}') elif self.attach_to_existing: - raise RuntimeError('Could not find existing runtime to attach to.') + raise RuntimeNotFoundError( + f'Could not find existing runtime for SID: {self.sid}' + ) else: self.send_status_message('STATUS$STARTING_CONTAINER') if self.config.sandbox.runtime_container_image is None: diff --git a/openhands/server/listen.py b/openhands/server/listen.py index 8724daf1905f..433b13bde208 100644 --- a/openhands/server/listen.py +++ b/openhands/server/listen.py @@ -34,6 +34,7 @@ Request, UploadFile, WebSocket, + WebSocketDisconnect, status, ) from fastapi.responses import FileResponse, JSONResponse @@ -63,7 +64,12 @@ from openhands.llm import bedrock from openhands.runtime.base import Runtime from openhands.server.auth.auth import get_sid_from_token, sign_token -from openhands.server.middleware import LocalhostCORSMiddleware, NoCacheMiddleware +from openhands.server.middleware import ( + InMemoryRateLimiter, + LocalhostCORSMiddleware, + NoCacheMiddleware, + RateLimitMiddleware, +) from openhands.server.session import SessionManager load_dotenv() @@ -83,6 +89,10 @@ app.add_middleware(NoCacheMiddleware) +app.add_middleware( + RateLimitMiddleware, rate_limiter=InMemoryRateLimiter(requests=2, seconds=1) +) + security_scheme = HTTPBearer() @@ -238,7 +248,8 @@ async def attach_session(request: Request, call_next): request.state.conversation = await session_manager.attach_to_conversation( request.state.sid ) - if request.state.conversation is None: + if not request.state.conversation: + logger.error(f'Runtime not found for session: {request.state.sid}') return JSONResponse( status_code=status.HTTP_404_NOT_FOUND, content={'error': 'Session not found'}, @@ -344,7 +355,13 @@ async def websocket_endpoint(websocket: WebSocket): latest_event_id = -1 if websocket.query_params.get('latest_event_id'): - latest_event_id = int(websocket.query_params.get('latest_event_id')) + try: + latest_event_id = int(websocket.query_params.get('latest_event_id')) + except ValueError: + logger.warning( + f'Invalid latest_event_id: {websocket.query_params.get("latest_event_id")}' + ) + pass async_stream = AsyncEventStreamWrapper( session.agent_session.event_stream, latest_event_id + 1 @@ -361,7 +378,14 @@ async def websocket_endpoint(websocket: WebSocket): ), ): continue - await websocket.send_json(event_to_dict(event)) + try: + await websocket.send_json(event_to_dict(event)) + except WebSocketDisconnect: + logger.warning( + 'Websocket disconnected while sending event history, before loop started' + ) + session.close() + return await session.loop_recv() diff --git a/openhands/server/middleware.py b/openhands/server/middleware.py index 218a949fca58..872241fc865f 100644 --- a/openhands/server/middleware.py +++ b/openhands/server/middleware.py @@ -1,6 +1,11 @@ +import asyncio +from collections import defaultdict +from datetime import datetime, timedelta from urllib.parse import urlparse +from fastapi import Request from fastapi.middleware.cors import CORSMiddleware +from fastapi.responses import JSONResponse from starlette.middleware.base import BaseHTTPMiddleware from starlette.types import ASGIApp @@ -41,3 +46,55 @@ async def dispatch(self, request, call_next): response.headers['Pragma'] = 'no-cache' response.headers['Expires'] = '0' return response + + +class InMemoryRateLimiter: + history: dict + requests: int + seconds: int + sleep_seconds: int + + def __init__(self, requests: int = 2, seconds: int = 1, sleep_seconds: int = 1): + self.requests = requests + self.seconds = seconds + self.history = defaultdict(list) + + def _clean_old_requests(self, key: str) -> None: + now = datetime.now() + cutoff = now - timedelta(seconds=self.seconds) + self.history[key] = [ts for ts in self.history[key] if ts > cutoff] + + async def __call__(self, request: Request) -> bool: + key = request.client.host + now = datetime.now() + + self._clean_old_requests(key) + + self.history[key].append(now) + + if len(self.history[key]) > self.requests * 2: + return False + elif len(self.history[key]) > self.requests: + if self.sleep_seconds > 0: + await asyncio.sleep(self.sleep_seconds) + return True + else: + return False + + return True + + +class RateLimitMiddleware(BaseHTTPMiddleware): + def __init__(self, app: ASGIApp, rate_limiter: InMemoryRateLimiter): + super().__init__(app) + self.rate_limiter = rate_limiter + + async def dispatch(self, request, call_next): + ok = await self.rate_limiter(request) + if not ok: + return JSONResponse( + status_code=429, + content={'message': 'Too many requests'}, + headers={'Retry-After': '1'}, + ) + return await call_next(request) diff --git a/openhands/server/session/agent_session.py b/openhands/server/session/agent_session.py index 8f9d20a5dc6e..76f6e2aa8bcb 100644 --- a/openhands/server/session/agent_session.py +++ b/openhands/server/session/agent_session.py @@ -11,7 +11,7 @@ from openhands.events.event import EventSource from openhands.events.stream import EventStream from openhands.runtime import get_runtime_cls -from openhands.runtime.base import Runtime +from openhands.runtime.base import Runtime, RuntimeUnavailableError from openhands.security import SecurityAnalyzer, options from openhands.storage.files import FileStore @@ -194,13 +194,13 @@ async def _create_runtime( try: await self.runtime.connect() - except Exception as e: + except RuntimeUnavailableError as e: logger.error(f'Runtime initialization failed: {e}', exc_info=True) if self._status_callback: self._status_callback( 'error', 'STATUS$ERROR_RUNTIME_DISCONNECTED', str(e) ) - raise + return if self.runtime is not None: logger.debug( diff --git a/openhands/server/session/manager.py b/openhands/server/session/manager.py index f746b3676e29..790b7c4bd1eb 100644 --- a/openhands/server/session/manager.py +++ b/openhands/server/session/manager.py @@ -6,6 +6,7 @@ from openhands.core.config import AppConfig from openhands.core.logger import openhands_logger as logger from openhands.events.stream import session_exists +from openhands.runtime.base import RuntimeUnavailableError from openhands.server.session.conversation import Conversation from openhands.server.session.session import Session from openhands.storage.files import FileStore @@ -26,7 +27,11 @@ async def attach_to_conversation(self, sid: str) -> Conversation | None: if not await session_exists(sid, self.file_store): return None c = Conversation(sid, file_store=self.file_store, config=self.config) - await c.connect() + try: + await c.connect() + except RuntimeUnavailableError as e: + logger.error(f'Error connecting to conversation {c.sid}: {e}') + return None end_time = time.time() logger.info( f'Conversation {c.sid} connected in {end_time - start_time} seconds' diff --git a/tests/unit/resolver/test_issue_handler.py b/tests/unit/resolver/test_issue_handler.py index e03eb1897655..d0c17d9088e9 100644 --- a/tests/unit/resolver/test_issue_handler.py +++ b/tests/unit/resolver/test_issue_handler.py @@ -30,7 +30,7 @@ def test_get_converted_issues_initializes_review_comments(): 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 @@ -158,7 +158,7 @@ def test_pr_handler_get_converted_issues_with_comments(): 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 @@ -364,7 +364,9 @@ def test_pr_handler_get_converted_issues_with_specific_thread_comment(): 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 @@ -464,7 +466,9 @@ def test_pr_handler_get_converted_issues_with_specific_review_thread_comment(): 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 @@ -584,7 +588,9 @@ def test_pr_handler_get_converted_issues_with_specific_comment_and_issue_refs(): 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 @@ -681,7 +687,7 @@ def test_pr_handler_get_converted_issues_with_duplicate_issue_refs(): 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 diff --git a/tests/unit/resolver/test_resolve_issues.py b/tests/unit/resolver/test_resolve_issues.py index 21784f186ee7..7851d8c2f8ee 100644 --- a/tests/unit/resolver/test_resolve_issues.py +++ b/tests/unit/resolver/test_resolve_issues.py @@ -113,7 +113,7 @@ def get_mock_response(url, *args, **kwargs): return mock_issues_response with patch('requests.get', side_effect=get_mock_response): - issues = handler.get_converted_issues() + issues = handler.get_converted_issues(issue_numbers=[1, 3]) assert len(issues) == 2 assert handler.issue_type == 'issue' @@ -226,7 +226,7 @@ def get_mock_response(url, *args, **kwargs): with patch('requests.get', side_effect=get_mock_response): with patch('requests.post', return_value=mock_graphql_response): - issues = handler.get_converted_issues() + issues = handler.get_converted_issues(issue_numbers=[1, 2, 3]) assert len(issues) == 3 assert handler.issue_type == 'pr' @@ -790,7 +790,7 @@ def get_mock_response(url, *args, **kwargs): with patch('requests.get', side_effect=get_mock_response): with patch('requests.post', return_value=mock_graphql_response): - issues = handler.get_converted_issues() + issues = handler.get_converted_issues(issue_numbers=[1]) assert len(issues) == 1 assert handler.issue_type == 'pr' @@ -846,7 +846,9 @@ def get_mock_response(url, *args, **kwargs): return mock_issue_response with patch('requests.get', side_effect=get_mock_response): - issues = handler.get_converted_issues(comment_id=specific_comment_id) + issues = handler.get_converted_issues( + issue_numbers=[1], comment_id=specific_comment_id + ) assert len(issues) == 1 assert issues[0].number == 1 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()