feat(tools): make the file tools' backing store pluggable - #6698
feat(tools): make the file tools' backing store pluggable#6698joaomdmoura wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughChangesFile storage support now exposes a Pluggable file storage
Sequence Diagram(s)sequenceDiagram
participant FileWriterTool
participant resolve_file_store
participant FileStore
participant FileReadTool
FileWriterTool->>resolve_file_store: resolve and bind store
FileWriterTool->>FileStore: resolve path and write_text
FileReadTool->>resolve_file_store: resolve and bind store
FileReadTool->>FileStore: resolve path and open_text
FileStore-->>FileReadTool: return file content
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR introduces a pluggable FileStore abstraction for FileReadTool / FileWriterTool so deployments (e.g. CrewAI AMP) can redirect file I/O to a durable backing store without changing existing tool APIs, while defaulting to the existing local-filesystem behavior.
Changes:
- Added a
FileStoreprotocol plus a process-wide registry (register_file_store_factory/resolve_file_store) with safe fallback to the local filesystem. - Refactored
FileReadToolandFileWriterToolto route normalization, containment checks, and I/O through a bound store instance. - Added seam tests using an in-memory store to prove the tools don’t reach around the store to
open()/os.path.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/crewai-tools/tests/file_storage/test_file_store_seam.py | New tests validating that file tools exclusively use the FileStore seam (including fallback behavior). |
| lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py | Refactors file writing to use FileStore for resolution/containment/I/O and binds the store per tool instance. |
| lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py | Refactors file reading to use FileStore for resolution/containment/I/O and pins declared paths via normalize(). |
| lib/crewai-tools/src/crewai_tools/file_storage/registry.py | New global factory registry for swapping the backing store with safe fallback to local filesystem. |
| lib/crewai-tools/src/crewai_tools/file_storage/local.py | New LocalFileStore implementation that preserves current sandboxed filesystem behavior. |
| lib/crewai-tools/src/crewai_tools/file_storage/base.py | New FileStore protocol + FileStoreError for store-specific failures. |
| lib/crewai-tools/src/crewai_tools/file_storage/init.py | Public exports for the new file storage seam and registry functions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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
🧹 Nitpick comments (1)
lib/crewai-tools/tests/file_storage/test_file_store_seam.py (1)
91-97: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
FileWriterTool(base_dir=...)with the memory store.No test here constructs
FileWriterToolwith an explicitbase_dirunder thestorefixture — that's exactly the gap that hides the_anchor_base_dir/os.path.realpathissue flagged infile_writer_tool.py. A test likeFileWriterTool(base_dir="/ws/sub")._run(...)againstMemoryFileStorewould catch regressions in that seam.🤖 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/tests/file_storage/test_file_store_seam.py` around lines 91 - 97, Add a test in the memory-store fixture coverage that constructs FileWriterTool with an explicit base_dir such as "/ws/sub" and exercises _run to write a file through MemoryFileStore. Keep the existing store fixture setup and assertions focused on successful anchored-path behavior, covering the _anchor_base_dir and os.path.realpath seam.
🤖 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 93-103: Move base_dir anchoring out of the _anchor_base_dir field
validator and into model_post_init after self._store is assigned. Use the
resolved store’s normalize() method to anchor base_dir once, preserving None
values and matching the FileReadTool initialization pattern; remove the
os.path.realpath-based validator.
---
Nitpick comments:
In `@lib/crewai-tools/tests/file_storage/test_file_store_seam.py`:
- Around line 91-97: Add a test in the memory-store fixture coverage that
constructs FileWriterTool with an explicit base_dir such as "/ws/sub" and
exercises _run to write a file through MemoryFileStore. Keep the existing store
fixture setup and assertions focused on successful anchored-path behavior,
covering the _anchor_base_dir and os.path.realpath seam.
🪄 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: 4472258b-5e6b-40bb-bab2-771ff6216cec
📒 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
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py:98
base_diris currently anchored withos.path.realpath()in the field validator. That makesbase_dirlocal-filesystem-specific and can break non-localFileStoreimplementations (e.g., a remote store receiving a local absolute path when the user passed a relativebase_dir). Since the store owns normalization/containment,base_dirshould be normalized viastore.normalize()after the store is bound, and the validator should avoid filesystem resolution.
@field_validator("base_dir")
@classmethod
def _anchor_base_dir(cls, value: str | None) -> str | None:
"""Resolve base_dir once so a later chdir cannot move the sandbox."""
return os.path.realpath(value) if value is not None else None
lib/crewai-tools/src/crewai_tools/file_storage/local.py:57
LocalFileStore.resolve_within()wrapsOSErrorintoValueError(str(exc)). For many OS errors,str(exc)includes the absolute path (e.g. “File name too long: '/abs/...')", which can leak sandbox prefixes in tool error messages. Prefer propagating a sanitized message (likeexc.strerror) when converting toValueError.
try:
resolved = Path(os.path.join(directory, filename)).resolve()
except (OSError, ValueError) as exc:
# e.g. an embedded null byte, which trips the underlying syscall.
raise ValueError(str(exc)) from exc
Addresses review feedback on #6698. Cursor, Copilot and CodeRabbit all independently flagged the same two problems, both of which I introduced. FileWriterTool still anchored base_dir with os.path.realpath in a pydantic field validator. Validators run before model_post_init, so _store was not bound yet and there was nothing else available — but the effect is local-filesystem semantics applied to a path a remote store may not read that way at all, so the computed sandbox root could disagree with the same store's resolve()/resolve_within(). FileReadTool already did this correctly through store.normalize. Anchoring moves into model_post_init, after the store is bound, so every path decision stays inside the seam and the two tools now agree on the same input. It still resolves once, so a later chdir cannot move the sandbox. LocalFileStore.resolve_within wrapped OSError as ValueError(str(exc)), and the writer returns that message verbatim. str() on an OSError carries the absolute filename, so an over-long name or an embedded null byte leaked a host path into agent-visible output — undoing the redaction added in #6692. It now reduces to the reason via format_error_for_display. Also drops the ellipsis bodies from the FileStore protocol methods. Each already carries a docstring, which is a valid body on its own, so the trailing `...` was a genuine no-op statement (8 static-analysis reports) and reads as noise. The protocol still declares all 8 methods and still discriminates conforming from non-conforming stores. The in-memory test double was less faithful than the real cdo store: its resolve() honoured only the store root and ignored base_dir, so it could not have caught the first bug. It now confines to base_dir when one is given, which is what made the new regression test meaningful. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed all the bot review feedback in 82529b8. Every inline thread has a reply. Two real regressions, both mineCursor, Copilot and CodeRabbit flagged these independently, which was a fair signal.
Absolute path leak (Cursor low, Copilot). Copilot also correctly predicted the follow-through: removing the validator stranded the Static-analysis noise, fixed properlyThe 8 "statement has no effect" reports were on the The test double was hiding the first bugWorth calling out: the in-memory store used by the seam tests honoured only its own root and ignored Testing123 passing. Four new tests: the writer anchors through the store (asserting the store root appears and the local cwd does not),
|
This is not true. :) Still, I understand. |
Also not true. Tasks for the same execution should always be able to access the same files. |
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>
82529b8 to
0609959
Compare
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.
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.