diff --git a/lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py b/lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py index 6ff11ab0c..f507d0f86 100644 --- a/lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py +++ b/lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py @@ -4,7 +4,7 @@ from typing import Any from crewai.tools import BaseTool from pydantic import BaseModel, Field, PrivateAttr -from crewai_tools.file_storage import FileStore, resolve_file_store +from crewai_tools.file_storage import FileStore, FileStoreError, resolve_file_store from crewai_tools.security.safe_path import ( format_error_for_display, format_sandbox_error, @@ -102,29 +102,43 @@ class FileReadTool(BaseTool): encoding (str): Text encoding used to decode the file. **kwargs: Additional keyword arguments passed to BaseTool. """ + # Hand these to pydantic rather than assigning them afterwards, so the + # fields are already populated by the time model_post_init derives + # everything that depends on them. That hook is the only place the + # derivation can live: see its docstring. + if file_path is not None: + kwargs["file_path"] = file_path + if base_dir is not None: + kwargs["base_dir"] = base_dir + kwargs["encoding"] = encoding + + super().__init__(**kwargs) + + def model_post_init(self, context: object) -> None: + """Bind the store, then derive everything that depends on it. + + This has to happen here rather than in ``__init__`` because pydantic + reconstructs a serialized tool through ``model_validate``, which skips + ``__init__`` entirely — ``BaseTool._resolve_tool_dict`` does exactly + that when a crew carrying this tool is rebuilt. Deriving here means a + reconstructed reader binds its store and pins its declared path just + like a freshly constructed one, instead of coming back with no store + and failing on the first read. + """ + super().model_post_init(context) + store = resolve_file_store() + self._store = store # Anchor base_dir once, so the sandbox root cannot move under a later # chdir while the declared file stays pinned to its original location. - if base_dir is not None: - base_dir = store.normalize(base_dir) + if self.base_dir is not None: + self.base_dir = store.normalize(self.base_dir) - display_path = None - if file_path is not None: - display_path = store.display(store.normalize(file_path, base_dir), base_dir) - kwargs["description"] = ( - f"A tool that reads file content. The default file is {display_path}, which is read when 'file_path' is omitted. You can also provide a different 'file_path' parameter to read another file, though reads are confined to the tool's allowed directory and a path that resolves outside it is rejected. Specify 'start_line' and 'line_count' to read specific parts of the file." - ) - - super().__init__(**kwargs) - self.file_path = file_path - self.base_dir = base_dir - self.encoding = encoding - self._store = store - self._declared_realpath = ( - store.normalize(file_path, base_dir) if file_path is not None else None - ) - self._declared_label = display_path + if self.file_path is not None: + self._declared_realpath = store.normalize(self.file_path, self.base_dir) + self._declared_label = store.display(self._declared_realpath, self.base_dir) + self.description = f"A tool that reads file content. The default file is {self._declared_label}, which is read when 'file_path' is omitted. You can also provide a different 'file_path' parameter to read another file, though reads are confined to the tool's allowed directory and a path that resolves outside it is rejected. Specify 'start_line' and 'line_count' to read specific parts of the file." def _resolve_path(self, file_path: str) -> str: """Resolve *file_path* and confirm the tool is allowed to read it. @@ -160,6 +174,27 @@ class FileReadTool(BaseTool): line_count: int | None = None, ) -> str: """Read a file, or a window of its lines, as text.""" + try: + return self._read(file_path, start_line, line_count) + except (FileStoreError, OSError) as e: + # A store can fail for reasons the local filesystem never had: an + # unreachable endpoint, a rejected request. Every other exit from + # this tool is an agent-visible string, and raising here would kill + # the agent's step rather than let it react, so this one is too. + # Path resolution and the store's own path labelling both live + # under here, which is why the message carries no path. + return ( + f"Error: the {self._store.label} store failed. " + f"{format_error_for_display(e)}" + ) + + def _read( + self, + file_path: str | None, + start_line: int | None, + line_count: int | None, + ) -> str: + """Do the read. Wrapped by :meth:`_run`, which reports store failures.""" start_line = start_line or 1 line_count = line_count or None diff --git a/lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py b/lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py index e1e4b79a7..9968ea7d0 100644 --- a/lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py +++ b/lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py @@ -1,7 +1,7 @@ from crewai.tools import BaseTool from pydantic import BaseModel, Field, PrivateAttr -from crewai_tools.file_storage import FileStore, resolve_file_store +from crewai_tools.file_storage import FileStore, FileStoreError, resolve_file_store from crewai_tools.security.safe_path import ( format_error_for_display, format_sandbox_error, @@ -111,6 +111,28 @@ class FileWriterTool(BaseTool): overwrite: str | bool = False, ) -> str: """Write *content* to *filename*, confined to the tool's sandbox.""" + try: + return self._write(filename, content, directory, overwrite) + except (FileStoreError, OSError) as e: + # A store can fail for reasons the local filesystem never had: an + # unreachable endpoint, a size ceiling. Every other exit from this + # tool is an agent-visible string, and raising here would kill the + # agent's step rather than let it react, so this one is too. It + # also covers the calls with no handler of their own — exists() and + # the store's own path labelling — which is why it carries no path. + return ( + f"An error occurred while writing to the file: the " + f"{self._store.label} store failed. {format_error_for_display(e)}" + ) + + def _write( + self, + filename: str, + content: str, + directory: str | None, + overwrite: str | bool, + ) -> str: + """Do the write. Wrapped by :meth:`_run`, which reports store failures.""" directory = directory or "./" try: diff --git a/lib/crewai-tools/tests/file_storage/test_file_store_seam.py b/lib/crewai-tools/tests/file_storage/test_file_store_seam.py index fdda9fba1..e4c93f3b5 100644 --- a/lib/crewai-tools/tests/file_storage/test_file_store_seam.py +++ b/lib/crewai-tools/tests/file_storage/test_file_store_seam.py @@ -14,6 +14,7 @@ import posixpath from crewai_tools import FileReadTool, FileWriterTool from crewai_tools.file_storage import ( FileStore, + FileStoreError, register_file_store_factory, reset_file_store_factory, resolve_file_store, @@ -91,6 +92,58 @@ class MemoryFileStore: self.files[resolved] = content +class FailingFileStore(MemoryFileStore): + """A store where one named operation fails, the way a remote one can. + + The local filesystem cannot produce most of these — an unreachable + endpoint has no local equivalent — so a store that only ever fails the + way `open()` does would not exercise the tools' error paths at all. + """ + + def __init__(self, failing: str, exc: Exception | None = None) -> None: + super().__init__() + self.failing = failing + self.exc = exc or FileStoreError("workspace endpoint unreachable") + + def _maybe_fail(self, name: str) -> None: + if name == self.failing: + raise self.exc + + def normalize(self, path: str, base_dir: str | None = None) -> str: + self._maybe_fail("normalize") + return super().normalize(path, base_dir) + + def resolve(self, path: str, base_dir: str | None = None) -> str: + self._maybe_fail("resolve") + return super().resolve(path, base_dir) + + def resolve_within(self, directory: str, filename: str) -> str: + self._maybe_fail("resolve_within") + return super().resolve_within(directory, filename) + + def display(self, resolved: str, base: str | None = None) -> str: + self._maybe_fail("display") + return super().display(resolved, base) + + def exists(self, resolved: str) -> bool: + self._maybe_fail("exists") + return super().exists(resolved) + + def ensure_parent(self, resolved: str) -> None: + self._maybe_fail("ensure_parent") + super().ensure_parent(resolved) + + def open_text(self, resolved: str, encoding: str): + self._maybe_fail("open_text") + return super().open_text(resolved, encoding) + + def write_text( + self, resolved: str, content: str, encoding: str, *, overwrite: bool + ) -> None: + self._maybe_fail("write_text") + super().write_text(resolved, content, encoding, overwrite=overwrite) + + @pytest.fixture def store(): memory = MemoryFileStore() @@ -99,6 +152,21 @@ def store(): reset_file_store_factory() +@pytest.fixture +def failing_store(): + """Register a store whose *named* operation fails; the test picks which.""" + created: list[FailingFileStore] = [] + + def _register(failing: str, exc: Exception | None = None) -> FailingFileStore: + broken = FailingFileStore(failing, exc) + register_file_store_factory(lambda: broken) + created.append(broken) + return broken + + yield _register + reset_file_store_factory() + + def test_default_store_is_local(): assert isinstance(resolve_file_store(), LocalFileStore) @@ -277,3 +345,178 @@ def test_os_error_message_does_not_leak_an_absolute_path(tmp_path, monkeypatch): assert "Error" in result assert str(tmp_path) not in result + + +# --- reconstruction through pydantic ----------------------------------------- +# +# BaseTool rebuilds a serialized tool with `model_validate` (see +# `_resolve_tool_dict`), which skips `__init__` entirely. Anything derived only +# in `__init__` comes back missing, and for the store that means every read +# raising AttributeError on a None. + + +def test_reconstructed_reader_reads_through_the_store(store, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + store.files["/ws/notes.txt"] = "reconstructed\n" + + tool = FileReadTool.model_validate({"file_path": "notes.txt"}) + + assert tool._store is store + assert tool._run() == "reconstructed\n" + + +def test_reconstructed_writer_writes_through_the_store(store, tmp_path, monkeypatch): + monkeypatch.chdir(tmp_path) + + tool = FileWriterTool.model_validate({"base_dir": "scoped"}) + + assert tool._store is store + assert tool.base_dir == "/ws/scoped" + assert "successfully written" in tool._run( + filename="a.txt", content="x", overwrite=True + ) + assert store.files == {"/ws/scoped/a.txt": "x"} + + +def test_reconstructed_reader_keeps_its_declared_file(store, tmp_path, monkeypatch): + """The declared path is pinned in the hook, so a rebuild pins it too.""" + monkeypatch.chdir(tmp_path) + store.files["/ws/sub/pinned.txt"] = "kept\n" + + fresh = FileReadTool(file_path="sub/pinned.txt") + rebuilt = FileReadTool.model_validate({"file_path": "sub/pinned.txt"}) + + assert rebuilt._declared_realpath == fresh._declared_realpath == "/ws/sub/pinned.txt" + assert rebuilt._declared_label == fresh._declared_label + assert rebuilt.description == fresh.description + # Addressable by the label the description shows, same as a fresh tool. + assert rebuilt._run(file_path=rebuilt._declared_label) == "kept\n" + + +def test_reader_round_trips_through_model_dump(store, tmp_path, monkeypatch): + """The full serialize/deserialize cycle, not just a hand-built dict.""" + monkeypatch.chdir(tmp_path) + store.files["/ws/notes.txt"] = "dumped\n" + + original = FileReadTool(file_path="notes.txt") + rebuilt = FileReadTool.model_validate(original.model_dump()) + + assert rebuilt._run() == "dumped\n" + + +# --- a store that fails ------------------------------------------------------ +# +# A store may fail where the local filesystem never could. Every exit from +# these tools is an agent-visible string, so a store failure has to become one +# too rather than raising into the agent's step. + + +@pytest.mark.parametrize( + "failing", ["resolve", "resolve_within", "display", "exists", "ensure_parent"] +) +def test_writer_reports_a_store_failure_instead_of_raising( + failing, failing_store, tmp_path, monkeypatch +): + monkeypatch.chdir(tmp_path) + failing_store(failing) + + result = FileWriterTool()._run(filename="a.txt", content="x", overwrite=True) + + assert "error occurred while writing" in result + assert "memory" in result + assert "workspace endpoint unreachable" in result + + +def test_writer_reports_a_failing_write(failing_store, tmp_path, monkeypatch): + """write_text already had a handler; it must keep its own wording.""" + monkeypatch.chdir(tmp_path) + failing_store("write_text") + + result = FileWriterTool()._run(filename="a.txt", content="x", overwrite=True) + + assert "An error occurred while writing to the file" in result + assert "workspace endpoint unreachable" in result + + +@pytest.mark.parametrize("failing", ["resolve", "display"]) +def test_reader_reports_a_store_failure_instead_of_raising( + failing, failing_store, tmp_path, monkeypatch +): + monkeypatch.chdir(tmp_path) + failing_store(failing) + + result = FileReadTool()._run(file_path="notes.txt") + + assert "Error" in result + assert "memory" in result + assert "workspace endpoint unreachable" in result + + +def test_reader_reports_a_failing_open(failing_store, tmp_path, monkeypatch): + """open_text already had a handler; it must keep its own wording.""" + monkeypatch.chdir(tmp_path) + failing_store("open_text") + + result = FileReadTool()._run(file_path="notes.txt") + + assert "Failed to read file" in result + assert "workspace endpoint unreachable" in result + + +def _run_either(tool_cls, tool): + """Drive whichever tool this is with its minimal arguments.""" + if tool_cls is FileReadTool: + return tool._run(file_path="notes.txt") + return tool._run(filename="a.txt", content="x", overwrite=True) + + +@pytest.mark.parametrize("tool_cls", [FileReadTool, FileWriterTool]) +def test_an_os_error_from_the_store_is_reported_too( + tool_cls, failing_store, tmp_path, monkeypatch +): + """Not every store failure arrives as FileStoreError. + + A store wrapping a socket or a subprocess can raise OSError from resolve, + which is neither a ValueError nor something the read/write handlers see. + ``strerror`` is what survives redaction, so that is what an agent gets. + """ + monkeypatch.chdir(tmp_path) + failing_store("resolve", OSError(104, "Connection reset by peer")) + + result = _run_either(tool_cls, tool_cls()) + + assert "Connection reset by peer" in result + + +@pytest.mark.parametrize("tool_cls", [FileReadTool, FileWriterTool]) +def test_a_bare_os_error_degrades_to_its_type_without_raising( + tool_cls, failing_store, tmp_path, monkeypatch +): + """A single-arg OSError has no strerror, so only its type survives. + + That is `format_error_for_display` holding the line it was given in #6692: + an OS-populated OSError renders its absolute filename into `str()`, so the + message is never passed through wholesale. The cost is a thin report for a + hand-raised `OSError("...")`; stores wanting a legible message should raise + `FileStoreError`, whose text is preserved. What matters here is that the + tool still returns rather than raising. + """ + monkeypatch.chdir(tmp_path) + failing_store("resolve", OSError("connection reset by peer")) + + result = _run_either(tool_cls, tool_cls()) + + assert "OSError" in result + assert "store failed" in result + + +def test_a_store_failure_does_not_leak_an_absolute_path( + failing_store, tmp_path, monkeypatch +): + """The store-failure path is agent-visible, so it gets the same redaction.""" + monkeypatch.chdir(tmp_path) + failing_store("resolve", OSError(2, "No such file", str(tmp_path / "secret.txt"))) + + result = FileWriterTool()._run(filename="a.txt", content="x", overwrite=True) + + assert str(tmp_path) not in result