Skip to content

Commit 3c8bdb2

Browse files
joaomdmouraclaude
andcommitted
fix(tools): make the declared FileReadTool file reachable by agents
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>
1 parent 64baf5c commit 3c8bdb2

8 files changed

Lines changed: 150 additions & 34 deletions

File tree

docs/edge/ar/tools/file-document/filereadtool.mdx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ mode: "wide"
1111
لا نزال نعمل على تحسين الأدوات، لذا قد يحدث سلوك غير متوقع أو تغييرات في المستقبل.
1212
</Note>
1313

14-
تمثل أداة FileReadTool مفهومياً مجموعة من الوظائف ضمن حزمة crewai_tools تهدف إلى تسهيل قراءة الملفات واسترجاع المحتوى. تتضمن هذه المجموعة أدوات لمعالجة ملفات نصية دفعية، وقراءة ملفات التكوين أثناء التشغيل، واستيراد البيانات للتحليلات. تدعم مجموعة متنوعة من صيغ الملفات النصية مثل `.txt` و `.csv` و `.json` وغيرها. يُعاد المحتوى دائماً كنص عادي.
14+
تمثل أداة FileReadTool مفهوميًا مجموعة من الوظائف ضمن حزمة crewai_tools تهدف إلى تسهيل قراءة الملفات واسترجاع المحتوى. تتضمن هذه المجموعة أدوات لمعالجة ملفات نصية دفعية، وقراءة ملفات التكوين أثناء التشغيل، واستيراد البيانات للتحليلات. تدعم مجموعة متنوعة من صيغ الملفات النصية مثل `.txt` و `.csv` و `.json` وغيرها. يُعاد المحتوى دائمًا نصًا عاديًا.
1515

1616
## التثبيت
1717

docs/edge/en/tools/file-document/filereadtool.mdx

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ partial_content = file_read_tool.run(
5353

5454
The agent supplies these at runtime:
5555

56-
- `file_path`: The path to the file you want to read. Accepts absolute and relative paths. Ensure the file exists and you have the necessary permissions to access it.
56+
- `file_path`: (Optional) The path to the file you want to read. Accepts absolute and relative paths. Ensure the file exists and you have the necessary permissions to access it. Omit it to read the default file configured at construction; if there is no default, the tool reports that no path was provided.
5757
- `start_line`: (Optional) The line number to start reading from (1-indexed). Defaults to `1`.
5858
- `line_count`: (Optional) The number of lines to read. If omitted, reads from `start_line` to the end of the file.
5959

@@ -68,7 +68,7 @@ You set these when constructing the tool:
6868
Because the file path is usually chosen by an LLM at runtime, reads are confined to a sandbox:
6969

7070
- Paths supplied at runtime must resolve inside `base_dir`, which defaults to the current working directory. `..` segments and symlinks are resolved before the check, so they cannot be used to escape.
71-
- A `file_path` passed to the constructor is developer-declared intent, so it is always readable — even outside `base_dir`. Declaring one file does not expose its siblings.
71+
- A `file_path` passed to the constructor is developer-declared intent, so it is always readable — even outside `base_dir`. It is pinned when the tool is built, so a later change of working directory cannot repoint it, and the agent can address it either by omitting `file_path` or by using the name shown in the tool's description. Declaring one file does not expose its siblings.
7272

7373
To let an agent read a directory tree outside the working directory, point `base_dir` at it:
7474

lib/crewai-tools/src/crewai_tools/tools/file_read_tool/README.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ partial_content = file_read_tool.run(file_path='path/to/your/file.txt', start_li
3737

3838
The agent supplies these at runtime:
3939

40-
- `file_path`: The path to the file you want to read. It accepts both absolute and relative paths. Ensure the file exists and you have the necessary permissions to access it.
40+
- `file_path`: (Optional) The path to the file you want to read. It accepts both absolute and relative paths. Ensure the file exists and you have the necessary permissions to access it. Omit it to read the default file configured at construction; if there is no default, the tool reports that no path was provided.
4141
- `start_line`: (Optional) The line number to start reading from (1-indexed). Defaults to 1 (the first line).
4242
- `line_count`: (Optional) The number of lines to read. If not provided, reads from the start_line to the end of the file.
4343

@@ -52,7 +52,7 @@ You set these when constructing the tool:
5252
Because the file path is usually chosen by an LLM at runtime, reads are confined to a sandbox:
5353

5454
- Paths supplied at runtime must resolve inside `base_dir` (the current working directory by default). `..` segments and symlinks are resolved before the check, so they cannot be used to escape.
55-
- A `file_path` passed to the constructor is developer-declared intent, so it is always readable — even outside `base_dir`. Declaring one file does not expose its siblings.
55+
- A `file_path` passed to the constructor is developer-declared intent, so it is always readable — even outside `base_dir`. It is pinned when the tool is built, so a later change of working directory cannot repoint it, and the agent can address it either by omitting `file_path` or by using the name shown in the tool's description. Declaring one file does not expose its siblings.
5656

5757
To let an agent read a directory tree outside the working directory, point `base_dir` at it:
5858

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

Lines changed: 47 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
from typing import Any
44

55
from crewai.tools import BaseTool
6-
from pydantic import BaseModel, Field
6+
from pydantic import BaseModel, Field, PrivateAttr
77

88
from crewai_tools.security.safe_path import (
99
format_error_for_display,
@@ -15,7 +15,13 @@
1515
class FileReadToolSchema(BaseModel):
1616
"""Input for FileReadTool."""
1717

18-
file_path: str = Field(..., description="Mandatory file full path to read the file")
18+
file_path: str | None = Field(
19+
None,
20+
description=(
21+
"Full path of the file to read. Omit it to read the tool's default "
22+
"file, which only works when one was configured."
23+
),
24+
)
1925
start_line: int | None = Field(
2026
1, description="Line number to start reading from (1-indexed)"
2127
)
@@ -28,9 +34,10 @@ class FileReadTool(BaseTool):
2834
"""A tool for reading file contents.
2935
3036
This tool inherits its schema handling from BaseTool to avoid recursive schema
31-
definition issues. The args_schema is set to FileReadToolSchema which defines
32-
the required file_path parameter. The schema should not be overridden in the
33-
constructor as it would break the inheritance chain and cause infinite loops.
37+
definition issues. The args_schema is set to FileReadToolSchema, whose
38+
file_path parameter is optional so the tool's default file can be read by
39+
omitting it. The schema should not be overridden in the constructor as it
40+
would break the inheritance chain and cause infinite loops.
3441
3542
The tool supports two ways of specifying the file path:
3643
1. At construction time via the file_path parameter
@@ -39,7 +46,9 @@ class FileReadTool(BaseTool):
3946
Paths supplied at runtime must resolve inside ``base_dir`` (the current
4047
working directory by default), since they are typically chosen by an LLM.
4148
A ``file_path`` given at construction time is developer-declared intent and
42-
is always readable, even when it lives outside ``base_dir``.
49+
is always readable, even when it lives outside ``base_dir``. It is pinned at
50+
construction, so a later chdir cannot repoint it, and it can be addressed
51+
either by omitting ``file_path`` or by the label shown in the description.
4352
4453
Args:
4554
file_path (Optional[str]): Path to the file to be read. If provided,
@@ -67,6 +76,12 @@ class FileReadTool(BaseTool):
6776
base_dir: str | None = None
6877
encoding: str = "utf-8"
6978

79+
# Pinned at construction so a later chdir cannot change which file the
80+
# developer-declared default refers to.
81+
_declared_realpath: str | None = PrivateAttr(default=None)
82+
# The label the tool's description shows the LLM for the declared file.
83+
_declared_label: str | None = PrivateAttr(default=None)
84+
7085
def __init__(
7186
self,
7287
file_path: str | None = None,
@@ -84,24 +99,31 @@ def __init__(
8499
encoding (str): Text encoding used to decode the file.
85100
**kwargs: Additional keyword arguments passed to BaseTool.
86101
"""
102+
display_path = None
87103
if file_path is not None:
88104
display_path = format_path_for_display(file_path, base_dir)
89105
kwargs["description"] = (
90-
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."
106+
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, and specify 'start_line' and 'line_count' to read specific parts of the file."
91107
)
92108

93109
super().__init__(**kwargs)
94110
self.file_path = file_path
95111
self.base_dir = base_dir
96112
self.encoding = encoding
113+
self._declared_realpath = (
114+
os.path.realpath(file_path) if file_path is not None else None
115+
)
116+
self._declared_label = display_path
97117

98118
def _resolve_path(self, file_path: str) -> str:
99119
"""Resolve *file_path* and confirm the tool is allowed to read it.
100120
101-
The path declared at construction time is always allowed: the developer
102-
named that exact file, and the tool would read it anyway when called
103-
with no arguments. Everything else — including any path an LLM picks at
104-
runtime — must resolve inside ``base_dir``.
121+
The file declared at construction time is always allowed: the developer
122+
named it, and the tool reads it anyway when ``file_path`` is omitted. It
123+
is addressable both by its real path and by the label the description
124+
shows the LLM, since that label is all the model is given. Everything
125+
else — including any path an LLM invents at runtime — must resolve
126+
inside ``base_dir``.
105127
106128
Args:
107129
file_path: The path to resolve.
@@ -112,9 +134,11 @@ def _resolve_path(self, file_path: str) -> str:
112134
Raises:
113135
ValueError: If the path resolves outside ``base_dir``.
114136
"""
115-
resolved = os.path.realpath(file_path)
116-
if self.file_path is not None and resolved == os.path.realpath(self.file_path):
117-
return resolved
137+
declared = self._declared_realpath
138+
if declared is not None and (
139+
file_path == self._declared_label or os.path.realpath(file_path) == declared
140+
):
141+
return declared
118142
return validate_file_path(file_path, self.base_dir)
119143

120144
def _run(
@@ -123,17 +147,19 @@ def _run(
123147
start_line: int | None = 1,
124148
line_count: int | None = None,
125149
) -> str:
126-
file_path = file_path or self.file_path
150+
"""Read a file, or a window of its lines, as text."""
127151
start_line = start_line or 1
128152
line_count = line_count or None
129153

130154
if file_path is None:
131-
return "Error: No file path provided. Please provide a file path either in the constructor or as an argument."
132-
133-
try:
134-
file_path = self._resolve_path(file_path)
135-
except ValueError as e:
136-
return f"Error: Invalid file path: {e!s}"
155+
if self._declared_realpath is None:
156+
return "Error: No file path provided. Please provide a file path either in the constructor or as an argument."
157+
file_path = self._declared_realpath
158+
else:
159+
try:
160+
file_path = self._resolve_path(file_path)
161+
except ValueError as e:
162+
return f"Error: Invalid file path: {e!s}"
137163

138164
display_path = format_path_for_display(file_path, self.base_dir)
139165
try:

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

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,17 @@
1212

1313

1414
def strtobool(val: str | bool) -> bool:
15+
"""Coerce the spellings of true/false an LLM is likely to emit into a bool.
16+
17+
Args:
18+
val: A bool, or one of y/yes/t/true/on/1 and n/no/f/false/off/0.
19+
20+
Returns:
21+
The corresponding boolean.
22+
23+
Raises:
24+
ValueError: If the string is not a recognized boolean spelling.
25+
"""
1526
if isinstance(val, bool):
1627
return val
1728
val = val.lower()
@@ -82,6 +93,7 @@ def _run(
8293
directory: str | None = "./",
8394
overwrite: str | bool = False,
8495
) -> str:
96+
"""Write *content* to *filename*, confined to the tool's sandbox."""
8597
directory = directory or "./"
8698

8799
try:
@@ -102,7 +114,14 @@ def _run(
102114
# components, so it is safe on case-insensitive filesystems and avoids
103115
# the "//" prefix edge case. A filepath that resolves to the directory
104116
# itself (e.g. an empty filename) is not a valid file target.
105-
resolved_filepath = Path(os.path.join(resolved_directory, filename)).resolve()
117+
try:
118+
resolved_filepath = Path(
119+
os.path.join(resolved_directory, filename)
120+
).resolve()
121+
except (OSError, ValueError) as e:
122+
# e.g. an embedded null byte, which trips the underlying syscall.
123+
return f"Error: Invalid file path: {format_error_for_display(e)}"
124+
106125
display_filepath = format_path_for_display(
107126
str(resolved_filepath), str(resolved_directory)
108127
)

lib/crewai-tools/tests/file_read_tool_test.py

Lines changed: 53 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -230,6 +230,51 @@ def test_constructor_path_outside_working_directory_is_readable(tmp_path, monkey
230230
assert tool._run(file_path=str(target)) == "declared content"
231231

232232

233+
def test_declared_file_is_reachable_the_way_an_agent_calls_it(tmp_path, monkeypatch):
234+
"""The label in the description must resolve to the declared file.
235+
236+
The description only shows a redacted label, so that label is all the model
237+
has to work with. It has to address the declared file.
238+
"""
239+
workspace = tmp_path / "workspace"
240+
workspace.mkdir()
241+
monkeypatch.chdir(workspace)
242+
target = tmp_path / "declared.txt"
243+
target.write_text("declared content")
244+
245+
tool = FileReadTool(file_path=str(target))
246+
label = tool._declared_label
247+
248+
assert label == "declared.txt"
249+
assert label in tool.description
250+
# Omitting file_path entirely, and passing the advertised label, both work.
251+
assert tool.run() == "declared content"
252+
assert tool.run(file_path=label) == "declared content"
253+
254+
255+
def test_run_without_file_path_reports_error_when_no_default(tmp_path, monkeypatch):
256+
"""file_path is optional in the schema, so this must not raise."""
257+
monkeypatch.chdir(tmp_path)
258+
259+
assert "Error: No file path provided" in FileReadTool().run()
260+
261+
262+
def test_declared_relative_path_survives_chdir(tmp_path, monkeypatch):
263+
"""The declared file is pinned at construction, not re-resolved per call."""
264+
monkeypatch.chdir(tmp_path)
265+
(tmp_path / "rel.txt").write_text("original")
266+
nested = tmp_path / "sub"
267+
nested.mkdir()
268+
(nested / "rel.txt").write_text("a different file")
269+
270+
tool = FileReadTool(file_path="rel.txt")
271+
assert tool._run() == "original"
272+
273+
monkeypatch.chdir(nested)
274+
assert tool._run() == "original"
275+
assert tool._run(file_path="rel.txt") == "original"
276+
277+
233278
def test_constructor_path_does_not_widen_the_sandbox(tmp_path, monkeypatch):
234279
"""Declaring one file must not expose its siblings to the LLM."""
235280
workspace = tmp_path / "workspace"
@@ -315,6 +360,14 @@ def test_encoding_is_configurable(tmp_path, monkeypatch):
315360
assert FileReadTool(encoding="latin-1")._run(file_path="latin.txt") == "café"
316361

317362

363+
def test_null_byte_path_returns_error_instead_of_raising(tmp_path, monkeypatch):
364+
monkeypatch.chdir(tmp_path)
365+
366+
result = FileReadTool()._run(file_path="a\x00b.txt")
367+
368+
assert "Error" in result
369+
370+
318371
def test_decode_error_names_the_encoding(tmp_path, monkeypatch):
319372
monkeypatch.chdir(tmp_path)
320373
(tmp_path / "binary.bin").write_bytes(bytes(range(256)))

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -368,6 +368,19 @@ def test_directory_that_is_an_existing_file(tool, temp_env):
368368
assert "overwrite" not in result
369369

370370

371+
@pytest.mark.parametrize(
372+
("filename", "directory"),
373+
[("a\x00b.txt", "./"), ("ok.txt", "d\x00ir")],
374+
)
375+
def test_null_byte_returns_error_instead_of_raising(tool, filename, directory):
376+
"""_run's contract is to return a descriptive string for bad input."""
377+
result = tool._run(
378+
filename=filename, directory=directory, content="x", overwrite=True
379+
)
380+
381+
assert "Error" in result
382+
383+
371384
def test_writes_utf8_by_default(tool, temp_env):
372385
content = "café — 日本語 — 🚀"
373386
tool._run(filename=temp_env["test_file"], content=content, overwrite=True)

0 commit comments

Comments
 (0)