feat(tools): make the file tools' backing store pluggable - #6709
Open
joaomdmoura wants to merge 1 commit into
Open
feat(tools): make the file tools' backing store pluggable#6709joaomdmoura wants to merge 1 commit into
joaomdmoura wants to merge 1 commit into
Conversation
FileReadTool and FileWriterTool assume a durable local disk. That holds on a developer's machine and breaks in any deployment environment where the runtime is ephemeral: whatever an agent writes is discarded when the run ends, and a later run cannot read it back. A crew that generates a report in one task and reads it in the next passes locally and fails there. This adds the seam needed to point those tools at durable storage instead. Both now route every path resolution and every read/write through a FileStore, defaulting to LocalFileStore — the current filesystem behavior, moved rather than rewritten. A deployment registers a different store through register_file_store_factory and the tools pick it up. The store owns its own containment, because the tools call nothing else before doing I/O. For the local store that stays validate_file_path plus the is_relative_to check; another store enforces whatever its own namespace requires, which may be prefix-based rather than realpath-based. resolve() and normalize() are separate so the reader can still pin its declared file for identity without a containment check, and base_dir is anchored through the store so both tools derive the same sandbox root from the same input. open_text() returns a handle rather than a string, which keeps the local store lazy: reading a small window out of a huge file does not pull the whole thing into memory. A store that must fetch eagerly can wrap its payload in StringIO. Behaviour is unchanged: every pre-existing file tool test passes untouched. The new suite stands in a store backed by a dict with no filesystem at all, which is what proves the seam is real — a tool that reached past it to open() or os.path would fail those assertions. It also covers the fallbacks that keep this safe to ship before any integration exists: a factory returning None, and a factory that raises, both leave the local filesystem in place rather than breaking file I/O. No new dependencies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughChangesThe pull request adds a pluggable File storage seam
Sequence Diagram(s)sequenceDiagram
participant FileWriterTool
participant FileStore
participant FileReadTool
FileWriterTool->>FileStore: resolve and resolve_within target
FileWriterTool->>FileStore: ensure_parent and write_text
FileReadTool->>FileStore: normalize and resolve path
FileReadTool->>FileStore: open_text resolved file
FileStore-->>FileReadTool: text content
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0609959. Configure here.
| # 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 = os.path.realpath(base_dir) |
There was a problem hiding this comment.
Reader store unbound on reconstruction
Medium Severity
FileReadTool binds _store only in its custom __init__, while FileWriterTool binds it in model_post_init so pydantic reconstruction still works. BaseTool checkpoint deserialization uses model_validate, which skips custom __init__, so _store stays None and every read hits an AttributeError. Before this change, reconstructed readers could still call validate_file_path / open directly.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 0609959. Configure here.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a pluggable FileStore seam so FileReadTool / FileWriterTool can route all path resolution and I/O through a durable backing store (while defaulting to the current local-filesystem behavior via LocalFileStore).
Changes:
- Added a
FileStoreprotocol plus aLocalFileStoreimplementation and a process-wide registry (register_file_store_factory/resolve_file_store). - Updated
FileReadToolandFileWriterToolto bind a store once per tool instance and perform all resolve/read/write operations through it. - Added dict-backed tests proving file tools do not bypass the store seam and verifying sandbox/containment and fallback behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/crewai-tools/tests/file_storage/test_file_store_seam.py | Adds seam-focused tests using an in-memory store to prove all I/O routes through FileStore. |
| lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py | Refactors writer to resolve paths and write via a bound FileStore instance. |
| lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py | Refactors reader to resolve/pin paths and read via a bound FileStore instance. |
| lib/crewai-tools/src/crewai_tools/file_storage/registry.py | Adds the global factory registry with safe fallbacks to the local store. |
| lib/crewai-tools/src/crewai_tools/file_storage/local.py | Implements LocalFileStore preserving existing sandboxed filesystem behavior. |
| lib/crewai-tools/src/crewai_tools/file_storage/base.py | Defines the FileStore protocol and FileStoreError. |
| lib/crewai-tools/src/crewai_tools/file_storage/init.py | Exposes the new storage seam API surface from the package. |
Comments suppressed due to low confidence (1)
lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py:139
store.resolve_within(...)is only caught asValueError. If a customFileStoreraisesOSError/FileStoreError(e.g. invalid key syntax, backend failure),_runwill raise instead of returning a structured error message.
# Then keep filename inside that directory.
try:
resolved_filepath = store.resolve_within(resolved_directory, filename)
except ValueError as e:
return f"Error: Invalid file path — {e!s}"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
126
to
129
| try: | ||
| resolved_directory = Path(validate_file_path(directory, self.base_dir)) | ||
| resolved_directory = store.resolve(directory, self.base_dir) | ||
| except ValueError as e: | ||
| return "Error: Invalid directory: " + format_sandbox_error( |
Comment on lines
171
to
178
| try: | ||
| file_path = self._resolve_path(file_path) | ||
| except ValueError as e: | ||
| return "Error: Invalid file path: " + format_sandbox_error( | ||
| e, | ||
| "Pass base_dir to FileReadTool to allow reading another " | ||
| "directory tree.", | ||
| ) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py`:
- Around line 121-139: Handle FileStoreError at every identified file-store
boundary, returning the same sanitized file-tool error format instead of
allowing it to escape. In file_writer_tool.py ranges 121-139 and 144-158, add
handling around store.resolve, store.resolve_within, store.ensure_parent, and
unguarded store.exists; in file_read_tool.py range 166-178, cover _resolve_path
and the read I/O boundary. Preserve existing handling for other exception types
and use each error’s details in the returned message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fb209830-11e6-43ab-a681-8b0101074b91
📒 Files selected for processing (7)
lib/crewai-tools/src/crewai_tools/file_storage/__init__.pylib/crewai-tools/src/crewai_tools/file_storage/base.pylib/crewai-tools/src/crewai_tools/file_storage/local.pylib/crewai-tools/src/crewai_tools/file_storage/registry.pylib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.pylib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.pylib/crewai-tools/tests/file_storage/test_file_store_seam.py
Comment on lines
+121
to
+139
| store = self._store | ||
|
|
||
| # Confine the target directory to base_dir so an LLM-chosen directory | ||
| # cannot reach outside the sandbox. validate_file_path also resolves | ||
| # symlinks and ".." components. | ||
| # cannot reach outside the sandbox. The store also resolves symlinks | ||
| # and ".." components before checking. | ||
| try: | ||
| resolved_directory = Path(validate_file_path(directory, self.base_dir)) | ||
| resolved_directory = store.resolve(directory, self.base_dir) | ||
| except ValueError as e: | ||
| return "Error: Invalid directory: " + format_sandbox_error( | ||
| e, | ||
| "Pass base_dir to FileWriterTool to allow writing to another " | ||
| "directory tree.", | ||
| ) | ||
|
|
||
| # Keep filename inside the target directory, blocking "..", absolute | ||
| # paths and symlink escapes. is_relative_to() compares whole path | ||
| # components, so it is safe on case-insensitive filesystems and avoids | ||
| # the "//" prefix edge case. A filepath that resolves to the directory | ||
| # itself (e.g. an empty filename) is not a valid file target. | ||
| # Then keep filename inside that directory. | ||
| try: | ||
| resolved_filepath = Path( | ||
| os.path.join(resolved_directory, filename) | ||
| ).resolve() | ||
| except (OSError, ValueError) as e: | ||
| # e.g. an embedded null byte, which trips the underlying syscall. | ||
| return f"Error: Invalid file path: {format_error_for_display(e)}" | ||
|
|
||
| display_filepath = format_path_for_display( | ||
| str(resolved_filepath), str(resolved_directory) | ||
| ) | ||
| if ( | ||
| not resolved_filepath.is_relative_to(resolved_directory) | ||
| or resolved_filepath == resolved_directory | ||
| ): | ||
| return "Error: Invalid file path — the filename must not escape the target directory." | ||
| resolved_filepath = store.resolve_within(resolved_directory, filename) | ||
| except ValueError as e: | ||
| return f"Error: Invalid file path — {e!s}" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files =="
git ls-files | rg '(^lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py$|^lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py$|^lib/crewai-tools/src/crewai_tools/file_storage/base.py$)|file_storage/|FileStore|FileStoreError' || true
echo "== file_writer_tool outline =="
ast-grep outline lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py --view compact || true
echo "== file_read_tool outline =="
ast-grep outline lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py --view compact || true
echo "== file_storage base outline =="
ast-grep outline lib/crewai-tools/src/crewai_tools/file_storage/base.py --view compact || true
echo "== relevant file_writer_tool =="
cat -n lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py | sed -n '1,220p'
echo "== relevant file_read_tool =="
cat -n lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py | sed -n '1,240p'
echo "== relevant file_storage base =="
cat -n lib/crewai-tools/src/crewai_tools/file_storage/base.py | sed -n '1,260p'Repository: crewAIInc/crewAI
Length of output: 24845
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file_storage implementations =="
for f in lib/crewai-tools/src/crewai_tools/file_storage/*.py lib/crewai-tools/src/crewai_tools/file_storage/__init__.py; do
echo "--- $f ---"
if [ -f "$f" ]; then
cat -n "$f" | sed -n '1,240p'
fi
done
echo "== tests for FileStoreError handling =="
cat -n lib/crewai-tools/src/crewai_tools/tests/file_storage/test_file_store_seam.py | sed -n '1,260p'
echo "== search FileStoreError usages =="
rg -n "FileStoreError|except Exception|except ValueError|except FileExistsError|except OSError" lib/crewai-tools/src/crewai_tools/file_storage lib/crewai-tools/src/crewai_tools/tools/file_writer_tool lib/crewai-tools/src/crewai_tools/tools/file_read_tool
echo "== determine which FileStore methods catch generic exceptions at implementation sites =="
python3 - <<'PY'
from pathlib import Path
for path in sorted(Path("lib/crewai-tools/src/crewai_tools/file_storage").glob("*.py")):
text = path.read_text()
if "resolve" in text or "ensure_parent" in text or "exists" in text:
print(f"\n{path}")
for method in ["resolve", "resolve_within", "ensure_parent", "exists", "open_text", "write_text", "normalize", "display"]:
# quick text context around each method body start
if f"def {method}" in text:
lines=text.splitlines()
for i,line in enumerate(lines,1):
if f"def {method}" in line:
print(f" {method}: lines {i}-{min(len(lines), i+22)}")
for j in range(i,min(len(lines), i+22)+1):
s=lines[j-1]
if j>i+18 or any(k in s for k in ["FileStoreError","except ","return "]):
print(f" {j:4}: {s}")
PYRepository: crewAIInc/crewAI
Length of output: 14926
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== search external repo for FileStoreError behavior =="
rg -n "(Error executing tool|error executing tool|Error.*executing|FileStoreError|error executing)" . -S --glob '*.py' | head -200 || true
echo "== git status/diff stat =="
git diff --stat || true
git diff --numstat || true
echo "== read-only behavioral probe for current local store guard coverage =="
python3 - <<'PY'
from pathlib import Path
cases = [
("file_writer_tool.py", "FWR", ["resolve", "resolve_within", "ensure_parent", "exists", "write_text", "open_text"]),
("file_read_tool.py", "FRD", ["_resolve_path", "open_text"]),
]
for root in ["lib/crewai-tools/src/crewai_tools", "tests"]:
p = Path(root, "file_storage/test_file_store_seam.py")
if p.exists():
print(f"--- tests at {p} ---")
text=p.read_text()
for term in ["FileStoreError", "store.resolve", "store.exists", "ensure_parent", "file_writer_tool", "file_read_tool", "return"]:
if term in text:
print(f"{term}: present")
print(text[:2000])
# Extract each store-boundary call inside FileReadTool<FileWriterTool._run blocks and report the exception types handled nearby.
files = {
"lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py": range(106, 175),
"lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py": range(156, 212),
}
for path, linespec in files.items():
text = Path(path).read_text()
lines = text.splitlines()
print(f"\n--- {path} handle-report ---")
in_try = None
handler_types = set()
for i in linespec:
line = lines[i-1]
if "try:" in line:
in_try = i
handler_types = set()
elif line.strip().startswith("except ") and in_try is not None:
# crude text parse: capture exception class names
s = line.strip().replace(",", " ").replace(" as e", " ")
for part in s.split():
if part.isidentifier():
handler_types.add(part)
elif ("store." in line or "_resolve_path" in line or "store.exists" in line) and in_try is not None and any(k in line for k in ["resolve(", "resolve_within(", "ensure_parent(", "exists(", "write_text(", "open_text(", "_resolve_path"]):
# Find nearest prior except; for current line use current handlers
print(f"{i}: {line.strip()} -> handlers: {sorted(handler_types)}")
PYRepository: crewAIInc/crewAI
Length of output: 5150
Handle FileStoreError at file-store boundaries. FileStoreError is the sanctioned exception for non-local-store failures, but FileReadTool._run() and FileWriterTool._run() only catch filesystem-specific types here; a remote store that raises this will escape as Error executing tool: ... instead of a sanitized file-tool error. Add except FileStoreError as e: handling around store.resolve/store.resolve_within, store.ensure_parent, unguarded store.exists, _resolve_path, and the read path’s I/O boundary.
📍 Affects 2 files
lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py#L121-L139(this comment)lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py#L144-L158lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py#L166-L178
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py`
around lines 121 - 139, Handle FileStoreError at every identified file-store
boundary, returning the same sanitized file-tool error format instead of
allowing it to escape. In file_writer_tool.py ranges 121-139 and 144-158, add
handling around store.resolve, store.resolve_within, store.ensure_parent, and
unguarded store.exists; in file_read_tool.py range 166-178, cover _resolve_path
and the read I/O boundary. Preserve existing handling for other exception types
and use each error’s details in the returned message.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
FileReadToolandFileWriterToolassume a durable local disk. That holds on a developer's machine and breaks in any deployment environment where the runtime is ephemeral: whatever an agent writes is discarded when the run ends, and a later run can't read it back. A crew that generates a report in one task and reads it in the next passes locally and fails there.This adds the seam needed to point those tools at durable storage instead. It is self-contained and changes no current behavior.
What this adds
A
FileStoreseam. Both tools now do every path resolution and every read/write through a store, defaulting toLocalFileStore— today's filesystem behavior, moved rather than rewritten. A deployment swaps in a different store withregister_file_store_factory.Design notes worth reviewing:
resolve()/resolve_within()must reject anything out of bounds. Locally that staysvalidate_file_pathplus theis_relative_tocheck; another store enforces whatever its own namespace requires, which may be prefix-based rather thanrealpath-based.resolve()andnormalize()are separate. The reader pins the file declared at construction so a laterchdircan't repoint it — that needs canonicalization without a containment check, whichnormalize()provides.open_text()returns a handle, not a string. That keeps the local store lazy, sostart_line/line_countstill reads a window out of a huge file without pulling it into memory. Stores that must fetch eagerly wrap the payload inStringIO.Safety
Two fallbacks make this inert until a store is actually registered, and keep a broken integration from taking file I/O down:
None(its backing service isn't configured) → local filesystem.Testing
123 tests pass. All pre-existing file tool tests are untouched — the refactor is behavior-preserving, which is the main thing to check.
The new tests stand in a store backed by a plain dict with no filesystem behind it. That's what proves the seam is real: a tool that reached past it to
open()oros.pathwould fail them. They assert the writer leaves the real cwd empty, that a file written by one tool is readable by the other (the round-trip that breaks on an ephemeral runtime), that line windows work against a non-filesystem store, that containment is honoured in both directions, that both tools derive the same sandbox root from the samebase_dir, that anOSErrormessage never carries an absolute path, and both fallbacks above.ruffand strictmypyclean,tool.specs.jsonregenerated.🤖 Generated with Claude Code
Note
Medium Risk
Touches agent file I/O and path sandboxing; behavior is intended to be preserved locally, but custom stores and global factory state add integration surface area.
Overview
Introduces a
FileStoreseam soFileReadToolandFileWriterToolcan persist to durable backends (e.g. object storage) without changing crew or tool arguments. Deployments register a process-wide factory viaregister_file_store_factory; tools resolve a store once at construction and route all path handling and I/O through it.LocalFileStorepreserves today’s sandboxed filesystem behavior (existingvalidate_file_path/ display helpers), moved behind the protocol. Custom stores own containment viaresolve/resolve_within, exposenormalizeseparately for pinning declared paths, and useopen_texthandles so line-window reads stay lazy.FileWriterToolanchorsbase_dirinmodel_post_init(not a field validator) so normalization uses store semantics. FactoryNoneor raised exceptions fall back to the local store with a warning. New dict-backed tests prove tools never bypass the seam and that read/write round-trips work off-disk.Reviewed by Cursor Bugbot for commit 0609959. Bugbot is set up for automated code reviews on this repo. Configure here.