From 1c252e2d832cfb057e8cbc999fedb61da6055732 Mon Sep 17 00:00:00 2001 From: Joao Moura Date: Mon, 3 Aug 2026 15:23:50 -0700 Subject: [PATCH] docs(tools): state what the declared-path pin means across a rebuild MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bugbot flagged that `model_post_init` re-derives `_declared_realpath` from the serialized `file_path`, so a rebuild in a different working directory can repoint the declared default. The mechanism is real. Traced it to exactly one case of three: absolute file_path -> survives a rebuild anywhere relative file_path + base_dir -> survives; base_dir is anchored already relative file_path, no base_dir -> re-anchors to the rebuilding cwd Keeping the re-anchor, deliberately. A bare relative path names nothing absolute to preserve, and the alternative is pinning a directory that, for a rebuild in a fresh container, no longer exists — reading a stale absolute path would be the worse failure. It is also not a regression in any case: before this branch a rebuilt reader lost the declared file outright and answered "No file path provided". Rewriting `file_path` to its resolved form at construction would close it, but `tests/agents/test_agent.py:2311` pins that the authored string survives, so that is a public-contract change rather than a fix. A serialized pin field would too, at the cost of a schema change — noted on the thread for whoever reviews, not taken unilaterally. So: all three cases now have a test, and the class docstring says which is which, so the behavior is a decision rather than something a reader has to infer. 38 tests in the seam suite, 338 across the file tools and crewai's tool suite. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01UNumDnNbiyw3pv1WakAe6t --- .../tools/file_read_tool/file_read_tool.py | 9 +++ .../file_storage/test_file_store_seam.py | 65 +++++++++++++++++++ 2 files changed, 74 insertions(+) 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 f507d0f86..c5f015766 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 @@ -50,6 +50,15 @@ class FileReadTool(BaseTool): construction, so a later chdir cannot repoint it, and it can be addressed either by omitting ``file_path`` or by the label shown in the description. + That pin is anchored from what was declared, which matters when a tool is + rebuilt from a serialized crew in a different working directory. An + absolute ``file_path``, or a relative one with ``base_dir`` set, names the + same file after the rebuild as before it. A *relative* ``file_path`` with + no ``base_dir`` names nothing absolute, so it re-anchors to the working + directory of the process doing the rebuilding — the same file the same + arguments would have named there. Pass ``base_dir`` when a declared + relative path must survive a move. + Args: file_path (Optional[str]): Path to the file to be read. If provided, this becomes the default file path for the tool. 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 e4c93f3b5..61c47578f 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,71 @@ def test_reader_round_trips_through_model_dump(store, tmp_path, monkeypatch): assert rebuilt._run() == "dumped\n" +# --- what the declared-path pin means across a rebuild ----------------------- +# +# The pin is derived from what was *declared*, so whether it survives a rebuild +# in a different working directory depends on whether the declaration named +# somewhere absolute. These three cases are the whole story; they are pinned +# here so the behavior is a decision rather than an accident. Note the local +# store is the one that makes this observable, since its `normalize` is the one +# that consults the process cwd. + + +def test_an_absolute_declared_path_survives_a_rebuild_elsewhere(tmp_path, monkeypatch): + """The strongest case: an absolute declaration is cwd-independent.""" + here, there = tmp_path / "here", tmp_path / "there" + here.mkdir(), there.mkdir() + (here / "notes.txt").write_text("from here\n") + (there / "notes.txt").write_text("from there\n") + + monkeypatch.chdir(here) + dumped = FileReadTool(file_path=str(here / "notes.txt")).model_dump() + monkeypatch.chdir(there) + rebuilt = FileReadTool.model_validate(dumped) + + assert rebuilt._run() == "from here\n" + + +def test_a_declared_base_dir_pins_a_relative_path_across_a_rebuild(tmp_path, monkeypatch): + """base_dir is anchored at construction, so it carries the pin with it.""" + here, there = tmp_path / "here", tmp_path / "there" + here.mkdir(), there.mkdir() + (here / "notes.txt").write_text("from here\n") + (there / "notes.txt").write_text("from there\n") + + monkeypatch.chdir(here) + dumped = FileReadTool(file_path="notes.txt", base_dir=str(here)).model_dump() + monkeypatch.chdir(there) + rebuilt = FileReadTool.model_validate(dumped) + + assert rebuilt._run() == "from here\n" + + +def test_a_bare_relative_declared_path_reanchors_on_rebuild(tmp_path, monkeypatch): + """A relative path with no base_dir names nothing absolute to preserve. + + It re-anchors to the rebuilding process's working directory — the same file + the same arguments would name there. Documented rather than "fixed": the + alternative is pinning a path from a working directory that, for a rebuild + in a fresh container, no longer exists. Callers needing the pin to survive + pass `base_dir`, which the test above covers. + """ + here, there = tmp_path / "here", tmp_path / "there" + here.mkdir(), there.mkdir() + (here / "notes.txt").write_text("from here\n") + (there / "notes.txt").write_text("from there\n") + + monkeypatch.chdir(here) + dumped = FileReadTool(file_path="notes.txt").model_dump() + monkeypatch.chdir(there) + rebuilt = FileReadTool.model_validate(dumped) + + assert rebuilt._run() == "from there\n" + # And it is a real re-anchor, not a stale absolute path that happens to read. + # resolve() because the store canonicalizes, and /tmp is a symlink on macOS. + assert rebuilt._declared_realpath == str((there / "notes.txt").resolve()) + + # --- a store that fails ------------------------------------------------------ # # A store may fail where the local filesystem never could. Every exit from