From 953e1b94ab08191d07896e9fa10c62cf49fdc0de Mon Sep 17 00:00:00 2001 From: Joao Moura Date: Mon, 3 Aug 2026 15:40:48 -0700 Subject: [PATCH] fix(tools): make construction-time store failures non-fatal where they can be MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Copilot flagged that `model_post_init` calls `store.normalize` / `store.display` unguarded, so a store raising there stops a serialized crew from loading — against this PR's own claim that a broken integration degrades rather than taking file I/O down. It suggested falling back to the local store. Not doing that: silently redirecting a crew configured for durable storage onto a disk that will be discarded trades a loud failure for quiet data loss, which is the exact bug this PR exists to fix. Three changes instead. The protocol now *states* the invariant it only implied. `normalize` and `display` must be pure string computation — no I/O, no raising — because the tools call them while a tool is being constructed. Both real stores already comply (`CdoFileStore` does `posixpath` arithmetic and never touches its client); this makes it a contract a new store is held to rather than a coincidence. The reader's declared-file derivation is now guarded, because a crew that cannot load over a *convenience default filename* is indefensible. It comes back without a default file and logs; any real problem resurfaces on the first read, where `_run` already reports it. `base_dir` anchoring stays unguarded, deliberately, and now says why: it is a containment guarantee, not a convenience. Leaving the root relative because a store hiccuped would let a later chdir move the sandbox — handing back a weaker sandbox than the caller asked for, silently. That failure should surface. 104 tests across the file tools. Two new, one per half of the asymmetry. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UNumDnNbiyw3pv1WakAe6t --- .../src/crewai_tools/file_storage/base.py | 12 ++++- .../tools/file_read_tool/file_read_tool.py | 33 +++++++++++-- .../file_storage/test_file_store_seam.py | 49 +++++++++++++++++++ 3 files changed, 90 insertions(+), 4 deletions(-) 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