diff --git a/lib/crewai-tools/src/crewai_tools/file_storage/base.py b/lib/crewai-tools/src/crewai_tools/file_storage/base.py index f8b078ef8..b21a13e1e 100644 --- a/lib/crewai-tools/src/crewai_tools/file_storage/base.py +++ b/lib/crewai-tools/src/crewai_tools/file_storage/base.py @@ -53,6 +53,14 @@ class FileStore(Protocol): construction, so it can recognize that path again later even if the working directory has since moved. Unlike :meth:`resolve` it never rejects: a path outside the sandbox still has a canonical form. + + Must be a pure computation on the string: no I/O, no network, and no + raising. The tools call it while a tool is being constructed — + including when pydantic rebuilds a serialized crew — so a store that + reaches its backing service here turns a transient outage into a crew + that cannot load at all. Defer anything that can fail to + :meth:`resolve`, :meth:`open_text` or :meth:`write_text`, where the + tools report failures instead of propagating them. """ def resolve_within(self, directory: str, filename: str) -> str: @@ -71,7 +79,9 @@ class FileStore(Protocol): """Return a label for *resolved* that is safe to show an LLM. Must not leak absolute directory prefixes; the tools put the result - straight into agent-visible output. + straight into agent-visible output. Pure and non-raising for the same + reason as :meth:`normalize` — the reader labels its declared file at + construction time. """ def exists(self, resolved: str) -> bool: 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 c5f015766..5a1bd774f 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 @@ -1,4 +1,5 @@ from itertools import islice +import logging from typing import Any from crewai.tools import BaseTool @@ -11,6 +12,9 @@ from crewai_tools.security.safe_path import ( ) +logger = logging.getLogger(__name__) + + class FileReadToolSchema(BaseModel): """Input for FileReadTool.""" @@ -141,13 +145,36 @@ class FileReadTool(BaseTool): # Anchor base_dir once, so the sandbox root cannot move under a later # chdir while the declared file stays pinned to its original location. + # Deliberately not guarded: anchoring is a containment guarantee, and + # quietly leaving the root relative would let a later chdir move the + # sandbox. A store that cannot normalize should fail loudly here rather + # than hand back a weaker sandbox than the caller asked for. if self.base_dir is not None: self.base_dir = store.normalize(self.base_dir) 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." + # Guarded, unlike base_dir above: the declared default is a + # convenience, so a store hiccuping while canonicalizing a filename + # should not stop a serialized crew from loading. The reader simply + # comes back without a default file, and any real problem resurfaces + # on the first read, where _run reports it instead of raising. + try: + self._declared_realpath = store.normalize(self.file_path, self.base_dir) + self._declared_label = store.display( + self._declared_realpath, self.base_dir + ) + except (FileStoreError, OSError): + logger.warning( + "the %s store could not resolve the declared file %r; the " + "tool will have no default file", + getattr(store, "label", "configured"), + self.file_path, + exc_info=True, + ) + self._declared_realpath = None + self._declared_label = None + else: + 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. 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 61c47578f..8fbb97361 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 @@ -404,6 +404,55 @@ def test_reader_round_trips_through_model_dump(store, tmp_path, monkeypatch): assert rebuilt._run() == "dumped\n" +# --- a store that fails while a tool is being built -------------------------- +# +# `normalize` and `display` are specified as pure and non-raising, precisely +# because the tools call them during construction — including when pydantic +# rebuilds a serialized crew. A store that breaks that contract anyway must not +# take a whole crew down with it, but the two derivations are not equal: the +# declared default file is a convenience, `base_dir` is a containment +# guarantee. So one degrades and one does not. + + +def test_a_store_failing_on_the_declared_file_still_builds_the_tool( + failing_store, tmp_path, monkeypatch, caplog +): + """Convenience: no default file rather than no crew.""" + monkeypatch.chdir(tmp_path) + store = failing_store("display") + + tool = FileReadTool(file_path="notes.txt") + + assert tool._declared_realpath is None + assert tool._declared_label is None + assert "no default file" in caplog.text + # Still a working tool — explicit paths go through the store as usual. + store.failing = "" + store.files["/ws/other.txt"] = "still working\n" + assert tool._run(file_path="other.txt") == "still working\n" + # And omitting the path reports the missing default rather than raising. + assert "No file path provided" in tool._run() + + +def test_a_store_failing_on_base_dir_is_not_swallowed( + failing_store, tmp_path, monkeypatch +): + """Containment: a sandbox root that cannot be anchored must not be faked. + + Degrading here would leave `base_dir` relative, so a later chdir could move + the sandbox — a weaker guarantee than the caller asked for, arrived at + silently. Failing loudly is the point. + """ + monkeypatch.chdir(tmp_path) + failing_store("normalize") + + with pytest.raises((FileStoreError, OSError)): + FileReadTool(base_dir="scoped") + + with pytest.raises((FileStoreError, OSError)): + FileWriterTool(base_dir="scoped") + + # --- what the declared-path pin means across a rebuild ----------------------- # # The pin is derived from what was *declared*, so whether it survives a rebuild