From 6772227c9dc82ac5036d0d360213f046613ee065 Mon Sep 17 00:00:00 2001 From: Eric Zhang Date: Wed, 12 Feb 2025 00:46:53 +0800 Subject: [PATCH 1/9] fix(frontend): fix public github repo cannot be selected (#6680) --- .../src/components/features/github/github-repo-selector.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/src/components/features/github/github-repo-selector.tsx b/frontend/src/components/features/github/github-repo-selector.tsx index 722134a42ddb..750474ded3c6 100644 --- a/frontend/src/components/features/github/github-repo-selector.tsx +++ b/frontend/src/components/features/github/github-repo-selector.tsx @@ -31,7 +31,7 @@ export function GitHubRepositorySelector({ const allRepositories: GitHubRepository[] = [ ...publicRepositories.filter( - (repo) => !publicRepositories.find((r) => r.id === repo.id), + (repo) => !userRepositories.find((r) => r.id === repo.id), ), ...userRepositories, ]; From 3188646195dce5f884c5ac56266476b1ec10b2e4 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Tue, 11 Feb 2025 12:37:44 -0500 Subject: [PATCH 2/9] refactor(runtime): Use openhands-aci file editor directly in runtime instead of execute it through ipython (#6671) Co-authored-by: openhands Co-authored-by: Graham Neubig Co-authored-by: Engel Nyst --- frontend/src/types/core/actions.ts | 17 +- .../codeact_agent/function_calling.py | 28 +- openhands/events/action/files.py | 76 +- openhands/events/observation/files.py | 17 +- openhands/events/serialization/action.py | 39 +- openhands/events/serialization/observation.py | 28 +- openhands/runtime/action_execution_server.py | 242 +++--- .../action_execution_client.py | 11 +- openhands/runtime/utils/edit.py | 12 +- openhands/runtime/utils/files.py | 4 +- tests/runtime/test_aci_edit.py | 692 ++++++++++++++++++ tests/runtime/test_ipython.py | 64 -- .../{test_edit.py => test_llm_based_edit.py} | 0 tests/unit/test_action_serialization.py | 210 +++++- tests/unit/test_observation_serialization.py | 87 +++ 15 files changed, 1288 insertions(+), 239 deletions(-) create mode 100644 tests/runtime/test_aci_edit.py rename tests/runtime/{test_edit.py => test_llm_based_edit.py} (100%) diff --git a/frontend/src/types/core/actions.ts b/frontend/src/types/core/actions.ts index eb8aba6ada63..21c76d5d22a3 100644 --- a/frontend/src/types/core/actions.ts +++ b/frontend/src/types/core/actions.ts @@ -83,7 +83,9 @@ export interface FileReadAction extends OpenHandsActionEvent<"read"> { args: { path: string; thought: string; - translated_ipython_code: string | null; + security_risk: ActionSecurityRisk | null; + impl_source?: string; + view_range?: number[] | null; }; } @@ -100,7 +102,18 @@ export interface FileEditAction extends OpenHandsActionEvent<"edit"> { source: "agent"; args: { path: string; - translated_ipython_code: string; + command?: string; + file_text?: string | null; + view_range?: number[] | null; + old_str?: string | null; + new_str?: string | null; + insert_line?: number | null; + content?: string; + start?: number; + end?: number; + thought: string; + security_risk: ActionSecurityRisk | null; + impl_source?: string; }; } diff --git a/openhands/agenthub/codeact_agent/function_calling.py b/openhands/agenthub/codeact_agent/function_calling.py index 196457db0f6e..be39a1624ab5 100644 --- a/openhands/agenthub/codeact_agent/function_calling.py +++ b/openhands/agenthub/codeact_agent/function_calling.py @@ -16,7 +16,6 @@ FunctionCallNotExistsError, FunctionCallValidationError, ) -from openhands.core.logger import openhands_logger as logger from openhands.events.action import ( Action, AgentDelegateAction, @@ -541,26 +540,27 @@ def response_to_actions(response: ModelResponse) -> list[Action]: raise FunctionCallValidationError( f'Missing required argument "path" in tool call {tool_call.function.name}' ) + path = arguments['path'] + command = arguments['command'] + other_kwargs = { + k: v for k, v in arguments.items() if k not in ['command', 'path'] + } - # We implement this in agent_skills, which can be used via Jupyter - # convert tool_call.function.arguments to kwargs that can be passed to file_editor - code = f'print(file_editor(**{arguments}))' - logger.debug( - f'TOOL CALL: str_replace_editor -> file_editor with code: {code}' - ) - - if arguments['command'] == 'view': + if command == 'view': action = FileReadAction( - path=arguments['path'], - translated_ipython_code=code, + path=path, impl_source=FileReadSource.OH_ACI, + view_range=other_kwargs.get('view_range', None), ) else: + if 'view_range' in other_kwargs: + # Remove view_range from other_kwargs since it is not needed for FileEditAction + other_kwargs.pop('view_range') action = FileEditAction( - path=arguments['path'], - content='', # dummy value -- we don't need it - translated_ipython_code=code, + path=path, + command=command, impl_source=FileEditSource.OH_ACI, + **other_kwargs, ) elif tool_call.function.name == 'browser': if 'code' not in arguments: diff --git a/openhands/events/action/files.py b/openhands/events/action/files.py index de2db691f146..ac626c02f50c 100644 --- a/openhands/events/action/files.py +++ b/openhands/events/action/files.py @@ -21,7 +21,7 @@ class FileReadAction(Action): runnable: ClassVar[bool] = True security_risk: ActionSecurityRisk | None = None impl_source: FileReadSource = FileReadSource.DEFAULT - translated_ipython_code: str = '' # translated openhands-aci IPython code + view_range: list[int] | None = None # ONLY used in OH_ACI mode @property def message(self) -> str: @@ -60,29 +60,79 @@ def __repr__(self) -> str: @dataclass class FileEditAction(Action): - """Edits a file by provided a draft at a given path. - - Can be set to edit specific lines using start and end (1-index, inclusive) if the file is too long. - Default lines 1:-1 (whole file). - - If start is set to -1, the FileEditAction will simply append the content to the file. + """Edits a file using various commands including view, create, str_replace, insert, and undo_edit. + + This class supports two main modes of operation: + 1. LLM-based editing (impl_source = FileEditSource.LLM_BASED_EDIT) + 2. ACI-based editing (impl_source = FileEditSource.OH_ACI) + + Attributes: + path (str): The path to the file being edited. Works for both LLM-based and OH_ACI editing. + OH_ACI only arguments: + command (str): The editing command to be performed (view, create, str_replace, insert, undo_edit, write). + file_text (str): The content of the file to be created (used with 'create' command in OH_ACI mode). + old_str (str): The string to be replaced (used with 'str_replace' command in OH_ACI mode). + new_str (str): The string to replace old_str (used with 'str_replace' and 'insert' commands in OH_ACI mode). + insert_line (int): The line number after which to insert new_str (used with 'insert' command in OH_ACI mode). + LLM-based editing arguments: + content (str): The content to be written or edited in the file (used in LLM-based editing and 'write' command). + start (int): The starting line for editing (1-indexed, inclusive). Default is 1. + end (int): The ending line for editing (1-indexed, inclusive). Default is -1 (end of file). + thought (str): The reasoning behind the edit action. + action (str): The type of action being performed (always ActionType.EDIT). + runnable (bool): Indicates if the action can be executed (always True). + security_risk (ActionSecurityRisk | None): Indicates any security risks associated with the action. + impl_source (FileEditSource): The source of the implementation (LLM_BASED_EDIT or OH_ACI). + + Usage: + - For LLM-based editing: Use path, content, start, and end attributes. + - For ACI-based editing: Use path, command, and the appropriate attributes for the specific command. + + Note: + - If start is set to -1 in LLM-based editing, the content will be appended to the file. + - The 'write' command behaves similarly to LLM-based editing, using content, start, and end attributes. """ path: str - content: str + + # OH_ACI arguments + command: str = '' + file_text: str | None = None + old_str: str | None = None + new_str: str | None = None + insert_line: int | None = None + + # LLM-based editing arguments + content: str = '' start: int = 1 end: int = -1 + + # Shared arguments thought: str = '' action: str = ActionType.EDIT runnable: ClassVar[bool] = True security_risk: ActionSecurityRisk | None = None - impl_source: FileEditSource = FileEditSource.LLM_BASED_EDIT - translated_ipython_code: str = '' + impl_source: FileEditSource = FileEditSource.OH_ACI def __repr__(self) -> str: ret = '**FileEditAction**\n' - ret += f'Thought: {self.thought}\n' - ret += f'Range: [L{self.start}:L{self.end}]\n' ret += f'Path: [{self.path}]\n' - ret += f'Content:\n```\n{self.content}\n```\n' + ret += f'Thought: {self.thought}\n' + + if self.impl_source == FileEditSource.LLM_BASED_EDIT: + ret += f'Range: [L{self.start}:L{self.end}]\n' + ret += f'Content:\n```\n{self.content}\n```\n' + else: # OH_ACI mode + ret += f'Command: {self.command}\n' + if self.command == 'create': + ret += f'Created File with Text:\n```\n{self.file_text}\n```\n' + elif self.command == 'str_replace': + ret += f'Old String: ```\n{self.old_str}\n```\n' + ret += f'New String: ```\n{self.new_str}\n```\n' + elif self.command == 'insert': + ret += f'Insert Line: {self.insert_line}\n' + ret += f'New String: ```\n{self.new_str}\n```\n' + elif self.command == 'undo_edit': + ret += 'Undo Edit\n' + # We ignore "view" command because it will be mapped to a FileReadAction return ret diff --git a/openhands/events/observation/files.py b/openhands/events/observation/files.py index cc921052312c..0b988852c2c5 100644 --- a/openhands/events/observation/files.py +++ b/openhands/events/observation/files.py @@ -50,15 +50,18 @@ class FileEditObservation(Observation): The observation includes both the old and new content of the file, and can generate a diff visualization showing the changes. The diff is computed lazily and cached to improve performance. + + The .content property can either be: + - Git diff in LLM-based editing mode + - the rendered message sent to the LLM in OH_ACI mode (e.g., "The file /path/to/file.txt is created with the provided content.") """ - path: str - prev_exist: bool - old_content: str - new_content: str + path: str = '' + prev_exist: bool = False + old_content: str | None = None + new_content: str | None = None observation: str = ObservationType.EDIT impl_source: FileEditSource = FileEditSource.LLM_BASED_EDIT - formatted_output_and_error: str = '' _diff_cache: str | None = None # Cache for the diff visualization @property @@ -75,6 +78,8 @@ def get_edit_groups(self, n_context_lines: int = 2) -> list[dict[str, list[str]] Returns: A list of edit groups, where each group contains before/after edits. """ + if self.old_content is None or self.new_content is None: + return [] old_lines = self.old_content.split('\n') new_lines = self.new_content.split('\n') # Borrowed from difflib.unified_diff to directly parse into structured format @@ -173,7 +178,7 @@ def visualize_diff( def __str__(self) -> str: """Get a string representation of the file edit observation.""" if self.impl_source == FileEditSource.OH_ACI: - return self.formatted_output_and_error + return self.content if not self.prev_exist: assert ( diff --git a/openhands/events/serialization/action.py b/openhands/events/serialization/action.py index be9990750fc6..b6b09ebad318 100644 --- a/openhands/events/serialization/action.py +++ b/openhands/events/serialization/action.py @@ -1,3 +1,5 @@ +import re + from openhands.core.exceptions import LLMMalformedActionError from openhands.events.action.action import Action from openhands.events.action.agent import ( @@ -38,6 +40,38 @@ ACTION_TYPE_TO_CLASS = {action_class.action: action_class for action_class in actions} # type: ignore[attr-defined] +def handle_action_deprecated_args(args: dict) -> dict: + # keep_prompt has been deprecated in https://github.com/All-Hands-AI/OpenHands/pull/4881 + if 'keep_prompt' in args: + args.pop('keep_prompt') + + # Handle translated_ipython_code deprecation + if 'translated_ipython_code' in args: + code = args.pop('translated_ipython_code') + + # Check if it's a file_editor call + file_editor_pattern = r'print\(file_editor\(\*\*(.*?)\)\)' + if code is not None and (match := re.match(file_editor_pattern, code)): + try: + # Extract and evaluate the dictionary string + import ast + + file_args = ast.literal_eval(match.group(1)) + + # Update args with the extracted file editor arguments + args.update(file_args) + except (ValueError, SyntaxError): + # If parsing fails, just remove the translated_ipython_code + pass + + if args.get('command') == 'view': + args.pop( + 'command' + ) # "view" will be translated to FileReadAction which doesn't have a command argument + + return args + + def action_from_dict(action: dict) -> Action: if not isinstance(action, dict): raise LLMMalformedActionError('action must be a dictionary') @@ -67,9 +101,8 @@ def action_from_dict(action: dict) -> Action: if 'images_urls' in args: args['image_urls'] = args.pop('images_urls') - # keep_prompt has been deprecated in https://github.com/All-Hands-AI/OpenHands/pull/4881 - if 'keep_prompt' in args: - args.pop('keep_prompt') + # handle deprecated args + args = handle_action_deprecated_args(args) try: decoded_action = action_class(**args) diff --git a/openhands/events/serialization/observation.py b/openhands/events/serialization/observation.py index f1ee333c019a..89164fff1e80 100644 --- a/openhands/events/serialization/observation.py +++ b/openhands/events/serialization/observation.py @@ -64,6 +64,23 @@ def _update_cmd_output_metadata( return metadata +def handle_observation_deprecated_extras(extras: dict) -> dict: + # These are deprecated in https://github.com/All-Hands-AI/OpenHands/pull/4881 + if 'exit_code' in extras: + extras['metadata'] = _update_cmd_output_metadata( + extras.get('metadata', None), exit_code=extras.pop('exit_code') + ) + if 'command_id' in extras: + extras['metadata'] = _update_cmd_output_metadata( + extras.get('metadata', None), pid=extras.pop('command_id') + ) + + # formatted_output_and_error has been deprecated in https://github.com/All-Hands-AI/OpenHands/pull/6671 + if 'formatted_output_and_error' in extras: + extras.pop('formatted_output_and_error') + return extras + + def observation_from_dict(observation: dict) -> Observation: observation = observation.copy() if 'observation' not in observation: @@ -78,15 +95,8 @@ def observation_from_dict(observation: dict) -> Observation: content = observation.pop('content', '') extras = copy.deepcopy(observation.pop('extras', {})) - # Handle legacy attributes for CmdOutputObservation - if 'exit_code' in extras: - extras['metadata'] = _update_cmd_output_metadata( - extras.get('metadata', None), exit_code=extras.pop('exit_code') - ) - if 'command_id' in extras: - extras['metadata'] = _update_cmd_output_metadata( - extras.get('metadata', None), pid=extras.pop('command_id') - ) + extras = handle_observation_deprecated_extras(extras) + # convert metadata to CmdOutputMetadata if it is a dict if observation_class is CmdOutputObservation: if 'metadata' in extras and isinstance(extras['metadata'], dict): diff --git a/openhands/runtime/action_execution_server.py b/openhands/runtime/action_execution_server.py index f2d12196fede..ccf267bb2ca6 100644 --- a/openhands/runtime/action_execution_server.py +++ b/openhands/runtime/action_execution_server.py @@ -9,10 +9,8 @@ import asyncio import base64 import io -import json import mimetypes import os -import re import shutil import tempfile import time @@ -26,7 +24,9 @@ from fastapi.exceptions import RequestValidationError from fastapi.responses import JSONResponse, StreamingResponse from fastapi.security import APIKeyHeader -from openhands_aci.utils.diff import get_diff +from openhands_aci.editor.editor import OHEditor +from openhands_aci.editor.exceptions import ToolError +from openhands_aci.editor.results import ToolResult from pydantic import BaseModel from starlette.exceptions import HTTPException as StarletteHTTPException from uvicorn import run @@ -37,6 +37,7 @@ BrowseInteractiveAction, BrowseURLAction, CmdRunAction, + FileEditAction, FileReadAction, FileWriteAction, IPythonRunCellAction, @@ -78,6 +79,58 @@ def verify_api_key(api_key: str = Depends(api_key_header)): return api_key +def _execute_file_editor( + editor: OHEditor, + command: str, + path: str, + file_text: str | None = None, + view_range: list[int] | None = None, + old_str: str | None = None, + new_str: str | None = None, + insert_line: int | None = None, + enable_linting: bool = False, +) -> str: + """Execute file editor command and handle exceptions. + + Args: + editor: The OHEditor instance + command: Editor command to execute + path: File path + file_text: Optional file text content + view_range: Optional view range tuple (start, end) + old_str: Optional string to replace + new_str: Optional replacement string + insert_line: Optional line number for insertion + enable_linting: Whether to enable linting + + Returns: + str: Result string from the editor operation + """ + result: ToolResult | None = None + try: + result = editor( + command=command, + path=path, + file_text=file_text, + view_range=view_range, + old_str=old_str, + new_str=new_str, + insert_line=insert_line, + enable_linting=enable_linting, + ) + except ToolError as e: + result = ToolResult(error=e.message) + + if result.error: + return f'ERROR:\n{result.error}' + + if not result.output: + logger.warning(f'No output from file_editor for {path}') + return '' + + return result.output + + class ActionExecutor: """ActionExecutor is running inside docker sandbox. It is responsible for executing actions received from OpenHands backend and producing observations. @@ -104,6 +157,7 @@ def __init__( self.bash_session: BashSession | None = None self.lock = asyncio.Lock() self.plugins: dict[str, Plugin] = {} + self.file_editor = OHEditor() self.browser = BrowserEnv(browsergym_eval_env) self.start_time = time.time() self.last_execution_time = self.start_time @@ -237,65 +291,6 @@ async def run_ipython(self, action: IPythonRunCellAction) -> Observation: obs: IPythonRunCellObservation = await _jupyter_plugin.run(action) obs.content = obs.content.rstrip() - matches = re.findall( - r'(.*?)', - obs.content, - re.DOTALL, - ) - if matches: - results: list[str] = [] - if len(matches) == 1: - # Use specific actions/observations types - match = matches[0] - try: - result_dict = json.loads(match) - if result_dict.get('path'): # Successful output - if ( - result_dict['new_content'] is not None - ): # File edit commands - diff = get_diff( - old_contents=result_dict['old_content'] - or '', # old_content is None when file is created - new_contents=result_dict['new_content'], - filepath=result_dict['path'], - ) - return FileEditObservation( - content=diff, - path=result_dict['path'], - old_content=result_dict['old_content'], - new_content=result_dict['new_content'], - prev_exist=result_dict['prev_exist'], - impl_source=FileEditSource.OH_ACI, - formatted_output_and_error=result_dict[ - 'formatted_output_and_error' - ], - ) - else: # File view commands - return FileReadObservation( - content=result_dict['formatted_output_and_error'], - path=result_dict['path'], - impl_source=FileReadSource.OH_ACI, - ) - else: # Error output - results.append(result_dict['formatted_output_and_error']) - except json.JSONDecodeError: - # Handle JSON decoding errors if necessary - results.append( - f"Invalid JSON in 'openhands-aci' output: {match}" - ) - else: - for match in matches: - try: - result_dict = json.loads(match) - results.append(result_dict['formatted_output_and_error']) - except json.JSONDecodeError: - # Handle JSON decoding errors if necessary - results.append( - f"Invalid JSON in 'openhands-aci' output: {match}" - ) - - # Combine the results (e.g., join them) or handle them as required - obs.content = '\n'.join(str(result) for result in results) if action.include_extra: obs.content += ( @@ -317,11 +312,17 @@ def _resolve_path(self, path: str, working_dir: str) -> str: async def read(self, action: FileReadAction) -> Observation: assert self.bash_session is not None if action.impl_source == FileReadSource.OH_ACI: - return await self.run_ipython( - IPythonRunCellAction( - code=action.translated_ipython_code, - include_extra=False, - ) + result_str = _execute_file_editor( + self.file_editor, + command='view', + path=action.path, + view_range=action.view_range, + ) + + return FileReadObservation( + content=result_str, + path=action.path, + impl_source=FileReadSource.OH_ACI, ) # NOTE: the client code is running inside the sandbox, @@ -378,56 +379,75 @@ async def write(self, action: FileWriteAction) -> Observation: filepath = self._resolve_path(action.path, working_dir) insert = action.content.split('\n') + if not os.path.exists(os.path.dirname(filepath)): + os.makedirs(os.path.dirname(filepath)) + + file_exists = os.path.exists(filepath) + if file_exists: + file_stat = os.stat(filepath) + else: + file_stat = None + + mode = 'w' if not file_exists else 'r+' try: - if not os.path.exists(os.path.dirname(filepath)): - os.makedirs(os.path.dirname(filepath)) + with open(filepath, mode, encoding='utf-8') as file: + if mode != 'w': + all_lines = file.readlines() + new_file = insert_lines(insert, all_lines, action.start, action.end) + else: + new_file = [i + '\n' for i in insert] + + file.seek(0) + file.writelines(new_file) + file.truncate() - file_exists = os.path.exists(filepath) + except FileNotFoundError: + return ErrorObservation(f'File not found: {filepath}') + except IsADirectoryError: + return ErrorObservation( + f'Path is a directory: {filepath}. You can only write to files' + ) + except UnicodeDecodeError: + return ErrorObservation(f'File could not be decoded as utf-8: {filepath}') + + # Attempt to handle file permissions + try: if file_exists: - file_stat = os.stat(filepath) + assert file_stat is not None + # restore the original file permissions if the file already exists + os.chmod(filepath, file_stat.st_mode) + os.chown(filepath, file_stat.st_uid, file_stat.st_gid) else: - file_stat = None - - mode = 'w' if not file_exists else 'r+' - try: - with open(filepath, mode, encoding='utf-8') as file: - if mode != 'w': - all_lines = file.readlines() - new_file = insert_lines( - insert, all_lines, action.start, action.end - ) - else: - new_file = [i + '\n' for i in insert] - - file.seek(0) - file.writelines(new_file) - file.truncate() - - # Handle file permissions - if file_exists: - assert file_stat is not None - # restore the original file permissions if the file already exists - os.chmod(filepath, file_stat.st_mode) - os.chown(filepath, file_stat.st_uid, file_stat.st_gid) - else: - # set the new file permissions if the file is new - os.chmod(filepath, 0o664) - os.chown(filepath, self.user_id, self.user_id) - - except FileNotFoundError: - return ErrorObservation(f'File not found: {filepath}') - except IsADirectoryError: - return ErrorObservation( - f'Path is a directory: {filepath}. You can only write to files' - ) - except UnicodeDecodeError: - return ErrorObservation( - f'File could not be decoded as utf-8: {filepath}' - ) - except PermissionError: - return ErrorObservation(f'Malformed paths not permitted: {filepath}') + # set the new file permissions if the file is new + os.chmod(filepath, 0o664) + os.chown(filepath, self.user_id, self.user_id) + except PermissionError as e: + return ErrorObservation( + f'File {filepath} written, but failed to change ownership and permissions: {e}' + ) return FileWriteObservation(content='', path=filepath) + async def edit(self, action: FileEditAction) -> Observation: + assert action.impl_source == FileEditSource.OH_ACI + result_str = _execute_file_editor( + self.file_editor, + command=action.command, + path=action.path, + file_text=action.file_text, + old_str=action.old_str, + new_str=action.new_str, + insert_line=action.insert_line, + enable_linting=False, + ) + + return FileEditObservation( + content=result_str, + path=action.path, + old_content=action.old_str, + new_content=action.new_str, + impl_source=FileEditSource.OH_ACI, + ) + async def browse(self, action: BrowseURLAction) -> Observation: return await browse(action, self.browser) diff --git a/openhands/runtime/impl/action_execution/action_execution_client.py b/openhands/runtime/impl/action_execution/action_execution_client.py index 7f1282447461..258dcf3a85f7 100644 --- a/openhands/runtime/impl/action_execution/action_execution_client.py +++ b/openhands/runtime/impl/action_execution/action_execution_client.py @@ -24,6 +24,7 @@ IPythonRunCellAction, ) from openhands.events.action.action import Action +from openhands.events.action.files import FileEditSource from openhands.events.observation import ( ErrorObservation, NullObservation, @@ -217,8 +218,11 @@ def get_vscode_token(self) -> str: return '' def send_action_for_execution(self, action: Action) -> Observation: - if isinstance(action, FileEditAction): - return self.edit(action) + if ( + isinstance(action, FileEditAction) + and action.impl_source == FileEditSource.LLM_BASED_EDIT + ): + return self.llm_based_edit(action) # set timeout to default if not set if action.timeout is None: @@ -281,6 +285,9 @@ def read(self, action: FileReadAction) -> Observation: def write(self, action: FileWriteAction) -> Observation: return self.send_action_for_execution(action) + def edit(self, action: FileEditAction) -> Observation: + return self.send_action_for_execution(action) + def browse(self, action: BrowseURLAction) -> Observation: return self.send_action_for_execution(action) diff --git a/openhands/runtime/utils/edit.py b/openhands/runtime/utils/edit.py index 3dce0544f0a7..a66b2039674d 100644 --- a/openhands/runtime/utils/edit.py +++ b/openhands/runtime/utils/edit.py @@ -13,7 +13,6 @@ FileWriteAction, IPythonRunCellAction, ) -from openhands.events.event import FileEditSource from openhands.events.observation import ( ErrorObservation, FileEditObservation, @@ -205,16 +204,7 @@ def _get_lint_error( return ErrorObservation(error_message) return None - def edit(self, action: FileEditAction) -> Observation: - if action.impl_source == FileEditSource.OH_ACI: - # Translate to ipython command to file_editor - return self.run_ipython( - IPythonRunCellAction( - code=action.translated_ipython_code, - include_extra=False, - ) - ) - + def llm_based_edit(self, action: FileEditAction) -> Observation: obs = self.read(FileReadAction(path=action.path)) if ( isinstance(obs, ErrorObservation) diff --git a/openhands/runtime/utils/files.py b/openhands/runtime/utils/files.py index b9664cafc45f..1d54c90b3609 100644 --- a/openhands/runtime/utils/files.py +++ b/openhands/runtime/utils/files.py @@ -140,6 +140,6 @@ async def write_file( ) except UnicodeDecodeError: return ErrorObservation(f'File could not be decoded as utf-8: {path}') - except PermissionError: - return ErrorObservation(f'Malformed paths not permitted: {path}') + except PermissionError as e: + return ErrorObservation(f'Permission error on {path}: {e}') return FileWriteObservation(content='', path=path) diff --git a/tests/runtime/test_aci_edit.py b/tests/runtime/test_aci_edit.py new file mode 100644 index 000000000000..e6de0410e8f8 --- /dev/null +++ b/tests/runtime/test_aci_edit.py @@ -0,0 +1,692 @@ +"""Editor-related tests for the DockerRuntime.""" + +import os + +from conftest import _close_test_runtime, _load_runtime + +from openhands.core.logger import openhands_logger as logger +from openhands.events.action import FileEditAction, FileWriteAction + + +def test_view_file(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + # Create test file + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='This is a test file.\nThis file is for testing purposes.', + path=test_file, + ) + obs = runtime.run_action(action) + + # Test view command + action = FileEditAction( + command='view', + path=test_file, + ) + obs = runtime.run_action(action) + + assert f"Here's the result of running `cat -n` on {test_file}:" in obs.content + assert '1\tThis is a test file.' in obs.content + assert '2\tThis file is for testing purposes.' in obs.content + + finally: + _close_test_runtime(runtime) + + +def test_view_directory(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + # Create test file + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='This is a test file.\nThis file is for testing purposes.', + path=test_file, + ) + obs = runtime.run_action(action) + + # Test view command + action = FileEditAction( + command='view', + path=config.workspace_mount_path_in_sandbox, + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert ( + obs.content + == f"""Here's the files and directories up to 2 levels deep in {config.workspace_mount_path_in_sandbox}, excluding hidden items: +{config.workspace_mount_path_in_sandbox}/ +{config.workspace_mount_path_in_sandbox}/test.txt""" + ) + + finally: + _close_test_runtime(runtime) + + +def test_create_file(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + new_file = os.path.join(config.workspace_mount_path_in_sandbox, 'new_file.txt') + action = FileEditAction( + command='create', + path=new_file, + file_text='New file content', + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'File created successfully' in obs.content + + # Verify file content + action = FileEditAction( + command='view', + path=new_file, + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'New file content' in obs.content + + finally: + _close_test_runtime(runtime) + + +def test_create_file_with_empty_content(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + new_file = os.path.join(config.workspace_mount_path_in_sandbox, 'new_file.txt') + action = FileEditAction( + command='create', + path=new_file, + file_text='', + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'File created successfully' in obs.content + + # Verify file content + action = FileEditAction( + command='view', + path=new_file, + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert '1\t' in obs.content + + finally: + _close_test_runtime(runtime) + + +def test_create_with_none_file_text(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + new_file = os.path.join( + config.workspace_mount_path_in_sandbox, 'none_content.txt' + ) + action = FileEditAction( + command='create', + path=new_file, + file_text=None, + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert ( + obs.content + == 'ERROR:\nParameter `file_text` is required for command: create.' + ) + finally: + _close_test_runtime(runtime) + + +def test_str_replace(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + # Create test file + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='This is a test file.\nThis file is for testing purposes.', + path=test_file, + ) + runtime.run_action(action) + + # Test str_replace command + action = FileEditAction( + command='str_replace', + path=test_file, + old_str='test file', + new_str='sample file', + ) + obs = runtime.run_action(action) + assert f'The file {test_file} has been edited' in obs.content + + # Verify file content + action = FileEditAction( + command='view', + path=test_file, + ) + obs = runtime.run_action(action) + assert 'This is a sample file.' in obs.content + + finally: + _close_test_runtime(runtime) + + +def test_str_replace_multi_line(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='This is a test file.\nThis file is for testing purposes.', + path=test_file, + ) + runtime.run_action(action) + + # Test str_replace command + action = FileEditAction( + command='str_replace', + path=test_file, + old_str='This is a test file.\nThis file is for testing purposes.', + new_str='This is a sample file.\nThis file is for testing purposes.', + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert f'The file {test_file} has been edited.' in obs.content + assert 'This is a sample file.' in obs.content + assert 'This file is for testing purposes.' in obs.content + + finally: + _close_test_runtime(runtime) + + +def test_str_replace_multi_line_with_tabs(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='def test():\n\tprint("Hello, World!")', + path=test_file, + ) + runtime.run_action(action) + + # Test str_replace command + action = FileEditAction( + command='str_replace', + path=test_file, + old_str='def test():\n\tprint("Hello, World!")', + new_str='def test():\n\tprint("Hello, Universe!")', + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert ( + obs.content + == f"""The file {test_file} has been edited. Here's the result of running `cat -n` on a snippet of {test_file}: + 1\tdef test(): + 2\t{'\t'.expandtabs()}print("Hello, Universe!") + 3\t +Review the changes and make sure they are as expected. Edit the file again if necessary.""" + ) + + finally: + _close_test_runtime(runtime) + + +def test_str_replace_error_multiple_occurrences( + temp_dir, runtime_cls, run_as_openhands +): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='This is a test file.\nThis file is for testing purposes.', + path=test_file, + ) + runtime.run_action(action) + + action = FileEditAction( + command='str_replace', path=test_file, old_str='test', new_str='sample' + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'Multiple occurrences of old_str `test`' in obs.content + assert '[1, 2]' in obs.content # Should show both line numbers + finally: + _close_test_runtime(runtime) + + +def test_str_replace_error_multiple_multiline_occurrences( + temp_dir, runtime_cls, run_as_openhands +): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + # Create a file with two identical multi-line blocks + multi_block = """def example(): + print("Hello") + return True""" + content = f"{multi_block}\n\nprint('separator')\n\n{multi_block}" + action = FileWriteAction( + content=content, + path=test_file, + ) + runtime.run_action(action) + + # Test str_replace command + action = FileEditAction( + command='str_replace', + path=test_file, + old_str=multi_block, + new_str='def new():\n print("World")', + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'Multiple occurrences of old_str' in obs.content + assert '[1, 7]' in obs.content # Should show correct starting line numbers + + finally: + _close_test_runtime(runtime) + + +def test_str_replace_nonexistent_string(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='Line 1\nLine 2', + path=test_file, + ) + runtime.run_action(action) + action = FileEditAction( + command='str_replace', + path=test_file, + old_str='Non-existent Line', + new_str='New Line', + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'No replacement was performed' in obs.content + assert ( + f'old_str `Non-existent Line` did not appear verbatim in {test_file}' + in obs.content + ) + finally: + _close_test_runtime(runtime) + + +def test_str_replace_with_empty_new_str(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='Line 1\nLine to remove\nLine 3', + path=test_file, + ) + runtime.run_action(action) + action = FileEditAction( + command='str_replace', + path=test_file, + old_str='Line to remove\n', + new_str='', + ) + obs = runtime.run_action(action) + assert 'Line to remove' not in obs.content + assert 'Line 1' in obs.content + assert 'Line 3' in obs.content + + finally: + _close_test_runtime(runtime) + + +def test_str_replace_with_empty_old_str(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='Line 1\nLine 2\nLine 3', + path=test_file, + ) + runtime.run_action(action) + action = FileEditAction( + command='str_replace', + path=test_file, + old_str='', + new_str='New string', + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert ( + 'No replacement was performed. Multiple occurrences of old_str `` in lines [1, 2, 3, 4]. Please ensure it is unique.' + in obs.content + ) + finally: + _close_test_runtime(runtime) + + +def test_str_replace_with_none_old_str(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='Line 1\nLine 2\nLine 3', + path=test_file, + ) + runtime.run_action(action) + + action = FileEditAction( + command='str_replace', + path=test_file, + old_str=None, + new_str='new content', + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'old_str' in obs.content + finally: + _close_test_runtime(runtime) + + +def test_insert(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + # Create test file + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='Line 1\nLine 2', + path=test_file, + ) + runtime.run_action(action) + + # Test insert command + action = FileEditAction( + command='insert', + path=test_file, + insert_line=1, + new_str='Inserted line', + ) + obs = runtime.run_action(action) + assert f'The file {test_file} has been edited' in obs.content + + # Verify file content + action = FileEditAction( + command='view', + path=test_file, + ) + obs = runtime.run_action(action) + assert 'Line 1' in obs.content + assert 'Inserted line' in obs.content + assert 'Line 2' in obs.content + + finally: + _close_test_runtime(runtime) + + +def test_insert_invalid_line(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='Line 1\nLine 2', + path=test_file, + ) + runtime.run_action(action) + action = FileEditAction( + command='insert', + path=test_file, + insert_line=10, + new_str='Invalid Insert', + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'Invalid `insert_line` parameter' in obs.content + assert 'It should be within the range of lines of the file' in obs.content + finally: + _close_test_runtime(runtime) + + +def test_insert_with_empty_string(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='Line 1\nLine 2', + path=test_file, + ) + runtime.run_action(action) + action = FileEditAction( + command='insert', + path=test_file, + insert_line=1, + new_str='', + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert '1\tLine 1' in obs.content + assert '2\t\n' in obs.content + assert '3\tLine 2' in obs.content + finally: + _close_test_runtime(runtime) + + +def test_insert_with_none_new_str(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='Line 1\nLine 2', + path=test_file, + ) + runtime.run_action(action) + + action = FileEditAction( + command='insert', + path=test_file, + insert_line=1, + new_str=None, + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'ERROR' in obs.content + assert 'Parameter `new_str` is required for command: insert' in obs.content + finally: + _close_test_runtime(runtime) + + +def test_undo_edit(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + # Create test file + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='This is a test file.', + path=test_file, + ) + runtime.run_action(action) + + # Make an edit + action = FileEditAction( + command='str_replace', + path=test_file, + old_str='test', + new_str='sample', + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'This is a sample file.' in obs.content + + # Undo the edit + action = FileEditAction( + command='undo_edit', + path=test_file, + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'Last edit to' in obs.content + assert 'This is a test file.' in obs.content + + # Verify file content + action = FileEditAction( + command='view', + path=test_file, + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'This is a test file.' in obs.content + + finally: + _close_test_runtime(runtime) + + +def test_validate_path_invalid(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + invalid_file = os.path.join( + config.workspace_mount_path_in_sandbox, 'nonexistent.txt' + ) + action = FileEditAction( + command='view', + path=invalid_file, + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'Invalid `path` parameter' in obs.content + assert f'The path {invalid_file} does not exist' in obs.content + finally: + _close_test_runtime(runtime) + + +def test_create_existing_file_error(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='Line 1\nLine 2', + path=test_file, + ) + runtime.run_action(action) + action = FileEditAction( + command='create', + path=test_file, + file_text='New content', + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'File already exists' in obs.content + finally: + _close_test_runtime(runtime) + + +def test_str_replace_missing_old_str(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='Line 1\nLine 2', + path=test_file, + ) + runtime.run_action(action) + action = FileEditAction( + command='str_replace', + path=test_file, + old_str='', + new_str='sample', + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert ( + 'No replacement was performed. Multiple occurrences of old_str ``' + in obs.content + ) + finally: + _close_test_runtime(runtime) + + +def test_str_replace_new_str_and_old_str_same(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='Line 1\nLine 2', + path=test_file, + ) + runtime.run_action(action) + action = FileEditAction( + command='str_replace', + path=test_file, + old_str='test file', + new_str='test file', + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert ( + 'No replacement was performed. `new_str` and `old_str` must be different.' + in obs.content + ) + finally: + _close_test_runtime(runtime) + + +def test_insert_missing_line_param(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + test_file = os.path.join(config.workspace_mount_path_in_sandbox, 'test.txt') + action = FileWriteAction( + content='Line 1\nLine 2', + path=test_file, + ) + runtime.run_action(action) + action = FileEditAction( + command='insert', + path=test_file, + new_str='Missing insert line', + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'Parameter `insert_line` is required for command: insert' in obs.content + finally: + _close_test_runtime(runtime) + + +def test_undo_edit_no_history_error(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + empty_file = os.path.join(config.workspace_mount_path_in_sandbox, 'empty.txt') + action = FileWriteAction( + content='', + path=empty_file, + ) + runtime.run_action(action) + + action = FileEditAction( + command='undo_edit', + path=empty_file, + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert 'No edit history found for' in obs.content + finally: + _close_test_runtime(runtime) + + +def test_view_large_file_with_truncation(temp_dir, runtime_cls, run_as_openhands): + runtime, config = _load_runtime(temp_dir, runtime_cls, run_as_openhands) + try: + # Create a large file to trigger truncation + large_file = os.path.join( + config.workspace_mount_path_in_sandbox, 'large_test.txt' + ) + large_content = 'Line 1\n' * 16000 # 16000 lines should trigger truncation + action = FileWriteAction( + content=large_content, + path=large_file, + ) + runtime.run_action(action) + + action = FileEditAction( + command='view', + path=large_file, + ) + obs = runtime.run_action(action) + logger.info(obs, extra={'msg_type': 'OBSERVATION'}) + assert ( + 'Due to the max output limit, only part of this file has been shown to you.' + in obs.content + ) + finally: + _close_test_runtime(runtime) diff --git a/tests/runtime/test_ipython.py b/tests/runtime/test_ipython.py index ea0db4ac88b5..c9fe1bf3e729 100644 --- a/tests/runtime/test_ipython.py +++ b/tests/runtime/test_ipython.py @@ -10,12 +10,10 @@ from openhands.core.logger import openhands_logger as logger from openhands.events.action import ( CmdRunAction, - FileEditAction, FileReadAction, FileWriteAction, IPythonRunCellAction, ) -from openhands.events.event import FileEditSource from openhands.events.observation import ( CmdOutputObservation, ErrorObservation, @@ -310,65 +308,3 @@ def test_ipython_file_editor_permissions_as_openhands(temp_dir, runtime_cls): assert obs.exit_code == 0 _close_test_runtime(runtime) - - -def test_file_read_and_edit_via_oh_aci(runtime_cls, run_as_openhands): - runtime, config = _load_runtime(None, runtime_cls, run_as_openhands) - sandbox_dir = '/workspace' - - actions = [ - { - 'command': 'create', - 'test_code': f"print(file_editor(command='create', path='{sandbox_dir}/test.txt', file_text='Line 1\\nLine 2\\nLine 3'))", - 'action_cls': FileEditAction, - 'assertions': ['File created successfully'], - }, - { - 'command': 'view', - 'test_code': f"print(file_editor(command='view', path='{sandbox_dir}/test.txt'))", - 'action_cls': FileReadAction, - 'assertions': ['Line 1', 'Line 2', 'Line 3'], - }, - { - 'command': 'str_replace', - 'test_code': f"print(file_editor(command='str_replace', path='{sandbox_dir}/test.txt', old_str='Line 2', new_str='New Line 2'))", - 'action_cls': FileEditAction, - 'assertions': ['New Line 2'], - }, - { - 'command': 'undo_edit', - 'test_code': f"print(file_editor(command='undo_edit', path='{sandbox_dir}/test.txt'))", - 'action_cls': FileEditAction, - 'assertions': ['Last edit to', 'undone successfully'], - }, - { - 'command': 'insert', - 'test_code': f"print(file_editor(command='insert', path='{sandbox_dir}/test.txt', insert_line=2, new_str='Line 4'))", - 'action_cls': FileEditAction, - 'assertions': ['Line 4'], - }, - ] - - for action_info in actions: - action_cls = action_info['action_cls'] - - kwargs = { - 'path': f'{sandbox_dir}/test.txt', - 'translated_ipython_code': action_info['test_code'], - 'impl_source': FileEditSource.OH_ACI, - } - if action_info['action_cls'] == FileEditAction: - kwargs['content'] = '' # dummy value required for FileEditAction - - action = action_cls(**kwargs) - - logger.info(action, extra={'msg_type': 'ACTION'}) - obs = runtime.run_action(action) - logger.info(obs, extra={'msg_type': 'OBSERVATION'}) - for assertion in action_info['assertions']: - if action_cls == FileReadAction: - assert assertion in obs.content - else: - assert assertion in str(obs) - - _close_test_runtime(runtime) diff --git a/tests/runtime/test_edit.py b/tests/runtime/test_llm_based_edit.py similarity index 100% rename from tests/runtime/test_edit.py rename to tests/runtime/test_llm_based_edit.py diff --git a/tests/unit/test_action_serialization.py b/tests/unit/test_action_serialization.py index 32b29e44b231..84eb03148454 100644 --- a/tests/unit/test_action_serialization.py +++ b/tests/unit/test_action_serialization.py @@ -5,11 +5,13 @@ BrowseInteractiveAction, BrowseURLAction, CmdRunAction, + FileEditAction, FileReadAction, FileWriteAction, MessageAction, ) from openhands.events.action.action import ActionConfirmationStatus +from openhands.events.action.files import FileEditSource, FileReadSource from openhands.events.serialization import ( event_from_dict, event_to_dict, @@ -135,7 +137,7 @@ def test_file_read_action_serialization_deserialization(): 'end': -1, 'thought': 'None', 'impl_source': 'default', - 'translated_ipython_code': '', + 'view_range': None, }, } serialization_deserialization(original_action_dict, FileReadAction) @@ -155,7 +157,47 @@ def test_file_write_action_serialization_deserialization(): serialization_deserialization(original_action_dict, FileWriteAction) -def test_legacy_serialization(): +def test_file_edit_action_aci_serialization_deserialization(): + original_action_dict = { + 'action': 'edit', + 'args': { + 'path': '/path/to/file.txt', + 'command': 'str_replace', + 'file_text': None, + 'old_str': 'old text', + 'new_str': 'new text', + 'insert_line': None, + 'content': '', + 'start': 1, + 'end': -1, + 'thought': 'Replacing text', + 'impl_source': 'oh_aci', + }, + } + serialization_deserialization(original_action_dict, FileEditAction) + + +def test_file_edit_action_llm_serialization_deserialization(): + original_action_dict = { + 'action': 'edit', + 'args': { + 'path': '/path/to/file.txt', + 'command': None, + 'file_text': None, + 'old_str': None, + 'new_str': None, + 'insert_line': None, + 'content': 'Updated content', + 'start': 1, + 'end': 10, + 'thought': 'Updating file content', + 'impl_source': 'llm_based_edit', + }, + } + serialization_deserialization(original_action_dict, FileEditAction) + + +def test_cmd_run_action_legacy_serialization(): original_action_dict = { 'action': 'run', 'args': { @@ -183,3 +225,167 @@ def test_legacy_serialization(): assert event_dict['args']['command'] == 'echo "Hello world"' assert event_dict['args']['thought'] == '' assert event_dict['args']['is_input'] is False + + +def test_file_llm_based_edit_action_legacy_serialization(): + original_action_dict = { + 'action': 'edit', + 'args': { + 'path': '/path/to/file.txt', + 'content': 'dummy content', + 'start': 1, + 'end': -1, + 'thought': 'Replacing text', + 'impl_source': 'oh_aci', + 'translated_ipython_code': None, + }, + } + event = event_from_dict(original_action_dict) + assert isinstance(event, Action) + assert isinstance(event, FileEditAction) + + # Common arguments + assert event.path == '/path/to/file.txt' + assert event.thought == 'Replacing text' + assert event.impl_source == FileEditSource.OH_ACI + assert not hasattr(event, 'translated_ipython_code') + + # OH_ACI arguments + assert event.command == '' + assert event.file_text is None + assert event.old_str is None + assert event.new_str is None + assert event.insert_line is None + + # LLM-based editing arguments + assert event.content == 'dummy content' + assert event.start == 1 + assert event.end == -1 + + event_dict = event_to_dict(event) + assert 'translated_ipython_code' not in event_dict['args'] + + # Common arguments + assert event_dict['args']['path'] == '/path/to/file.txt' + assert event_dict['args']['impl_source'] == 'oh_aci' + assert event_dict['args']['thought'] == 'Replacing text' + + # OH_ACI arguments + assert event_dict['args']['command'] == '' + assert event_dict['args']['file_text'] is None + assert event_dict['args']['old_str'] is None + assert event_dict['args']['new_str'] is None + assert event_dict['args']['insert_line'] is None + + # LLM-based editing arguments + assert event_dict['args']['content'] == 'dummy content' + assert event_dict['args']['start'] == 1 + assert event_dict['args']['end'] == -1 + + +def test_file_ohaci_edit_action_legacy_serialization(): + original_action_dict = { + 'action': 'edit', + 'args': { + 'path': '/workspace/game_2048.py', + 'content': '', + 'start': 1, + 'end': -1, + 'thought': "I'll help you create a simple 2048 game in Python. I'll use the str_replace_editor to create the file.", + 'impl_source': 'oh_aci', + 'translated_ipython_code': "print(file_editor(**{'command': 'create', 'path': '/workspace/game_2048.py', 'file_text': 'New file content'}))", + }, + } + event = event_from_dict(original_action_dict) + assert isinstance(event, Action) + assert isinstance(event, FileEditAction) + + # Common arguments + assert event.path == '/workspace/game_2048.py' + assert ( + event.thought + == "I'll help you create a simple 2048 game in Python. I'll use the str_replace_editor to create the file." + ) + assert event.impl_source == FileEditSource.OH_ACI + assert not hasattr(event, 'translated_ipython_code') + + # OH_ACI arguments + assert event.command == 'create' + assert event.file_text == 'New file content' + assert event.old_str is None + assert event.new_str is None + assert event.insert_line is None + + # LLM-based editing arguments + assert event.content == '' + assert event.start == 1 + assert event.end == -1 + + event_dict = event_to_dict(event) + assert 'translated_ipython_code' not in event_dict['args'] + + # Common arguments + assert event_dict['args']['path'] == '/workspace/game_2048.py' + assert event_dict['args']['impl_source'] == 'oh_aci' + assert ( + event_dict['args']['thought'] + == "I'll help you create a simple 2048 game in Python. I'll use the str_replace_editor to create the file." + ) + + # OH_ACI arguments + assert event_dict['args']['command'] == 'create' + assert event_dict['args']['file_text'] == 'New file content' + assert event_dict['args']['old_str'] is None + assert event_dict['args']['new_str'] is None + assert event_dict['args']['insert_line'] is None + + # LLM-based editing arguments + assert event_dict['args']['content'] == '' + assert event_dict['args']['start'] == 1 + assert event_dict['args']['end'] == -1 + + +def test_file_read_action_legacy_serialization(): + original_action_dict = { + 'action': 'read', + 'args': { + 'path': '/workspace/test.txt', + 'start': 0, + 'end': -1, + 'thought': 'Reading the file contents', + 'impl_source': 'oh_aci', + 'translated_ipython_code': "print(file_editor(**{'command': 'view', 'path': '/workspace/test.txt'}))", + }, + } + + event = event_from_dict(original_action_dict) + assert isinstance(event, Action) + assert isinstance(event, FileReadAction) + + # Common arguments + assert event.path == '/workspace/test.txt' + assert event.thought == 'Reading the file contents' + assert event.impl_source == FileReadSource.OH_ACI + assert not hasattr(event, 'translated_ipython_code') + assert not hasattr( + event, 'command' + ) # FileReadAction should not have command attribute + + # Read-specific arguments + assert event.start == 0 + assert event.end == -1 + + event_dict = event_to_dict(event) + assert 'translated_ipython_code' not in event_dict['args'] + assert ( + 'command' not in event_dict['args'] + ) # command should not be in serialized args + + # Common arguments in serialized form + assert event_dict['args']['path'] == '/workspace/test.txt' + assert event_dict['args']['impl_source'] == 'oh_aci' + assert event_dict['args']['thought'] == 'Reading the file contents' + + # Read-specific arguments in serialized form + assert event_dict['args']['start'] == 0 + assert event_dict['args']['end'] == -1 diff --git a/tests/unit/test_observation_serialization.py b/tests/unit/test_observation_serialization.py index 626ea2c14774..64888bbb7e2d 100644 --- a/tests/unit/test_observation_serialization.py +++ b/tests/unit/test_observation_serialization.py @@ -1,6 +1,8 @@ +from openhands.events.action.files import FileEditSource from openhands.events.observation import ( CmdOutputMetadata, CmdOutputObservation, + FileEditObservation, Observation, ) from openhands.events.serialization import ( @@ -146,3 +148,88 @@ def test_legacy_serialization(): assert event_dict['extras']['metadata']['pid'] == 3 assert event_dict['extras']['command'] == 'ls -l' assert event_dict['extras']['hidden'] is False + + +def test_file_edit_observation_serialization(): + original_observation_dict = { + 'observation': 'edit', + 'extras': { + '_diff_cache': None, + 'impl_source': FileEditSource.LLM_BASED_EDIT, + 'new_content': None, + 'old_content': None, + 'path': '', + 'prev_exist': False, + }, + 'message': 'I edited the file .', + 'content': '[Existing file /path/to/file.txt is edited with 1 changes.]', + } + serialization_deserialization(original_observation_dict, FileEditObservation) + + +def test_file_edit_observation_new_file_serialization(): + original_observation_dict = { + 'observation': 'edit', + 'content': '[New file /path/to/newfile.txt is created with the provided content.]', + 'extras': { + '_diff_cache': None, + 'impl_source': FileEditSource.LLM_BASED_EDIT, + 'new_content': None, + 'old_content': None, + 'path': '', + 'prev_exist': False, + }, + 'message': 'I edited the file .', + } + + serialization_deserialization(original_observation_dict, FileEditObservation) + + +def test_file_edit_observation_oh_aci_serialization(): + original_observation_dict = { + 'observation': 'edit', + 'content': 'The file /path/to/file.txt is edited with the provided content.', + 'extras': { + '_diff_cache': None, + 'impl_source': FileEditSource.LLM_BASED_EDIT, + 'new_content': None, + 'old_content': None, + 'path': '', + 'prev_exist': False, + }, + 'message': 'I edited the file .', + } + serialization_deserialization(original_observation_dict, FileEditObservation) + + +def test_file_edit_observation_legacy_serialization(): + original_observation_dict = { + 'observation': 'edit', + 'content': 'content', + 'extras': { + 'path': '/workspace/game_2048.py', + 'prev_exist': False, + 'old_content': None, + 'new_content': 'new content', + 'impl_source': 'oh_aci', + 'formatted_output_and_error': 'File created successfully at: /workspace/game_2048.py', + }, + } + + event = event_from_dict(original_observation_dict) + assert isinstance(event, Observation) + assert isinstance(event, FileEditObservation) + assert event.impl_source == FileEditSource.OH_ACI + assert event.path == '/workspace/game_2048.py' + assert event.prev_exist is False + assert event.old_content is None + assert event.new_content == 'new content' + assert not hasattr(event, 'formatted_output_and_error') + + event_dict = event_to_dict(event) + assert event_dict['extras']['impl_source'] == 'oh_aci' + assert event_dict['extras']['path'] == '/workspace/game_2048.py' + assert event_dict['extras']['prev_exist'] is False + assert event_dict['extras']['old_content'] is None + assert event_dict['extras']['new_content'] == 'new content' + assert 'formatted_output_and_error' not in event_dict['extras'] From 1afe7f10585d299994972c9d424907cba021b66b Mon Sep 17 00:00:00 2001 From: Robert Brennan Date: Tue, 11 Feb 2025 12:43:46 -0500 Subject: [PATCH 3/9] Fix debug in remote runtime (#6688) --- openhands/runtime/impl/remote/remote_runtime.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openhands/runtime/impl/remote/remote_runtime.py b/openhands/runtime/impl/remote/remote_runtime.py index f33acedbf4ad..c54bb69dbdde 100644 --- a/openhands/runtime/impl/remote/remote_runtime.py +++ b/openhands/runtime/impl/remote/remote_runtime.py @@ -215,7 +215,7 @@ def _start_runtime(self): environment = { 'DEBUG': 'true' if self.config.debug or os.environ.get('DEBUG', 'false').lower() == 'true' - else {}, + else '', } environment.update(self.config.sandbox.runtime_startup_env_vars) start_request = { From 425ccc9b1fc705a8bcfc89eee689ac7041cb79b7 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 11 Feb 2025 23:53:51 +0400 Subject: [PATCH 4/9] chore(deps-dev): bump @tanstack/eslint-plugin-query from 5.66.0 to 5.66.1 in /frontend in the eslint group (#6682) Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- frontend/package-lock.json | 8 ++++---- frontend/package.json | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/frontend/package-lock.json b/frontend/package-lock.json index 974dfc18d044..43278fa1cea1 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -53,7 +53,7 @@ "@playwright/test": "^1.50.1", "@react-router/dev": "^7.1.5", "@tailwindcss/typography": "^0.5.16", - "@tanstack/eslint-plugin-query": "^5.66.0", + "@tanstack/eslint-plugin-query": "^5.66.1", "@testing-library/dom": "^10.4.0", "@testing-library/jest-dom": "^6.6.1", "@testing-library/react": "^16.2.0", @@ -6126,9 +6126,9 @@ } }, "node_modules/@tanstack/eslint-plugin-query": { - "version": "5.66.0", - "resolved": "https://registry.npmjs.org/@tanstack/eslint-plugin-query/-/eslint-plugin-query-5.66.0.tgz", - "integrity": "sha512-CzZhBxicLDuuSJbkZ4nPcuBqWnhLu72Zt9p/7qLQ93BepVnZJV6ZDlBLBuN5eg7YRACwECPLsntnwo1zuhgseQ==", + "version": "5.66.1", + "resolved": "https://registry.npmjs.org/@tanstack/eslint-plugin-query/-/eslint-plugin-query-5.66.1.tgz", + "integrity": "sha512-pYMVTGgJ7yPk9Rm6UWEmbY6TX0EmMmxJqYkthgeDCwEznToy2m+W928nUODFirtZBZlhBsqHy33LO0kyTlgf0w==", "dev": true, "license": "MIT", "dependencies": { diff --git a/frontend/package.json b/frontend/package.json index 8634bc195d8b..d776fc5dab25 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -80,7 +80,7 @@ "@playwright/test": "^1.50.1", "@react-router/dev": "^7.1.5", "@tailwindcss/typography": "^0.5.16", - "@tanstack/eslint-plugin-query": "^5.66.0", + "@tanstack/eslint-plugin-query": "^5.66.1", "@testing-library/dom": "^10.4.0", "@testing-library/jest-dom": "^6.6.1", "@testing-library/react": "^16.2.0", From a371562d942be0e517d564428a20e223a556b199 Mon Sep 17 00:00:00 2001 From: Xingyao Wang Date: Tue, 11 Feb 2025 17:30:40 -0500 Subject: [PATCH 5/9] refactor: do not add DEBUG env var when it is not set (#6690) --- openhands/runtime/impl/remote/remote_runtime.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/openhands/runtime/impl/remote/remote_runtime.py b/openhands/runtime/impl/remote/remote_runtime.py index c54bb69dbdde..068461cc61fb 100644 --- a/openhands/runtime/impl/remote/remote_runtime.py +++ b/openhands/runtime/impl/remote/remote_runtime.py @@ -212,11 +212,9 @@ def _start_runtime(self): plugins=self.plugins, app_config=self.config, ) - environment = { - 'DEBUG': 'true' - if self.config.debug or os.environ.get('DEBUG', 'false').lower() == 'true' - else '', - } + environment = {} + if self.config.debug or os.environ.get('DEBUG', 'false').lower() == 'true': + environment['DEBUG'] = 'true' environment.update(self.config.sandbox.runtime_startup_env_vars) start_request = { 'image': self.container_image, From ff25e794ef19be1d8a642110153107c3c65c034c Mon Sep 17 00:00:00 2001 From: "sp.wack" <83104063+amanape@users.noreply.github.com> Date: Wed, 12 Feb 2025 17:57:13 +0400 Subject: [PATCH 6/9] Revert "Only show start project button in conversations" (#6698) --- .../src/components/shared/buttons/exit-project-button.tsx | 6 ------ 1 file changed, 6 deletions(-) diff --git a/frontend/src/components/shared/buttons/exit-project-button.tsx b/frontend/src/components/shared/buttons/exit-project-button.tsx index 8e40bf5efac3..5a6db074f616 100644 --- a/frontend/src/components/shared/buttons/exit-project-button.tsx +++ b/frontend/src/components/shared/buttons/exit-project-button.tsx @@ -1,5 +1,4 @@ import { useTranslation } from "react-i18next"; -import { useLocation } from "react-router"; import { I18nKey } from "#/i18n/declaration"; import NewProjectIcon from "#/icons/new-project.svg?react"; import { TooltipButton } from "./tooltip-button"; @@ -10,12 +9,7 @@ interface ExitProjectButtonProps { export function ExitProjectButton({ onClick }: ExitProjectButtonProps) { const { t } = useTranslation(); - const location = useLocation(); const startNewProject = t(I18nKey.PROJECT$START_NEW); - - // Only show the button in the conversations page - if (!location.pathname.startsWith("/conversations")) return null; - return ( Date: Wed, 12 Feb 2025 09:18:34 -0500 Subject: [PATCH 7/9] using all available system memory when RUNTIME_MAX_MEMORY_GB is not set (#6691) --- openhands/runtime/action_execution_server.py | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/openhands/runtime/action_execution_server.py b/openhands/runtime/action_execution_server.py index ccf267bb2ca6..536a6a6ed82d 100644 --- a/openhands/runtime/action_execution_server.py +++ b/openhands/runtime/action_execution_server.py @@ -19,7 +19,6 @@ from pathlib import Path from zipfile import ZipFile -import psutil from fastapi import Depends, FastAPI, HTTPException, Request, UploadFile from fastapi.exceptions import RequestValidationError from fastapi.responses import JSONResponse, StreamingResponse @@ -163,21 +162,14 @@ def __init__( self.last_execution_time = self.start_time self._initialized = False + self.max_memory_gb: int | None = None if _override_max_memory_gb := os.environ.get('RUNTIME_MAX_MEMORY_GB', None): self.max_memory_gb = int(_override_max_memory_gb) logger.info( f'Setting max memory to {self.max_memory_gb}GB (according to the RUNTIME_MAX_MEMORY_GB environment variable)' ) else: - # Get available system memory - total_memory_gb = psutil.virtual_memory().total / ( - 1024 * 1024 * 1024 - ) # Convert to GB - self.max_memory_gb = int(max(0.5, total_memory_gb - 1.0)) - # Reserve 1GB as head room, minimum of 0.5GB - logger.info( - f'Total memory: {total_memory_gb}GB, setting limit to {self.max_memory_gb}GB (reserved 1GB for action execution server, minimum 0.5GB)' - ) + logger.info('No max memory limit set, using all available system memory') @property def initial_cwd(self): @@ -191,7 +183,7 @@ async def ainit(self): no_change_timeout_seconds=int( os.environ.get('NO_CHANGE_TIMEOUT_SECONDS', 30) ), - max_memory_mb=self.max_memory_gb * 1024, + max_memory_mb=self.max_memory_gb * 1024 if self.max_memory_gb else None, ) self.bash_session.initialize() From 7e359eda4af6c66d3b08efd1db0d74ff6bc7ccd4 Mon Sep 17 00:00:00 2001 From: tofarr Date: Wed, 12 Feb 2025 15:28:10 +0000 Subject: [PATCH 8/9] Fix log formatting error (#6699) --- openhands/core/logger.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/openhands/core/logger.py b/openhands/core/logger.py index 7b7fd89a97a5..701a5bb0c39a 100644 --- a/openhands/core/logger.py +++ b/openhands/core/logger.py @@ -86,7 +86,8 @@ class NoColorFormatter(logging.Formatter): def format(self, record: logging.LogRecord) -> str: # Create a deep copy of the record to avoid modifying the original - new_record: logging.LogRecord = copy.deepcopy(record) + new_record = _fix_record(record) + # Strip ANSI color codes from the message new_record.msg = strip_ansi(new_record.msg) @@ -130,7 +131,18 @@ def format(self, record): return f'{msg}' else: return record.msg - return super().format(record) + + new_record = _fix_record(record) + return super().format(new_record) + + +def _fix_record(record: logging.LogRecord): + new_record = copy.copy(record) + # The formatter expects non boolean values, and will raise an exception if there is a boolean - so we fix these + if new_record.exc_info is True and not new_record.exc_text: # type: ignore + new_record.exc_info = sys.exc_info() # type: ignore + new_record.stack_info = None # type: ignore + return new_record file_formatter = NoColorFormatter( From ba599c7dd6dd8b0e99af8cec8d1883b480f2f811 Mon Sep 17 00:00:00 2001 From: "sp.wack" <83104063+amanape@users.noreply.github.com> Date: Wed, 12 Feb 2025 22:46:15 +0400 Subject: [PATCH 9/9] chore: Throw a 404 instead of returning defaults if settings does not exist (#6704) --- .../components/shared/modals/settings/settings-modal.tsx | 5 +++-- frontend/src/hooks/query/use-settings.ts | 8 ++++---- openhands/storage/settings/file_settings_store.py | 2 +- 3 files changed, 8 insertions(+), 7 deletions(-) diff --git a/frontend/src/components/shared/modals/settings/settings-modal.tsx b/frontend/src/components/shared/modals/settings/settings-modal.tsx index badd7babffe1..5caa97f325c4 100644 --- a/frontend/src/components/shared/modals/settings/settings-modal.tsx +++ b/frontend/src/components/shared/modals/settings/settings-modal.tsx @@ -5,9 +5,10 @@ import { LoadingSpinner } from "../../loading-spinner"; import { ModalBackdrop } from "../modal-backdrop"; import { SettingsForm } from "./settings-form"; import { Settings } from "#/types/settings"; +import { DEFAULT_SETTINGS } from "#/services/settings"; interface SettingsModalProps { - settings: Settings; + settings?: Settings; onClose: () => void; } @@ -38,7 +39,7 @@ export function SettingsModal({ onClose, settings }: SettingsModalProps) { )} {aiConfigOptions.data && ( { const query = useQuery({ queryKey: ["settings"], queryFn: getSettingsQueryFn, - initialData: DEFAULT_SETTINGS, - staleTime: 0, - retry: false, enabled: config?.APP_MODE !== "saas" || githubTokenIsSet, + // Only retry if the error is not a 404 because we + // would want to show the modal immediately if the + // settings are not found + retry: (_, error) => error.status !== 404, meta: { disableToast: true, }, diff --git a/openhands/storage/settings/file_settings_store.py b/openhands/storage/settings/file_settings_store.py index eaf35554d7ae..d3cc08677078 100644 --- a/openhands/storage/settings/file_settings_store.py +++ b/openhands/storage/settings/file_settings_store.py @@ -23,7 +23,7 @@ async def load(self) -> Settings | None: settings = Settings(**kwargs) return settings except FileNotFoundError: - return Settings.from_config() + return None async def store(self, settings: Settings): json_str = settings.model_dump_json(context={'expose_secrets': True})