mirror of
https://github.com/crewAIInc/crewAI.git
synced 2026-08-10 08:21:54 +00:00
docs(tools): state what the declared-path pin means across a rebuild
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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UNumDnNbiyw3pv1WakAe6t
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user