From fdc6e12945f01a27dd88d83515cac61743056ba9 Mon Sep 17 00:00:00 2001 From: Erik Peterson Date: Tue, 6 Jun 2023 17:54:02 -0700 Subject: [PATCH] Improve logic and error messages for file reading and writing with Python code (#4567) * Fix issues with file reading and writing with Python code * Change error message, use Workspace.get_path --------- Co-authored-by: Reinier van der Leer --- autogpt/commands/execute_code.py | 23 ++++++++++++++++++----- autogpt/commands/file_operations_utils.py | 4 +++- tests/integration/test_execute_code.py | 17 ++++++++++++----- tests/unit/test_file_operations.py | 6 ++++++ 4 files changed, 39 insertions(+), 11 deletions(-) diff --git a/autogpt/commands/execute_code.py b/autogpt/commands/execute_code.py index 8826e4782..999e40f8c 100644 --- a/autogpt/commands/execute_code.py +++ b/autogpt/commands/execute_code.py @@ -9,6 +9,8 @@ from docker.errors import ImageNotFound from autogpt.commands.command import command from autogpt.config import Config from autogpt.logs import logger +from autogpt.setup import CFG +from autogpt.workspace.workspace import Workspace @command("execute_python_file", "Execute Python File", '"filename": ""') @@ -21,17 +23,28 @@ def execute_python_file(filename: str, config: Config) -> str: Returns: str: The output of the file """ - logger.info(f"Executing file '{filename}'") + logger.info( + f"Executing python file '{filename}' in working directory '{CFG.workspace_path}'" + ) if not filename.endswith(".py"): return "Error: Invalid file type. Only .py files are allowed." - if not os.path.isfile(filename): - return f"Error: File '{filename}' does not exist." + workspace = Workspace(config.workspace_path, config.restrict_to_workspace) + + path = workspace.get_path(filename) + if not path.is_file(): + # Mimic the response that you get from the command line so that it's easier to identify + return ( + f"python: can't open file '{filename}': [Errno 2] No such file or directory" + ) if we_are_running_in_a_docker_container(): result = subprocess.run( - ["python", filename], capture_output=True, encoding="utf8" + ["python", str(path)], + capture_output=True, + encoding="utf8", + cwd=CFG.workspace_path, ) if result.returncode == 0: return result.stdout @@ -63,7 +76,7 @@ def execute_python_file(filename: str, config: Config) -> str: logger.info(status) container = client.containers.run( image_name, - ["python", str(Path(filename).relative_to(config.workspace_path))], + ["python", str(path.relative_to(workspace.root))], volumes={ config.workspace_path: { "bind": "/workspace", diff --git a/autogpt/commands/file_operations_utils.py b/autogpt/commands/file_operations_utils.py index 7f3e418da..b00779688 100644 --- a/autogpt/commands/file_operations_utils.py +++ b/autogpt/commands/file_operations_utils.py @@ -146,7 +146,9 @@ def is_file_binary_fn(file_path: str): def read_textual_file(file_path: str, logger: logs.Logger) -> str: if not os.path.isfile(file_path): - raise FileNotFoundError(f"{file_path} not found!") + raise FileNotFoundError( + f"read_file {file_path} failed: no such file or directory" + ) is_binary = is_file_binary_fn(file_path) file_extension = os.path.splitext(file_path)[1].lower() parser = extension_to_parser.get(file_extension) diff --git a/tests/integration/test_execute_code.py b/tests/integration/test_execute_code.py index fa3cf2591..ce3c1e160 100644 --- a/tests/integration/test_execute_code.py +++ b/tests/integration/test_execute_code.py @@ -1,6 +1,7 @@ import random import string import tempfile +from typing import Callable import pytest from pytest_mock import MockerFixture @@ -10,12 +11,12 @@ from autogpt.config import Config @pytest.fixture -def config_allow_execute(config: Config, mocker: MockerFixture): +def config_allow_execute(config: Config, mocker: MockerFixture) -> Callable: yield mocker.patch.object(config, "execute_local_commands", True) @pytest.fixture -def python_test_file(config: Config, random_string): +def python_test_file(config: Config, random_string) -> Callable: temp_file = tempfile.NamedTemporaryFile(dir=config.workspace_path, suffix=".py") temp_file.write(str.encode(f"print('Hello {random_string}!')")) temp_file.flush() @@ -34,18 +35,24 @@ def test_execute_python_file(python_test_file: str, random_string: str, config): assert result.replace("\r", "") == f"Hello {random_string}!\n" -def test_execute_python_file_invalid(config): +def test_execute_python_file_invalid(config: Config): assert all( s in sut.execute_python_file("not_python", config).lower() for s in ["error:", "invalid", ".py"] ) + + +def test_execute_python_file_not_found(config: Config): assert all( s in sut.execute_python_file("notexist.py", config).lower() - for s in ["error:", "does not exist"] + for s in [ + "python: can't open file 'notexist.py'", + "[errno 2] no such file or directory", + ] ) -def test_execute_shell(config_allow_execute, random_string, config): +def test_execute_shell(config_allow_execute: bool, random_string: str, config: Config): result = sut.execute_shell(f"echo 'Hello {random_string}!'", config) assert f"Hello {random_string}!" in result diff --git a/tests/unit/test_file_operations.py b/tests/unit/test_file_operations.py index 1d0219eb5..3da573751 100644 --- a/tests/unit/test_file_operations.py +++ b/tests/unit/test_file_operations.py @@ -229,6 +229,12 @@ def test_read_file( assert content.replace("\r", "") == file_content +def test_read_file_not_found(config: Config): + filename = "does_not_exist.txt" + content = file_ops.read_file(filename, config) + assert "Error:" in content and filename in content and "no such file" in content + + def test_write_to_file(test_file_path: Path, config): new_content = "This is new content.\n" file_ops.write_to_file(str(test_file_path), new_content, config)