Skip to content

fix(tools): sandbox FileWriterTool writes and fix file tool rough edges - #6692

Merged
joaomdmoura merged 7 commits into
mainfrom
fix/file-tools-path-sandbox
Jul 28, 2026
Merged

fix(tools): sandbox FileWriterTool writes and fix file tool rough edges#6692
joaomdmoura merged 7 commits into
mainfrom
fix/file-tools-path-sandbox

Conversation

@joaomdmoura

@joaomdmoura joaomdmoura commented Jul 28, 2026

Copy link
Copy Markdown
Collaborator

Why

FileReadTool confined reads to the working directory, but FileWriterTool only checked that filename stayed inside directory — and directory is itself an LLM-supplied schema field with no validation. So an agent could write anywhere the process had permission to:

w._run(filename="pwned.txt", directory="/any/dir/outside/cwd", content="X", overwrite=True)
# -> Content successfully written to pwned.txt    (and makedirs created the parents)

r._run(file_path="/any/dir/outside/cwd/pwned.txt")
# -> Error: Invalid file path: ... outside the allowed directory

The reader could not read back what the writer had just written. FileWriterTool was the only filesystem tool in the package that did not route through validate_file_pathfiles_compressor_tool validates even its output path. The existing write tests covered only filename escaping directory, never directory escaping the working directory.

What changed

Write sandbox. The resolved directory must now sit inside base_dir (the working directory by default), and the resolved file must sit inside that directory. The pre-existing filename-containment check is kept verbatim and still applies even when CREWAI_TOOLS_ALLOW_UNSAFE_PATHS is set, so the guarantee added in #4895 is not weakened.

base_dir on both tools, so widening the sandbox is a deliberate per-tool decision rather than a process-wide kill switch that also disables SSRF protection on URL-fetching tools. Relative directories anchor to base_dir, so FileWriterTool(base_dir="/var/output") works with the default directory.

FileReadTool no longer rejects its own constructor path. FileReadTool(file_path="/data/report.csv") built a friendly description at construction and then failed on every call. That path is developer-declared intent, so it is always readable — and declaring one file does not expose its siblings.

Also fixed:

Before After
Line windows Scanned the whole file — 10 lines from a 5 GB log read 5 GB islice stops at the last requested line
_run(**kwargs) Documented positional snippet raised TypeError; missing overwrite gave "error accessing key" Named params in the documented (filename, content, directory) order
directory names an existing file "already exists and overwrite option was not passed" — with overwrite=True Explains the real problem
Nested filename No such file or directory unless directory was passed Parents created either way
Encoding Locale default — cp1252 on Windows UTF-8 default, encoding field to override
Writer schema No field descriptions for the LLM Described, including the accepted overwrite spellings

Docs. Removed the claim that FileReadTool parses JSON into a dict (it never has) in all four locales, replaced the snippet that raised TypeError, documented the sandbox and base_dir, and dropped the stray "Here's the rewritten README for the FileWriterTool:" preamble that had been committed verbatim.

Breaking change & migration

Exactly one change narrows existing behavior: FileWriterTool no longer writes outside the working directory.

Status
Relative directory ("output", default "./") works, unchanged
Absolute directory inside the working directory works, unchanged
Absolute directory outside it (/tmp/out, /Users/me/reports) now rejected

The rejection returns an error string rather than raising, so the crew does not crash and the agent can retry with a relative path. The message names base_dir as the fix:

Error: Invalid directory: Path 'out' is outside the allowed directory.
Pass base_dir to FileWriterTool to allow writing to another directory tree.

To migrate, authorize the tree you intend:

FileWriterTool(base_dir="/var/output")   # agent may write anywhere under it

CREWAI_TOOLS_ALLOW_UNSAFE_PATHS=true restores the old behavior, but it is process-wide and also disables the SSRF protections on URL-fetching tools, so prefer base_dir.

Everything else widens or is cosmetic

  • Reads are not newly restricted. FileReadTool has been confined to the working directory since fix: add SSRF and path traversal protections #5315; this PR only relaxes that (constructor-declared paths, plus base_dir).
  • file_path became optional (was required) — strictly more permissive, and it makes the long-documented run()-with-no-arguments case work for the first time.
  • Nested filename parents are now created instead of erroring.
  • One behavior change worth noting: both tools now use UTF-8 instead of the platform locale encoding. A no-op on Linux/macOS under a UTF-8 locale. On Windows (cp1252) the bytes written for non-ASCII content change, and reading a legacy cp1252 file now fails with a decode error — pass encoding="cp1252" for those. The docs already claimed UTF-8, so this aligns the code with documented behavior.
  • One narrow signature change: FileWriterTool._run(**kwargs) became named parameters. Keyword calls are unchanged and positional calls now work (they previously always raised). The only regression is a direct ._run(...) with unknown extra kwargs, which used to be silently swallowed. Agents are unaffected — the public run() path validates against the schema, which drops extras before _run is reached.
  • Error-message text and the tool descriptions changed, which affects only exact-string assertions and LLM prompt caches.

Testing

53 tests pass (18 read, 35 write), up from 24. New coverage: directory escaping the working directory (absolute and via ..), base_dir widening and still blocking, relative-directory anchoring, the escape hatch, the constructor-path exemption not widening the sandbox, early-stop verified by counting lines consumed, nested-filename parents, the corrected directory-is-a-file message, UTF-8 defaults and encoding overrides, and the documented positional signature.

ruff check, ruff format --check, and strict mypy are clean. lib/crewai agent tool tests pass. Two lib/cli/tests/test_create_crew.py failures and the files_compressor_tool_test.py collection error reproduce on a clean origin/main and are unrelated to this change.

🤖 Generated with Claude Code


Note

High Risk
Changes agent filesystem sandboxing and breaks prior FileWriterTool behavior for absolute/out-of-cwd directories; security-sensitive but heavily tested.

Overview
FileWriterTool now sandboxes LLM-chosen paths: the resolved directory must stay inside base_dir (cwd by default), and the file must stay inside that directory—closing a gap where writes could land anywhere the process could touch while reads were already confined.

Both FileReadTool and FileWriterTool gain configurable base_dir and encoding (UTF-8 default). FileReadTool treats a constructor file_path as developer intent (readable even outside the sandbox, pinned across chdir, without exposing sibling files), makes runtime file_path optional when a default exists, uses islice so line windows do not scan huge files, and surfaces sandbox failures via format_sandbox_error (prefer base_dir over the global unsafe-paths env var).

FileWriterTool switches to explicit _run parameters, richer schema field descriptions, clearer errors (e.g. directory vs file conflicts), and parent creation for nested filename paths. Docs and tool.specs.json are updated; examples use run() instead of _run(), and the incorrect “JSON → dict” read behavior is removed.

Breaking: writes outside the working directory require FileWriterTool(base_dir=...) (or the escape hatch).

Reviewed by Cursor Bugbot for commit 570938e. Bugbot is set up for automated code reviews on this repo. Configure here.

FileReadTool confined reads to the working directory, but FileWriterTool
only checked that `filename` stayed inside `directory` — and `directory`
itself is an LLM-supplied schema field. An agent could therefore write
anywhere the process had permission to, including ~/.ssh and site-packages,
while the reader refused to read back what the writer had just written.
FileWriterTool was the only filesystem tool in the package that did not go
through validate_file_path; files_compressor_tool validates even its
output path.

Writes are now confined to base_dir (the working directory by default):
the resolved directory must sit inside base_dir, and the resolved file
must sit inside that directory. The pre-existing filename containment
check is kept as-is and still applies even when the unsafe-paths escape
hatch is on, so no existing guarantee is weakened.

Both tools gain a base_dir field so a developer can widen the sandbox
deliberately instead of reaching for the process-wide
CREWAI_TOOLS_ALLOW_UNSAFE_PATHS kill switch. FileReadTool also stops
rejecting a file_path given to its own constructor: that is
developer-declared intent, and declaring one file does not expose its
siblings.

Also fixed:

- FileReadTool scanned the whole file when reading a line window; it now
  stops via islice once the requested lines are collected.
- FileWriterTool._run(**kwargs) made the documented positional call
  signature raise TypeError and turned a missing overwrite into
  "error accessing key". It now takes named parameters in the documented
  (filename, content, directory) order.
- A directory naming an existing file reported "already exists and
  overwrite option was not passed" even with overwrite=True; it now
  explains the real problem.
- Subdirectories inside filename are created, matching what passing
  directory already did.
- Both tools now write and decode UTF-8 by default instead of the
  platform locale encoding, with an encoding field to override. The docs
  already claimed UTF-8 and recommended the writer to Windows users.
- The writer's schema fields had no descriptions for the LLM.
- Docs claimed FileReadTool parses JSON into a dict (it never has),
  shipped a snippet that raised TypeError, and did not mention the path
  sandbox. The writer README also began with a stray "Here's the
  rewritten README" preamble.

BREAKING CHANGE: FileWriterTool no longer writes outside the working
directory. Pass base_dir to authorize a different tree.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 28, 2026 08:08
@coderabbitai

coderabbitai Bot commented Jul 28, 2026

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

FileReadTool and FileWriterTool now support sandboxed path resolution, configurable encodings, updated runtime APIs, efficient line-window reads, richer schemas, and expanded documentation and tests across supported locales.

Changes

File tool behavior

Layer / File(s) Summary
FileReadTool path and range handling
lib/crewai-tools/src/crewai_tools/tools/file_read_tool/*, lib/crewai-tools/tests/file_read_tool_test.py, docs/edge/*/tools/file-document/filereadtool.mdx, lib/crewai-tools/tool.specs.json
FileReadTool validates runtime paths against base_dir, preserves constructor-declared paths, supports configurable encodings and bounded line reads, and documents these behaviors.
FileWriterTool sandboxed writes
lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/*, lib/crewai-tools/tests/tools/test_file_writer_tool.py, docs/edge/*/tools/file-document/filewritetool.mdx, lib/crewai-tools/tool.specs.json
FileWriterTool uses typed inputs, validates directories and resolved targets, creates parent directories, writes with configurable encoding, and documents overwrite and sandbox behavior.

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant FileReadTool
  participant FileWriterTool
  participant Filesystem
  Caller->>FileReadTool: read a validated line window
  FileReadTool-->>Caller: return plain text
  Caller->>FileWriterTool: write content with named arguments
  FileWriterTool->>Filesystem: create parent directories and write encoded content
  Filesystem-->>FileWriterTool: write result
  FileWriterTool-->>Caller: return success or error
Loading

Suggested reviewers: copilot

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 59.09% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title matches the main change: sandboxing FileWriterTool writes and addressing file tool edge cases.
Description check ✅ Passed The description is directly related to the changeset and accurately explains the sandboxing, path, and docs updates.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/file-tools-path-sandbox

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Hardens crewai-tools filesystem tools by aligning FileWriterTool sandboxing behavior with FileReadTool, introducing an explicit base_dir boundary (defaulting to the working directory) and improving ergonomics (UTF-8 default encoding, better errors, and efficient partial reads). This reduces the risk of LLM-controlled path parameters writing/reading outside intended directories and updates docs/tests accordingly.

Changes:

  • Sandboxed FileWriterTool writes under base_dir, added UTF-8 default + configurable encoding, improved directory/overwrite behaviors, and clarified schemas/docs.
  • Improved FileReadTool with base_dir, UTF-8 default + configurable encoding, constructor-path exemption, and islice-based early-stop for line windows.
  • Expanded test coverage substantially and updated docs across multiple locales to reflect the new sandboxing and usage patterns.

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py Adds base_dir sandboxing + UTF-8 default encoding and improves write-path handling and errors.
lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py Adds base_dir/encoding, constructor-path exemption, and islice-based partial reads.
lib/crewai-tools/tests/tools/test_file_writer_tool.py Adds extensive writer sandbox/encoding/behavior tests and updates fixture to run under cwd sandbox.
lib/crewai-tools/tests/file_read_tool_test.py Adds tests for constructor exemption, base_dir behavior, early-stop window reads, and encoding/decode errors.
lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/README.md Documents sandboxing, base_dir, UTF-8 default, and updates examples to use run().
lib/crewai-tools/src/crewai_tools/tools/file_read_tool/README.md Corrects behavior claims (returns text only), documents sandboxing and base_dir/encoding, and updates examples.
docs/edge/en/tools/file-document/filewritetool.mdx Updates English docs for sandboxing, UTF-8 default, constructor options, and run() example.
docs/edge/en/tools/file-document/filereadtool.mdx Updates English docs to reflect text-only return, windowed reads, and sandboxing rules.
docs/edge/pt-BR/tools/file-document/filewritetool.mdx Updates Portuguese docs example to use run() with named parameters.
docs/edge/pt-BR/tools/file-document/filereadtool.mdx Removes incorrect JSON-to-dict claim in Portuguese docs.
docs/edge/ko/tools/file-document/filewritetool.mdx Updates Korean docs example to use run() with named parameters.
docs/edge/ko/tools/file-document/filereadtool.mdx Removes incorrect JSON-to-dict claim in Korean docs.
docs/edge/ar/tools/file-document/filewritetool.mdx Updates Arabic docs example to use run() with named parameters.
docs/edge/ar/tools/file-document/filereadtool.mdx Removes incorrect JSON-to-dict claim in Arabic docs.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py Outdated
Copilot AI review requested due to automatic review settings July 28, 2026 08:13

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
docs/edge/ko/tools/file-document/filereadtool.mdx (1)

16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Document the sandbox API consistently across localized guides.

The non-English guides omit base_dir, encoding, runtime containment, and the process-wide unsafe-path override, despite this being a breaking behavior change.

  • docs/edge/ko/tools/file-document/filereadtool.mdx#L16-L16: Add localized runtime arguments and sandbox guidance.
  • docs/edge/ar/tools/file-document/filereadtool.mdx#L14-L14: Add localized runtime arguments and sandbox guidance.
  • docs/edge/pt-BR/tools/file-document/filereadtool.mdx#L16-L16: Add localized runtime arguments and sandbox guidance.
🤖 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 `@docs/edge/ko/tools/file-document/filereadtool.mdx` at line 16, Update the
localized FileReadTool guides to document the runtime arguments base_dir and
encoding, explain that file access is contained within the configured sandbox,
and describe the process-wide unsafe-path override. Apply the localized guidance
at docs/edge/ko/tools/file-document/filereadtool.mdx lines 16-16,
docs/edge/ar/tools/file-document/filereadtool.mdx lines 14-14, and
docs/edge/pt-BR/tools/file-document/filereadtool.mdx lines 16-16, preserving
each guide’s language and existing formatting.
🤖 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 `@docs/edge/ar/tools/file-document/filereadtool.mdx`:
- Line 14: In the FileReadTool description, replace the Arabic spelling
“مفهومياً” with “مفهوميًا” while leaving the surrounding text unchanged.

In
`@lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py`:
- Around line 92-113: Wrap the filepath construction and resolution in _run with
the same invalid-input exception handling used for resolved_directory, catching
ValueError and OSError and returning a descriptive invalid file path error
string. Keep the existing containment and directory-target checks unchanged for
successfully resolved paths.

---

Nitpick comments:
In `@docs/edge/ko/tools/file-document/filereadtool.mdx`:
- Line 16: Update the localized FileReadTool guides to document the runtime
arguments base_dir and encoding, explain that file access is contained within
the configured sandbox, and describe the process-wide unsafe-path override.
Apply the localized guidance at
docs/edge/ko/tools/file-document/filereadtool.mdx lines 16-16,
docs/edge/ar/tools/file-document/filereadtool.mdx lines 14-14, and
docs/edge/pt-BR/tools/file-document/filereadtool.mdx lines 16-16, preserving
each guide’s language and existing formatting.
🪄 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: 86d4e24a-7a0b-41ed-a195-47ceaa699a62

📥 Commits

Reviewing files that changed from the base of the PR and between 97981ed and 64baf5c.

📒 Files selected for processing (15)
  • docs/edge/ar/tools/file-document/filereadtool.mdx
  • docs/edge/ar/tools/file-document/filewritetool.mdx
  • docs/edge/en/tools/file-document/filereadtool.mdx
  • docs/edge/en/tools/file-document/filewritetool.mdx
  • docs/edge/ko/tools/file-document/filereadtool.mdx
  • docs/edge/ko/tools/file-document/filewritetool.mdx
  • docs/edge/pt-BR/tools/file-document/filereadtool.mdx
  • docs/edge/pt-BR/tools/file-document/filewritetool.mdx
  • lib/crewai-tools/src/crewai_tools/tools/file_read_tool/README.md
  • lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py
  • lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/README.md
  • lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py
  • lib/crewai-tools/tests/file_read_tool_test.py
  • lib/crewai-tools/tests/tools/test_file_writer_tool.py
  • lib/crewai-tools/tool.specs.json

Comment thread docs/edge/ar/tools/file-document/filereadtool.mdx Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py:91

  • FileReadTool treats a constructor file_path as trusted by comparing os.path.realpath(file_path) to os.path.realpath(self.file_path). If a developer passes a relative file_path and the working directory changes after construction, self.file_path will resolve against the new CWD and can point at a different file (or fail), breaking the “developer-declared intent” guarantee.

Consider normalizing the constructor file_path to an absolute real path at initialization so it remains stable regardless of later cwd changes.

        if file_path is not None:
            display_path = format_path_for_display(file_path, base_dir)
            kwargs["description"] = (
                f"A tool that reads file content. The default file is {display_path}, but you can provide a different 'file_path' parameter to read another file. You can also specify 'start_line' and 'line_count' to read specific parts of the file."
            )

@mintlify

mintlify Bot commented Jul 28, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
crewai 🟢 Ready View Preview Jul 28, 2026, 8:24 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

Addresses review feedback on #6692.

The constructor-path exemption did not actually work the way an agent
calls the tool. The description only advertises a redacted label (the
basename, when the file sits outside the sandbox), but resolution
required the exact absolute path, so the model's call was sandboxed and
the declared file was never read. Worse, file_path was a required schema
field, so the long-documented "call with no arguments to read the default
file" raised a validation error instead:

    FileReadTool(file_path="/outside/declared.txt")
    .run()                          -> ValueError: validation failed
    .run(file_path="declared.txt")  -> Error: File not found
    .run(file_path="/outside/declared.txt") -> works, but the model was
                                               never told this path

file_path is now optional in the schema, so omitting it reads the default,
and the declared file is addressable by the label the description shows
the model as well as by its real path. Declaring one file still does not
expose its siblings.

The declared path is also pinned to its real path at construction, so a
later chdir cannot silently repoint it at a different file — previously a
relative constructor path re-resolved against the new working directory
on every call.

Also guards the writer's filepath resolution, which could raise
ValueError out of _run for a filename containing a null byte, breaking
the contract of always returning a descriptive string. The directory and
read paths were already guarded.

Adds docstrings to strtobool and both _run methods, corrects an Arabic
tanween spelling and a kaf-as-descriptor calque in the localized read
docs, and regenerates tool.specs.json for the schema change.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 28, 2026 08:33
@joaomdmoura

Copy link
Copy Markdown
Collaborator Author

Addressed the review feedback in 3c8bdb2. Summary of what changed and what I deliberately did not change.

Fixed

  • Constructor path unusable by agents (Cursor Bugbot, high) — real, and worse than reported: file_path was a required schema field, so the long-documented "call with no arguments to read the default file" raised a validation error, and the label advertised in the description resolved to the wrong place. file_path is now optional and the declared file is addressable by that label as well as by its real path. Declaring one file still does not expose its siblings.
  • Relative constructor path + chdir (Copilot) — the trusted target followed the working directory. It is now pinned to its real path at construction.
  • Unguarded resolution (CodeRabbit) — a null byte in filename raised ValueError out of _run while directory and the read path were already guarded. Now consistent.
  • Arabic tanween (CodeRabbit) — corrected, including in the sentence this PR added, along with the kaf calque flagged next to it.
  • Docstring coverage (pre-merge warning) — added docstrings to strtobool and both _run methods.

Also regenerated tool.specs.json for the schema change, so no follow-up bot commit is needed.

Tests are now 69 (from 24 on main), all passing, with ruff and strict mypy clean.

Not changed

  • Localized sandbox docs (CodeRabbit nitpick) — correct that ar/ko/pt-BR do not document base_dir, encoding, or the sandbox. I fixed the code snippets and factual errors in every locale, but deliberately left the new prose English-only rather than hand-rolling Arabic and Korean technical writing. Worth a follow-up from whoever owns translations. Note the pre-existing UTF-8 claims in those locales became true as a side effect of the encoding fix.
  • line_count=0 reads the whole file — pre-existing, out of scope for this PR by request; the early-stop rewrite preserves the existing behavior rather than silently changing it.

CI

The tests matrix failure on the first run was flaky — a re-run of the same commit passed with zero failed jobs. Every visible group had passed and the jobs were fail-fast cancellations, with no FAILED line anywhere in the logs.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py:33

  • strtobool() calls .lower() on any non-bool input, so passing a non-string (e.g. overwrite=1 or overwrite=None via direct _run() calls) will raise AttributeError and escape the except ValueError guard, crashing the tool instead of returning an error string. This also conflicts with the docs/schema text that mentions 1/0 spellings.
def strtobool(val: str | bool) -> bool:
    """Coerce the spellings of true/false an LLM is likely to emit into a bool.

    Args:
        val: A bool, or one of y/yes/t/true/on/1 and n/no/f/false/off/0.

    Returns:
        The corresponding boolean.

    Raises:
        ValueError: If the string is not a recognized boolean spelling.
    """
    if isinstance(val, bool):
        return val
    val = val.lower()
    if val in ("y", "yes", "t", "true", "on", "1"):
        return True
    if val in ("n", "no", "f", "false", "off", "0"):
        return False
    raise ValueError(f"invalid value to cast to bool: {val!r}")

lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py:152

  • line_count = line_count or None treats an explicit line_count=0 the same as None, which will read from start_line to EOF rather than reading 0 lines. Using an explicit is None check preserves the intended meaning of 0.
        """Read a file, or a window of its lines, as text."""
        start_line = start_line or 1
        line_count = line_count or None

lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py:178

  • When line_count is explicitly 0 (after preserving it), islice(file, start_idx, start_idx) correctly yields no lines, but the current not selected_lines and start_idx > 0 check will incorrectly report that start_line exceeds the file length. This should only error when the caller actually requested at least 1 line.
                if not selected_lines and start_idx > 0:
                    return f"Error: Start line {start_line} exceeds the number of lines in the file."

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@docs/edge/en/tools/file-document/filereadtool.mdx`:
- Line 71: Update the file_path behavior description in the file-read tool
documentation to say constructor-declared paths are always allowed rather than
always readable, while preserving the explanation that they bypass base_dir
containment checks and may still fail during reading.
🪄 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: 178bb623-b020-4229-a4e4-e7b5724873be

📥 Commits

Reviewing files that changed from the base of the PR and between 64baf5c and 3c8bdb2.

📒 Files selected for processing (8)
  • docs/edge/ar/tools/file-document/filereadtool.mdx
  • docs/edge/en/tools/file-document/filereadtool.mdx
  • lib/crewai-tools/src/crewai_tools/tools/file_read_tool/README.md
  • lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py
  • lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py
  • lib/crewai-tools/tests/file_read_tool_test.py
  • lib/crewai-tools/tests/tools/test_file_writer_tool.py
  • lib/crewai-tools/tool.specs.json
🚧 Files skipped from review as they are similar to previous changes (6)
  • lib/crewai-tools/src/crewai_tools/tools/file_read_tool/README.md
  • lib/crewai-tools/tool.specs.json
  • lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py
  • lib/crewai-tools/tests/tools/test_file_writer_tool.py
  • lib/crewai-tools/tests/file_read_tool_test.py
  • lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py

Comment thread docs/edge/en/tools/file-document/filereadtool.mdx Outdated
Addresses the second round of review feedback on #6692.

The previous commit pinned a relative constructor file_path with
os.path.realpath, which anchors to the working directory, while both
format_path_for_display and validate_file_path anchor a relative path to
base_dir. With the two roots disagreeing, the same relative string meant
two different files — and the tool served the cwd one under a label that
looks like it belongs to the sandbox:

    FileReadTool(file_path="data.txt", base_dir="/allowed")   # cwd=/work
    label advertised to the model -> "data.txt"
    run(file_path="data.txt")     -> contents of /work/data.txt

That reads a file from outside base_dir, so it was a sandbox escape
introduced by the exemption itself, not just a wrong-file bug.

Resolution now goes through a single _resolve_against_base helper that
anchors relative paths exactly the way the sandbox does, so the pinned
path, the advertised label and the containment check all agree. Covered
by test_relative_declared_path_anchors_to_base_dir.

Also softens "always readable" to "always allowed past the containment
check" in the docstring, README and docs, since bypassing containment
does not guarantee the read succeeds — it can still fail on a missing
file, a directory, or permissions.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py:95

  • description still implies file_path is always required and does not mention the new base_dir sandboxing/encoding behavior. This is user- and LLM-facing metadata, so it should reflect the updated API and security constraints.
    name: str = "Read a file's content"
    description: str = "A tool that reads the content of a file. To use this tool, provide a 'file_path' parameter with the path to the file you want to read. Optionally, provide 'start_line' to start reading from a specific line and 'line_count' to limit the number of lines read."
    args_schema: type[BaseModel] = FileReadToolSchema

lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py:85

  • FileWriterTool.description is still the pre-sandbox wording, so it doesn’t communicate that writes are confined to base_dir (cwd by default) or that encoding is configurable. This string is used as tool metadata (and is reflected in tool.specs.json), so it should be updated to match the new security model.
    name: str = "File Writer Tool"
    description: str = "A tool to write content to a specified file. Accepts filename, content, and optionally a directory path and overwrite flag as input."
    args_schema: type[BaseModel] = FileWriterToolInput

lib/crewai-tools/tests/tools/test_file_writer_tool.py:114

  • This docstring contradicts the assertion below: tool.run(...) does raise a ValueError on schema validation failure (as the test expects). Updating the docstring avoids confusion for future maintainers.
def test_missing_required_fields_via_run(tool, temp_env):
    """The public entry point reports schema violations instead of raising."""
    with pytest.raises(ValueError, match="validation failed"):

Addresses the low-confidence notes from the Copilot review on #6692.

Both tools' descriptions were pre-sandbox wording, so the model learned
about containment only by attempting a path and reading the error back.
Both now state that access is confined to the tool's allowed directory
and that a path resolving outside it is rejected.

The wording deliberately says "the tool's allowed directory" rather than
"the working directory", because the root is base_dir when one is set,
and naming the absolute root would leak it into the prompt — the same
reason paths are redacted in errors.

Not changed: the notes also suggested advertising `encoding`. That is a
constructor-only field the model cannot set, so describing it to the LLM
would be misleading.

Also fixes a test docstring that contradicted its own assertion — the
public run() path does raise on schema validation failure, which is what
the test asserts.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 28, 2026 08:49
@joaomdmoura

Copy link
Copy Markdown
Collaborator Author

Second review round addressed in 7f35b76 and f42cb2d.

Fixed

  • Relative declared path pinned against the wrong root (Cursor Bugbot, medium) — real, and it was a sandbox escape rather than just a wrong-file bug. I pinned with os.path.realpath (cwd-anchored) while format_path_for_display and validate_file_path both anchor relative paths to base_dir, so with FileReadTool(file_path="data.txt", base_dir="/allowed") and cwd /work, the label advertised as data.txt served /work/data.txt — a file outside the sandbox. All three now resolve through one _resolve_against_base helper, so the pin, the label, and the containment check agree. Regression test added.
  • "always readable" overclaimed (CodeRabbit) — now "always allowed past the containment check", with the failure modes named, in the docstring, README, and docs.
  • Tool descriptions were pre-sandbox wording (Copilot, low confidence but valid) — the model previously discovered containment only by trying a path and reading the error. Both descriptions now state that access is confined to the tool's allowed directory. Phrased as "the tool's allowed directory" rather than "the working directory", since the root is base_dir when set, and naming the absolute root would leak it into the prompt.
  • Self-contradicting test docstring (Copilot) — mine; run() does raise on schema validation failure, which is what the test asserts.

Not changed

  • Advertising encoding in the LLM-facing description — it is a constructor-only field the model cannot set, so describing it would mislead.
  • FileReadTool.description "implies file_path is always required" — for a tool built without a default, it is required, and when a default exists the description is replaced with one that says so. Accurate as written.
  • Localized sandbox prose, and line_count=0, as noted above.

CI is green (28 passing) and 70 tests pass locally.

Reviewer note

Two consecutive rounds found real defects in the same small piece of code — the constructor-path exemption — both stemming from three different notions of "resolve this path" that had to agree and didn't. That is now centralized, but it is the part of this PR I would most want a careful human read on. The write sandbox itself has held up unchanged across both rounds.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (3)

lib/crewai-tools/tool.specs.json:10146

  • The directory field description says it defaults to the current working directory, but when base_dir is set the default "./" resolves under base_dir (not under the process CWD). This mismatch can mislead the model about where writes will land.
            "description": "Directory to write the file into. Created if it does not exist. Defaults to the current working directory.",

lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/README.md:36

  • This says directory defaults to the current working directory, but when base_dir is set the tool resolves the default ./ under base_dir (not the process CWD). Updating this avoids documenting behavior that becomes wrong as soon as base_dir is used.
- `directory` (optional): The path to the directory where the file will be created. Defaults to the current working directory. If the directory does not exist, it will be created.

docs/edge/en/tools/file-document/filewritetool.mdx:49

  • This says directory defaults to the current working directory, but when base_dir is set the tool resolves the default ./ under base_dir (not the process CWD). Updating this avoids documenting behavior that becomes wrong as soon as base_dir is used.
- `directory` (optional): The path to the directory where the file will be created. Defaults to the current working directory. If the directory does not exist, it will be created.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit f42cb2d. Configure here.

Addresses the third review round on #6692.

Both remaining findings came from the same habit: storing an unanchored
string and re-resolving it later.

A relative base_dir was kept verbatim and re-resolved against getcwd() on
every call, while the declared file was pinned once at construction. After
a chdir the sandbox root moved but the declared default did not, so one
tool applied two different roots. base_dir is now resolved once — in the
reader's __init__, and via a field_validator on the writer so it also
applies on the model_validate path.

That also covers the serialization concern. model_dump drops the private
pin, and __init__ re-runs on restore, so a relative file_path was
re-anchored against whatever the working directory happened to be at load
time. With base_dir anchored, restore rebuilds the identical pin.

The residual case is a relative file_path with no base_dir, where the
sandbox root is the working directory too — so both move together and the
tool stays self-consistent. Covered by
test_declared_path_survives_a_serialization_round_trip and
test_relative_base_dir_is_anchored_at_construction on both tools.

Also corrects the writer's 'directory' description, README and docs: the
default resolves inside the tool's allowed directory, which is base_dir
when one is set, not always the working directory.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 28, 2026 08:59
@joaomdmoura

Copy link
Copy Markdown
Collaborator Author

Third review round addressed in 81bed00.

Fixed

Both Cursor findings had the same root cause — an unanchored string stored and re-resolved later.

  • Relative base_dir follows chdir (low) — base_dir was kept verbatim and re-resolved via getcwd() per call, while the declared file was pinned once, so after a chdir the tool applied two different roots. It is now resolved once: in __init__ for the reader, and via a field_validator on the writer so it also covers the model_validate path, which bypasses assignment-time normalization.
  • Pinned path lost on restore (medium) — the mechanism is confirmed, with one correction: model_validate does re-run __init__, so the pin is recomputed rather than silently left as None. I checked, because the latter would have been worse. With base_dir now anchored, model_dump carries an absolute root and restore rebuilds an identical pin.
  • directory default documented wrongly (Copilot) — it said "defaults to the current working directory", but a relative directory resolves inside base_dir when one is set. Corrected in the field description (so the model sees it), the README, and the English doc.
  • Self-contradicting test docstring (Copilot) — mine, fixed.

Not changed

  • A relative file_path with no base_dir still re-anchors on restore. There the sandbox root is the working directory too, so both move together and the tool stays self-consistent — that is the reasonable reading of "relative path, no explicit root". Fully closing it would mean normalising the public file_path field, which would break its existing contract (tool.file_path returns what the caller passed).
  • Advertising encoding to the LLM: constructor-only, the model cannot set it.
  • Localized sandbox prose, and line_count=0, as previously noted.

74 tests pass, ruff and strict mypy clean, tool.specs.json regenerated in-commit.

Where this PR stands

Three review rounds, ten findings addressed, five declined with reasons. Every round found something real in the constructor-path exemption and its interaction with base_dir — first the exemption not working for agent calls, then a sandbox escape from mismatched resolution roots, now root anchoring across chdir and serialization. Path resolution is now funnelled through a single _resolve_against_base helper with base_dir anchored once, which should close that class of bug. The write sandbox itself has needed no change since the first commit. I would still want a human eye on the exemption logic specifically before merge.

@mintlify

mintlify Bot commented Jul 28, 2026

Copy link
Copy Markdown

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
crewai 🟡 Building Jul 28, 2026, 8:08 AM

💡 Tip: Enable Workflows to automatically generate PRs for you.

@joaomdmoura

Copy link
Copy Markdown
Collaborator Author

Two follow-ups on the open threads.

Copilot's base_dir anchoring comments (one per tool) were the same issue Cursor Bugbot raised independently, and were already fixed in 81bed00 — including the specific suggestion to use the pinned value for format_path_for_display() and _declared_realpath, which is what the reader now does. Replied on both threads. Every inline thread on this PR now has a response.

Docstring Coverage pre-merge warning (59.09% vs an 80% threshold) — not planning to chase this, for three reasons:

  • Both production files are already at 100%: file_read_tool.py 6/6 and file_writer_tool.py 5/5 documented.
  • The entire shortfall is in test files, and the repo convention is against the threshold here — across lib/crewai-tools/tests, 249/603 test functions have docstrings (41%). My two files sit at 66% and 40%, i.e. at or above the norm.
  • It is a non-blocking warning; the CodeRabbit check itself passes.

Hitting 80% would mean adding ~25 docstrings to test functions, including pre-existing ones like test_basic_file_write that read fine without them. Happy to do it if a maintainer wants the threshold honoured, but it looks like noise rather than clarity from here.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 15 out of 15 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:109

  • overwrite is documented as accepting 1/0, but _run() can raise an AttributeError if it’s called directly with an int (e.g., overwrite=1) because strtobool() calls .lower() and this block only catches ValueError. This breaks _run()’s “return an error string” contract and contradicts the accepted overwrite spellings.
        try:
            overwrite_file = strtobool(overwrite)
        except ValueError as e:
            return f"An error occurred while writing to the file: {e!s}"

lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py:133

  • The per-instance description injected when file_path is provided says “reads are confined to the tool's allowed directory”, but this tool intentionally allows the constructor-declared default file even when it’s outside base_dir. Rewording to clarify that only runtime paths are confined avoids misleading users/agents about the default-file exemption.
            kwargs["description"] = (
                f"A tool that reads file content. The default file is {display_path}, 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."
            )

@joaomdmoura
joaomdmoura enabled auto-merge (squash) July 28, 2026 09:05
Rejections were echoing validate_file_path's text verbatim, which ends by
advertising CREWAI_TOOLS_ALLOW_UNSAFE_PATHS. That nudges users toward the
bluntest available remedy: it is process-wide and also disables the SSRF
checks on every URL-fetching tool. For these two tools the right answer is
almost always a narrower base_dir.

Adds format_sandbox_error to the security module, which substitutes a
tool-specific remedy for the escape-hatch advice, and uses it in both file
tools. The escape hatch is still documented, just no longer the first thing
a failing agent or developer is told to reach for.

    Before: Path 'x' is outside the allowed directory.
            Set CREWAI_TOOLS_ALLOW_UNSAFE_PATHS=true to bypass this check.
    After:  Path 'x' is outside the allowed directory.
            Pass base_dir to FileWriterTool to allow writing to another
            directory tree.

Other tools that call validate_file_path are unchanged: several have no
base_dir of their own, so their messages keep the existing advice.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 28, 2026 09:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py:182

  • line_count = line_count or None treats an explicit line_count=0 as “read to end of file” (because 0 is falsy). That’s surprising and can accidentally read huge files when the caller intended an empty window. Use explicit None checks instead of truthiness so 0 is preserved.
        start_line = start_line or 1
        line_count = line_count or None

@alex-clawd alex-clawd left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved.

I reproduced the vulnerability on main before assessing the fix, because a security claim is worth confirming rather than trusting:

MAIN:   Content successfully written to pwned.txt
        file created outside cwd: True

Arbitrary write anywhere the process has permission, via an LLM-supplied directory that nothing validated. The asymmetry you describe is the tell — FileWriterTool was the only filesystem tool in the package not routing through validate_file_path, while files_compressor_tool validates even its output path.

Then confirmed the fix on the branch:

escape attempt   -> Error: Invalid directory: ... is outside the allowed directory
                    file created outside: False
normal write     -> Content successfully written to ok.txt
read back        -> hello                      (reader/writer now agree)
base_dir opt-in  -> Content successfully written to allowed.txt
                    file created in opted-in dir: True

That last pair matters: the sandbox closes without making the tool useless, and widening it is a deliberate per-tool decision.

The layering is right, and I checked the part that's easy to get wrong. The claim that filename containment survives CREWAI_TOOLS_ALLOW_UNSAFE_PATHS holds: the env check short-circuits validate_file_path at the top, which is the directory check, but the filename check in _run is a separate is_relative_to comparison that still runs. So #4895's guarantee isn't weakened by the escape hatch — worth having verified rather than assumed, since that's exactly the kind of interaction that quietly reintroduces a hole.

Details I'd have wanted and found:

  • _anchor_base_dir resolves once at validation, so a later chdir can't move the sandbox out from under a long-lived tool
  • is_relative_to on whole components rather than string prefixes — safe on case-insensitive filesystems, and sidesteps the // root edge case, which the comment calls out explicitly
  • resolved_filepath == resolved_directory rejected, so an empty filename isn't treated as a valid target
  • OSError/ValueError around .resolve() for embedded null bytes, and FileExistsError distinguished from other OSErrors with a message that says what's actually wrong
  • per-tool base_dir instead of leaning on the process-wide env var, which would also have disabled SSRF protection on the URL-fetching tools

strtobool on overwrite is a nice touch — an LLM emitting "yes" previously errored on a field it had every reason to think was boolean.

96 passed locally across the writer, reader and safe_path suites. CI 27 pass / 1 skipping.

Worth noting for whoever tracks disclosures: this is an arbitrary-write fix in a shipped tool, so it may deserve a changelog line beyond "rough edges."

@joaomdmoura
joaomdmoura merged commit 2e95bfb into main Jul 28, 2026
59 checks passed
@joaomdmoura
joaomdmoura deleted the fix/file-tools-path-sandbox branch July 28, 2026 09:20
@joaomdmoura

Copy link
Copy Markdown
Collaborator Author

One note for the record, on the line_count = line_count or None observation in the latest Copilot review (raised as a low-confidence note rather than an inline comment).

The observation is correct: an explicit line_count=0 is falsy, so it is treated as "read to the end of the file" rather than an empty window.

It is deliberately out of scope for this PR, not an oversight. It is pre-existing behavior on main, it was identified and reported before this work started, and the decision was to leave it unchanged here so the PR stays scoped to the sandbox and the rough edges around it. The islice rewrite in this PR was written specifically to preserve the existing coercion rather than silently alter it — line_count=0 behaves exactly as it does on main.

Worth fixing separately: the right change is an explicit is None check so 0 means an empty window, which pairs naturally with the fact that the whole-file read path has no size cap.

joaomdmoura added a commit that referenced this pull request Jul 28, 2026
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>
joaomdmoura added a commit that referenced this pull request Aug 3, 2026
…failures

Two review findings, both only reachable once a non-local store is
registered — which is why the local-filesystem tests could not see them.

The reader bound `_store` in `__init__` only. `BaseTool._resolve_tool_dict`
rebuilds a serialized tool with `model_validate`, which skips `__init__`
entirely, so a reconstructed reader came back with `_store` still None and
raised AttributeError on the first read — where before this branch it would
have called `validate_file_path`/`open` directly and worked. Binding moved
to `model_post_init`, which pydantic runs on both paths, and the declared
path, its label and the generated description are derived there too so a
rebuilt reader is indistinguishable from a fresh one. The writer already
did this correctly.

Every store call was guarded for `ValueError` alone, but the protocol
explicitly sanctions `FileStoreError` for failures the local filesystem
cannot have. Such a failure escaped `_run` and aborted the agent's step
instead of returning the error string the tools otherwise always return.
Both tools now wrap the whole operation, which also covers `exists()` and
the store's own `display()` — neither of which had any handler. The
established messages stay on their specific boundaries.

A bare `OSError("...")` still degrades to its type, because
`format_error_for_display` only passes `strerror` through: an OS-populated
OSError renders its absolute filename into `str()`, and #6692 deliberately
closed that. Stores wanting a legible message should raise `FileStoreError`.
Left that helper alone rather than widen a redaction from here.

12 new tests: reconstruction through both `model_validate` and a full
`model_dump` round-trip, a store failing at each of resolve/resolve_within/
display/exists/ensure_parent/open_text/write_text, and the redaction holding
on the new path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UNumDnNbiyw3pv1WakAe6t
joaomdmoura added a commit that referenced this pull request Aug 3, 2026
…failures

Two review findings, both only reachable once a non-local store is
registered — which is why the local-filesystem tests could not see them.

The reader bound `_store` in `__init__` only. `BaseTool._resolve_tool_dict`
rebuilds a serialized tool with `model_validate`, which skips `__init__`
entirely, so a reconstructed reader came back with `_store` still None and
raised AttributeError on the first read — where before this branch it would
have called `validate_file_path`/`open` directly and worked. Binding moved
to `model_post_init`, which pydantic runs on both paths, and the declared
path, its label and the generated description are derived there too so a
rebuilt reader is indistinguishable from a fresh one. The writer already
did this correctly.

Every store call was guarded for `ValueError` alone, but the protocol
explicitly sanctions `FileStoreError` for failures the local filesystem
cannot have. Such a failure escaped `_run` and aborted the agent's step
instead of returning the error string the tools otherwise always return.
Both tools now wrap the whole operation, which also covers `exists()` and
the store's own `display()` — neither of which had any handler. The
established messages stay on their specific boundaries.

A bare `OSError("...")` still degrades to its type, because
`format_error_for_display` only passes `strerror` through: an OS-populated
OSError renders its absolute filename into `str()`, and #6692 deliberately
closed that. Stores wanting a legible message should raise `FileStoreError`.
Left that helper alone rather than widen a redaction from here.

12 new tests: reconstruction through both `model_validate` and a full
`model_dump` round-trip, a store failing at each of resolve/resolve_within/
display/exists/ensure_parent/open_text/write_text, and the redaction holding
on the new path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UNumDnNbiyw3pv1WakAe6t
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants