Compare commits

...

8 Commits

Author SHA1 Message Date
João Moura
c870f96f30 Merge branch 'main' into feat/file-tools-pluggable-store 2026-08-04 12:58:37 -03:00
João Moura
2018e950b6 Merge branch 'main' into feat/file-tools-pluggable-store 2026-08-04 10:58:54 -03:00
Joao Moura
73d16ac6ba fix(tools): reset the description when the declared-file pin is lost
Bugbot caught a hole in the previous commit's own fix. Guarding the declared
-file derivation clears `_declared_realpath` and `_declared_label`, but
`description` is a serialized *field* — on a `model_validate` rebuild it
arrives already saying "The default file is notes.txt, which is read when
'file_path' is omitted".

So a rebuilt reader that lost its pin still advertised a default it could not
read. The LLM would take the tool at its word, call it with no arguments, and
get "No file path provided" — the tool lying about its own contract, which is
worse than the missing default it was meant to degrade to.

The failure path now restores the class-default description alongside clearing
the pin, so what the tool says and what it does stay matched.

105 tests across the file tools; the new one asserts a real dumped description
advertises the default, and that after a rebuild losing the pin it does not,
matches the class default, and reports "No file path provided" on a bare call.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UNumDnNbiyw3pv1WakAe6t
2026-08-03 19:01:37 -07:00
Joao Moura
953e1b94ab fix(tools): make construction-time store failures non-fatal where they can be
Copilot flagged that `model_post_init` calls `store.normalize` / `store.display`
unguarded, so a store raising there stops a serialized crew from loading —
against this PR's own claim that a broken integration degrades rather than
taking file I/O down.

It suggested falling back to the local store. Not doing that: silently
redirecting a crew configured for durable storage onto a disk that will be
discarded trades a loud failure for quiet data loss, which is the exact bug
this PR exists to fix.

Three changes instead.

The protocol now *states* the invariant it only implied. `normalize` and
`display` must be pure string computation — no I/O, no raising — because the
tools call them while a tool is being constructed. Both real stores already
comply (`CdoFileStore` does `posixpath` arithmetic and never touches its
client); this makes it a contract a new store is held to rather than a
coincidence.

The reader's declared-file derivation is now guarded, because a crew that
cannot load over a *convenience default filename* is indefensible. It comes
back without a default file and logs; any real problem resurfaces on the
first read, where `_run` already reports it.

`base_dir` anchoring stays unguarded, deliberately, and now says why:
it is a containment guarantee, not a convenience. Leaving the root relative
because a store hiccuped would let a later chdir move the sandbox — handing
back a weaker sandbox than the caller asked for, silently. That failure
should surface.

104 tests across the file tools. Two new, one per half of the asymmetry.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UNumDnNbiyw3pv1WakAe6t
2026-08-03 15:40:48 -07:00
github-actions[bot]
1b198531cd chore: update tool specifications 2026-08-03 22:29:56 +00:00
Joao Moura
1c252e2d83 docs(tools): state what the declared-path pin means across a rebuild
Bugbot flagged that `model_post_init` re-derives `_declared_realpath` from
the serialized `file_path`, so a rebuild in a different working directory can
repoint the declared default. The mechanism is real. Traced it to exactly one
case of three:

  absolute file_path              -> survives a rebuild anywhere
  relative file_path + base_dir   -> survives; base_dir is anchored already
  relative file_path, no base_dir -> re-anchors to the rebuilding cwd

Keeping the re-anchor, deliberately. A bare relative path names nothing
absolute to preserve, and the alternative is pinning a directory that, for a
rebuild in a fresh container, no longer exists — reading a stale absolute path
would be the worse failure. It is also not a regression in any case: before
this branch a rebuilt reader lost the declared file outright and answered "No
file path provided".

Rewriting `file_path` to its resolved form at construction would close it,
but `tests/agents/test_agent.py:2311` pins that the authored string survives,
so that is a public-contract change rather than a fix. A serialized pin field
would too, at the cost of a schema change — noted on the thread for whoever
reviews, not taken unilaterally.

So: all three cases now have a test, and the class docstring says which is
which, so the behavior is a decision rather than something a reader has to
infer. 38 tests in the seam suite, 338 across the file tools and crewai's
tool suite.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UNumDnNbiyw3pv1WakAe6t
2026-08-03 15:28:23 -07:00
Joao Moura
b306793f1c fix(tools): bind the reader's store in model_post_init, report store 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
2026-08-03 15:28:23 -07:00
Joao Moura
77fb6bdb69 feat(tools): make the file tools' backing store pluggable
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>
2026-08-03 15:28:23 -07:00
8 changed files with 1133 additions and 83 deletions

View File

@@ -0,0 +1,30 @@
"""Pluggable backing store for :class:`FileReadTool` / :class:`FileWriterTool`.
The tools default to :class:`LocalFileStore`, which reads and writes the
local filesystem exactly as they always have. A deployment environment where
the local disk is ephemeral can register a different store, so the same tools
persist somewhere durable without the agent, the crew definition, or the tool
arguments changing.
A store owns its own containment. ``resolve`` and ``resolve_within`` must
reject any path the caller should not reach, because the tools call nothing
else before doing I/O.
"""
from crewai_tools.file_storage.base import FileStore, FileStoreError
from crewai_tools.file_storage.local import LocalFileStore
from crewai_tools.file_storage.registry import (
register_file_store_factory,
reset_file_store_factory,
resolve_file_store,
)
__all__ = [
"FileStore",
"FileStoreError",
"LocalFileStore",
"register_file_store_factory",
"reset_file_store_factory",
"resolve_file_store",
]

View File

@@ -0,0 +1,113 @@
"""The store protocol the file tools are written against."""
from __future__ import annotations
from contextlib import AbstractContextManager
from typing import Protocol, TextIO, runtime_checkable
class FileStoreError(Exception):
"""A store failed for a reason with no stdlib exception that fits.
Stores should prefer the built-in filesystem exceptions where one
applies — ``FileNotFoundError``, ``PermissionError``,
``IsADirectoryError``, ``FileExistsError`` — because the tools already
translate those into their established messages. Raise this only for
failures specific to the backing service, such as an unreachable
endpoint or a size limit the local filesystem does not have.
"""
@runtime_checkable
class FileStore(Protocol):
"""Where :class:`FileReadTool` and :class:`FileWriterTool` do their I/O.
Paths crossing this boundary are *store paths*: whatever ``resolve``
returned. For the local store those are absolute filesystem paths; for a
remote store they may be keys or workspace-relative paths. The tools
never interpret them, they only pass them back.
"""
#: Short human-readable name, used in error messages so a failure makes
#: clear which store produced it (e.g. ``"local filesystem"``).
label: str
def resolve(self, path: str, base_dir: str | None = None) -> str:
"""Normalize *path* and confirm the caller may touch it.
Args:
path: The caller-supplied path, absolute or relative.
base_dir: Optional containment root supplied by the tool.
Returns:
The store path to use for subsequent calls.
Raises:
ValueError: If the path falls outside what the store allows.
"""
def normalize(self, path: str, base_dir: str | None = None) -> str:
"""Normalize *path* for identity comparison, without containment.
:class:`FileReadTool` uses this to pin the file declared at
construction, so it can recognize that path again later even if the
working directory has since moved. Unlike :meth:`resolve` it never
rejects: a path outside the sandbox still has a canonical form.
Must be a pure computation on the string: no I/O, no network, and no
raising. The tools call it while a tool is being constructed —
including when pydantic rebuilds a serialized crew — so a store that
reaches its backing service here turns a transient outage into a crew
that cannot load at all. Defer anything that can fail to
:meth:`resolve`, :meth:`open_text` or :meth:`write_text`, where the
tools report failures instead of propagating them.
"""
def resolve_within(self, directory: str, filename: str) -> str:
"""Join *filename* under the already-resolved *directory*.
Kept separate from :meth:`resolve` because the writer applies two
levels of containment: the directory must be inside the store's
sandbox, and the filename must then stay inside that directory.
Raises:
ValueError: If *filename* escapes *directory*, or names the
directory itself.
"""
def display(self, resolved: str, base: str | None = None) -> str:
"""Return a label for *resolved* that is safe to show an LLM.
Must not leak absolute directory prefixes; the tools put the result
straight into agent-visible output. Pure and non-raising for the same
reason as :meth:`normalize` — the reader labels its declared file at
construction time.
"""
def exists(self, resolved: str) -> bool:
"""Whether something already lives at *resolved*."""
def ensure_parent(self, resolved: str) -> None:
"""Create the container *resolved* will live in, if it needs one.
Raises:
FileExistsError: If a non-container already occupies that name.
"""
def open_text(self, resolved: str, encoding: str) -> AbstractContextManager[TextIO]:
"""Open *resolved* for reading as text.
Returning a file-like object rather than a string keeps the local
store lazy, so reading a small window out of a huge file does not
pull the whole thing into memory. Remote stores that must fetch
eagerly can wrap the payload in ``io.StringIO``.
"""
def write_text(
self, resolved: str, content: str, encoding: str, *, overwrite: bool
) -> None:
"""Write *content* to *resolved*.
Raises:
FileExistsError: If the path exists and *overwrite* is false.
"""

View File

@@ -0,0 +1,88 @@
"""The default store: the local filesystem, sandboxed to a base directory."""
from __future__ import annotations
from contextlib import AbstractContextManager
import os
from pathlib import Path
from typing import TextIO
from crewai_tools.security.safe_path import (
format_error_for_display,
format_path_for_display,
validate_file_path,
)
class LocalFileStore:
"""Reads and writes the local filesystem.
Containment is :func:`validate_file_path`: a resolved path must stay
inside ``base_dir`` (the working directory by default), with symlinks and
``..`` segments resolved first.
"""
label = "local filesystem"
def resolve(self, path: str, base_dir: str | None = None) -> str:
"""Resolve *path*, confining it to *base_dir*."""
return validate_file_path(path, base_dir)
def normalize(self, path: str, base_dir: str | None = None) -> str:
"""Resolve *path* the way the sandbox does, without rejecting it.
``validate_file_path`` and ``format_path_for_display`` both join a
relative path onto *base_dir* rather than the working directory.
Normalization has to agree with them, or the same relative string
would mean two different files.
"""
if os.path.isabs(path):
return os.path.realpath(path)
base = os.path.realpath(base_dir) if base_dir is not None else os.getcwd()
return os.path.realpath(os.path.join(base, path))
def resolve_within(self, directory: str, filename: str) -> str:
"""Join *filename* under *directory*, blocking every escape route.
``..``, absolute paths and symlinks are all resolved before the
check. ``is_relative_to`` compares whole path components, so it is
safe on case-insensitive filesystems and avoids the "//" prefix edge
case. A filename resolving to the directory itself (an empty
filename, say) is not a valid file target.
"""
root = Path(directory)
try:
resolved = Path(os.path.join(directory, filename)).resolve()
except (OSError, ValueError) as exc:
# e.g. an embedded null byte or an over-long name, which trip the
# underlying syscall. str() on an OSError carries the absolute
# filename, and the tools put this message straight into
# agent-visible output, so strip it back to the reason.
raise ValueError(format_error_for_display(exc)) from exc
if not resolved.is_relative_to(root) or resolved == root:
raise ValueError("the filename must not escape the target directory")
return str(resolved)
def display(self, resolved: str, base: str | None = None) -> str:
"""Return a path label with absolute prefixes stripped."""
return format_path_for_display(resolved, base)
def exists(self, resolved: str) -> bool:
return os.path.exists(resolved)
def ensure_parent(self, resolved: str) -> None:
"""Create the parent directory, including any missing ancestors."""
os.makedirs(os.path.dirname(resolved) or ".", exist_ok=True)
def open_text(self, resolved: str, encoding: str) -> AbstractContextManager[TextIO]:
return open(resolved, "r", encoding=encoding)
def write_text(
self, resolved: str, content: str, encoding: str, *, overwrite: bool
) -> None:
# "x" makes the create-exclusive check atomic, so an existence race
# surfaces as FileExistsError rather than silently clobbering.
mode = "w" if overwrite else "x"
with open(resolved, mode, encoding=encoding) as handle:
handle.write(content)

View File

@@ -0,0 +1,74 @@
"""How a deployment swaps in a different store.
Kept as a process-wide factory rather than a tool argument on purpose: the
crews that need this are already written and deployed, and the point is that
they keep working unchanged when the runtime is ephemeral. An integration
package registers its factory at import time, and every file tool constructed
afterwards picks it up.
"""
from __future__ import annotations
from collections.abc import Callable
import logging
import threading
from crewai_tools.file_storage.base import FileStore
from crewai_tools.file_storage.local import LocalFileStore
logger = logging.getLogger(__name__)
#: A factory returns the store to use, or ``None`` to decline — which lets an
#: integration arm itself only when its backing service is actually
#: configured, and fall back to the local filesystem everywhere else.
FileStoreFactory = Callable[[], FileStore | None]
_lock = threading.Lock()
_factory: FileStoreFactory | None = None
_local = LocalFileStore()
def register_file_store_factory(factory: FileStoreFactory | None) -> None:
"""Install the factory consulted for every new file tool.
Args:
factory: Callable returning a :class:`FileStore`, or ``None`` to
decline and leave the local filesystem in place. Passing
``None`` as the factory itself unregisters.
"""
global _factory
with _lock:
_factory = factory
def reset_file_store_factory() -> None:
"""Drop any registered factory. Intended for tests."""
register_file_store_factory(None)
def resolve_file_store() -> FileStore:
"""Return the store the file tools should use.
A factory that raises is not allowed to take the tools down with it: an
integration failing to initialize should degrade to the local filesystem
— the behavior before it was installed — not break file I/O outright.
"""
with _lock:
factory = _factory
if factory is None:
return _local
try:
store = factory()
except Exception:
logger.warning(
"file store factory raised; falling back to the local filesystem",
exc_info=True,
)
return _local
if store is None:
return _local
return store

View File

@@ -1,36 +1,18 @@
from itertools import islice
import os
import logging
from typing import Any
from crewai.tools import BaseTool
from pydantic import BaseModel, Field, PrivateAttr
from crewai_tools.file_storage import FileStore, FileStoreError, resolve_file_store
from crewai_tools.security.safe_path import (
format_error_for_display,
format_path_for_display,
format_sandbox_error,
validate_file_path,
)
def _resolve_against_base(path: str, base_dir: str | None) -> str:
"""Resolve *path* the way the sandbox does, anchoring relatives to *base_dir*.
``validate_file_path`` and ``format_path_for_display`` both join a relative
path onto *base_dir* rather than the working directory. Resolution has to
agree with them, or the same relative string would mean two different files.
Args:
path: The path to resolve.
base_dir: The anchor for relative paths. Defaults to the working directory.
Returns:
The resolved absolute path.
"""
if os.path.isabs(path):
return os.path.realpath(path)
base = os.path.realpath(base_dir) if base_dir is not None else os.getcwd()
return os.path.realpath(os.path.join(base, path))
logger = logging.getLogger(__name__)
class FileReadToolSchema(BaseModel):
@@ -72,6 +54,15 @@ class FileReadTool(BaseTool):
construction, so a later chdir cannot repoint it, and it can be addressed
either by omitting ``file_path`` or by the label shown in the description.
That pin is anchored from what was declared, which matters when a tool is
rebuilt from a serialized crew in a different working directory. An
absolute ``file_path``, or a relative one with ``base_dir`` set, names the
same file after the rebuild as before it. A *relative* ``file_path`` with
no ``base_dir`` names nothing absolute, so it re-anchors to the working
directory of the process doing the rebuilding — the same file the same
arguments would have named there. Pass ``base_dir`` when a declared
relative path must survive a move.
Args:
file_path (Optional[str]): Path to the file to be read. If provided,
this becomes the default file path for the tool.
@@ -103,6 +94,9 @@ class FileReadTool(BaseTool):
_declared_realpath: str | None = PrivateAttr(default=None)
# The label the tool's description shows the LLM for the declared file.
_declared_label: str | None = PrivateAttr(default=None)
# Resolved once per tool: a deployment installs its store before the crew
# is built, and swapping mid-run would change where a path points.
_store: FileStore = PrivateAttr(default=None) # type: ignore[assignment]
def __init__(
self,
@@ -121,28 +115,76 @@ class FileReadTool(BaseTool):
encoding (str): Text encoding used to decode the file.
**kwargs: Additional keyword arguments passed to BaseTool.
"""
# Anchor base_dir once, so the sandbox root cannot move under a later
# chdir while the declared file stays pinned to its original location.
if base_dir is not None:
base_dir = os.path.realpath(base_dir)
display_path = None
# Hand these to pydantic rather than assigning them afterwards, so the
# fields are already populated by the time model_post_init derives
# everything that depends on them. That hook is the only place the
# derivation can live: see its docstring.
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}, 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."
)
kwargs["file_path"] = file_path
if base_dir is not None:
kwargs["base_dir"] = base_dir
kwargs["encoding"] = encoding
super().__init__(**kwargs)
self.file_path = file_path
self.base_dir = base_dir
self.encoding = encoding
self._declared_realpath = (
_resolve_against_base(file_path, base_dir)
if file_path is not None
else None
)
self._declared_label = display_path
def model_post_init(self, context: object) -> None:
"""Bind the store, then derive everything that depends on it.
This has to happen here rather than in ``__init__`` because pydantic
reconstructs a serialized tool through ``model_validate``, which skips
``__init__`` entirely — ``BaseTool._resolve_tool_dict`` does exactly
that when a crew carrying this tool is rebuilt. Deriving here means a
reconstructed reader binds its store and pins its declared path just
like a freshly constructed one, instead of coming back with no store
and failing on the first read.
"""
super().model_post_init(context)
store = resolve_file_store()
self._store = store
# Anchor base_dir once, so the sandbox root cannot move under a later
# chdir while the declared file stays pinned to its original location.
# Deliberately not guarded: anchoring is a containment guarantee, and
# quietly leaving the root relative would let a later chdir move the
# sandbox. A store that cannot normalize should fail loudly here rather
# than hand back a weaker sandbox than the caller asked for.
if self.base_dir is not None:
self.base_dir = store.normalize(self.base_dir)
if self.file_path is not None:
# Guarded, unlike base_dir above: the declared default is a
# convenience, so a store hiccuping while canonicalizing a filename
# should not stop a serialized crew from loading. The reader simply
# comes back without a default file, and any real problem resurfaces
# on the first read, where _run reports it instead of raising.
try:
self._declared_realpath = store.normalize(self.file_path, self.base_dir)
self._declared_label = store.display(
self._declared_realpath, self.base_dir
)
except (FileStoreError, OSError):
logger.warning(
"the %s store could not resolve the declared file %r; the "
"tool will have no default file",
getattr(store, "label", "configured"),
self.file_path,
exc_info=True,
)
self._declared_realpath = None
self._declared_label = None
# The description has to come back too. On a rebuild it arrives
# from the serialized data already advertising a default file,
# so leaving it would promise the LLM something omitting
# 'file_path' can no longer deliver — it would call the tool
# with no arguments and get "No file path provided". Restoring
# the class default keeps what the tool says matched to what it
# does.
default = type(self).model_fields["description"].default
if isinstance(default, str):
self.description = default
else:
self.description = f"A tool that reads file content. The default file is {self._declared_label}, 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."
def _resolve_path(self, file_path: str) -> str:
"""Resolve *file_path* and confirm the tool is allowed to read it.
@@ -166,10 +208,10 @@ class FileReadTool(BaseTool):
declared = self._declared_realpath
if declared is not None and (
file_path == self._declared_label
or _resolve_against_base(file_path, self.base_dir) == declared
or self._store.normalize(file_path, self.base_dir) == declared
):
return declared
return validate_file_path(file_path, self.base_dir)
return self._store.resolve(file_path, self.base_dir)
def _run(
self,
@@ -178,6 +220,27 @@ class FileReadTool(BaseTool):
line_count: int | None = None,
) -> str:
"""Read a file, or a window of its lines, as text."""
try:
return self._read(file_path, start_line, line_count)
except (FileStoreError, OSError) as e:
# A store can fail for reasons the local filesystem never had: an
# unreachable endpoint, a rejected request. Every other exit from
# this tool is an agent-visible string, and raising here would kill
# the agent's step rather than let it react, so this one is too.
# Path resolution and the store's own path labelling both live
# under here, which is why the message carries no path.
return (
f"Error: the {self._store.label} store failed. "
f"{format_error_for_display(e)}"
)
def _read(
self,
file_path: str | None,
start_line: int | None,
line_count: int | None,
) -> str:
"""Do the read. Wrapped by :meth:`_run`, which reports store failures."""
start_line = start_line or 1
line_count = line_count or None
@@ -195,9 +258,10 @@ class FileReadTool(BaseTool):
"directory tree.",
)
display_path = format_path_for_display(file_path, self.base_dir)
store = self._store
display_path = store.display(file_path, self.base_dir)
try:
with open(file_path, "r", encoding=self.encoding) as file:
with store.open_text(file_path, self.encoding) as file:
if start_line == 1 and line_count is None:
return file.read()

View File

@@ -1,14 +1,10 @@
import os
from pathlib import Path
from crewai.tools import BaseTool
from pydantic import BaseModel, Field, field_validator
from pydantic import BaseModel, Field, PrivateAttr
from crewai_tools.file_storage import FileStore, FileStoreError, resolve_file_store
from crewai_tools.security.safe_path import (
format_error_for_display,
format_path_for_display,
format_sandbox_error,
validate_file_path,
)
@@ -88,11 +84,24 @@ class FileWriterTool(BaseTool):
base_dir: str | None = None
encoding: str = "utf-8"
@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
# Resolved once per tool: a deployment installs its store before the crew
# is built, and swapping mid-run would change where a path points.
_store: FileStore = PrivateAttr(default=None) # type: ignore[assignment]
def model_post_init(self, context: object) -> None:
"""Bind the store, then anchor base_dir with the store's own grammar.
Anchoring cannot be a field validator: validators run before
``model_post_init``, so ``_store`` is not bound yet and the only option
there is ``os.path.realpath`` — local-filesystem semantics applied to a
path a remote store may not interpret that way at all. Doing it here
keeps every path decision inside the seam, and still resolves once so a
later chdir cannot move the sandbox.
"""
super().model_post_init(context)
self._store = resolve_file_store()
if self.base_dir is not None:
self.base_dir = self._store.normalize(self.base_dir)
def _run(
self,
@@ -102,6 +111,28 @@ class FileWriterTool(BaseTool):
overwrite: str | bool = False,
) -> str:
"""Write *content* to *filename*, confined to the tool's sandbox."""
try:
return self._write(filename, content, directory, overwrite)
except (FileStoreError, OSError) as e:
# A store can fail for reasons the local filesystem never had: an
# unreachable endpoint, a size ceiling. Every other exit from this
# tool is an agent-visible string, and raising here would kill the
# agent's step rather than let it react, so this one is too. It
# also covers the calls with no handler of their own — exists() and
# the store's own path labelling — which is why it carries no path.
return (
f"An error occurred while writing to the file: the "
f"{self._store.label} store failed. {format_error_for_display(e)}"
)
def _write(
self,
filename: str,
content: str,
directory: str | None,
overwrite: str | bool,
) -> str:
"""Do the write. Wrapped by :meth:`_run`, which reports store failures."""
directory = directory or "./"
try:
@@ -109,11 +140,13 @@ class FileWriterTool(BaseTool):
except ValueError as e:
return f"An error occurred while writing to the file: {e!s}"
store = self._store
# Confine the target directory to base_dir so an LLM-chosen directory
# cannot reach outside the sandbox. validate_file_path also resolves
# symlinks and ".." components.
# cannot reach outside the sandbox. The store also resolves symlinks
# and ".." components before checking.
try:
resolved_directory = Path(validate_file_path(directory, self.base_dir))
resolved_directory = store.resolve(directory, self.base_dir)
except ValueError as e:
return "Error: Invalid directory: " + format_sandbox_error(
e,
@@ -121,31 +154,17 @@ class FileWriterTool(BaseTool):
"directory tree.",
)
# Keep filename inside the target directory, blocking "..", absolute
# paths and symlink escapes. is_relative_to() compares whole path
# components, so it is safe on case-insensitive filesystems and avoids
# the "//" prefix edge case. A filepath that resolves to the directory
# itself (e.g. an empty filename) is not a valid file target.
# Then keep filename inside that directory.
try:
resolved_filepath = Path(
os.path.join(resolved_directory, filename)
).resolve()
except (OSError, ValueError) as e:
# e.g. an embedded null byte, which trips the underlying syscall.
return f"Error: Invalid file path: {format_error_for_display(e)}"
resolved_filepath = store.resolve_within(resolved_directory, filename)
except ValueError as e:
return f"Error: Invalid file path — {e!s}"
display_filepath = format_path_for_display(
str(resolved_filepath), str(resolved_directory)
)
if (
not resolved_filepath.is_relative_to(resolved_directory)
or resolved_filepath == resolved_directory
):
return "Error: Invalid file path — the filename must not escape the target directory."
display_filepath = store.display(resolved_filepath, resolved_directory)
# Covers both a missing 'directory' and subdirectories inside 'filename'.
try:
os.makedirs(resolved_filepath.parent, exist_ok=True)
store.ensure_parent(resolved_filepath)
except FileExistsError:
return (
f"Error: Cannot write to {display_filepath} because a file already "
@@ -157,13 +176,16 @@ class FileWriterTool(BaseTool):
f"{format_error_for_display(e)}"
)
if resolved_filepath.exists() and not overwrite_file:
if store.exists(resolved_filepath) and not overwrite_file:
return f"File {display_filepath} already exists and overwrite option was not passed."
mode = "w" if overwrite_file else "x"
try:
with open(resolved_filepath, mode, encoding=self.encoding) as file:
file.write(content)
store.write_text(
resolved_filepath,
content,
self.encoding,
overwrite=overwrite_file,
)
except FileExistsError:
return f"File {display_filepath} already exists and overwrite option was not passed."
except Exception as e:

View File

@@ -0,0 +1,659 @@
"""The file tools must route all I/O through the registered store.
These tests stand in a store that keeps everything in a dict, with no
filesystem behind it at all. If a tool reaches past the seam to `open()` or
`os.path` the assertions fail, which is the point: it is the guarantee any
non-filesystem store depends on.
"""
from __future__ import annotations
import io
import posixpath
from crewai_tools import FileReadTool, FileWriterTool
from crewai_tools.file_storage import (
FileStore,
FileStoreError,
register_file_store_factory,
reset_file_store_factory,
resolve_file_store,
)
from crewai_tools.file_storage.local import LocalFileStore
import pytest
class MemoryFileStore:
"""A store with no filesystem: paths are keys in a dict.
Modelled on a remote object store — POSIX-ish paths, a flat namespace, no
symlinks, and containment by prefix rather than by `realpath`.
"""
label = "memory"
def __init__(self, root: str = "/ws") -> None:
self.root = root
self.files: dict[str, str] = {}
self.dirs: set[str] = {root}
def _abs(self, path: str, base_dir: str | None = None) -> str:
base = base_dir or self.root
joined = path if path.startswith("/") else posixpath.join(base, path)
return posixpath.normpath(joined)
def normalize(self, path: str, base_dir: str | None = None) -> str:
return self._abs(path, base_dir)
def resolve(self, path: str, base_dir: str | None = None) -> str:
resolved = self._abs(path, base_dir)
# Confine to base_dir when one is given, else to the store root. A
# remote store has to honour base_dir the same way the local one does,
# or the tools' sandbox argument would silently mean nothing.
root = posixpath.normpath(base_dir) if base_dir else self.root
if resolved != root and not resolved.startswith(root.rstrip("/") + "/"):
raise ValueError(
f"Path '{posixpath.basename(resolved)}' is outside the allowed "
f"directory."
)
return resolved
def resolve_within(self, directory: str, filename: str) -> str:
resolved = self._abs(filename, directory)
if resolved == directory or not resolved.startswith(directory + "/"):
raise ValueError("the filename must not escape the target directory")
return resolved
def display(self, resolved: str, base: str | None = None) -> str:
base = base or self.root
if resolved.startswith(base + "/"):
return resolved[len(base) + 1 :]
return posixpath.basename(resolved)
def exists(self, resolved: str) -> bool:
return resolved in self.files
def ensure_parent(self, resolved: str) -> None:
parent = posixpath.dirname(resolved)
if parent in self.files:
raise FileExistsError(parent)
self.dirs.add(parent)
def open_text(self, resolved: str, encoding: str):
if resolved not in self.files:
raise FileNotFoundError(resolved)
return io.StringIO(self.files[resolved])
def write_text(
self, resolved: str, content: str, encoding: str, *, overwrite: bool
) -> None:
if resolved in self.files and not overwrite:
raise FileExistsError(resolved)
self.files[resolved] = content
class FailingFileStore(MemoryFileStore):
"""A store where one named operation fails, the way a remote one can.
The local filesystem cannot produce most of these — an unreachable
endpoint has no local equivalent — so a store that only ever fails the
way `open()` does would not exercise the tools' error paths at all.
"""
def __init__(self, failing: str, exc: Exception | None = None) -> None:
super().__init__()
self.failing = failing
self.exc = exc or FileStoreError("workspace endpoint unreachable")
def _maybe_fail(self, name: str) -> None:
if name == self.failing:
raise self.exc
def normalize(self, path: str, base_dir: str | None = None) -> str:
self._maybe_fail("normalize")
return super().normalize(path, base_dir)
def resolve(self, path: str, base_dir: str | None = None) -> str:
self._maybe_fail("resolve")
return super().resolve(path, base_dir)
def resolve_within(self, directory: str, filename: str) -> str:
self._maybe_fail("resolve_within")
return super().resolve_within(directory, filename)
def display(self, resolved: str, base: str | None = None) -> str:
self._maybe_fail("display")
return super().display(resolved, base)
def exists(self, resolved: str) -> bool:
self._maybe_fail("exists")
return super().exists(resolved)
def ensure_parent(self, resolved: str) -> None:
self._maybe_fail("ensure_parent")
super().ensure_parent(resolved)
def open_text(self, resolved: str, encoding: str):
self._maybe_fail("open_text")
return super().open_text(resolved, encoding)
def write_text(
self, resolved: str, content: str, encoding: str, *, overwrite: bool
) -> None:
self._maybe_fail("write_text")
super().write_text(resolved, content, encoding, overwrite=overwrite)
@pytest.fixture
def store():
memory = MemoryFileStore()
register_file_store_factory(lambda: memory)
yield memory
reset_file_store_factory()
@pytest.fixture
def failing_store():
"""Register a store whose *named* operation fails; the test picks which."""
created: list[FailingFileStore] = []
def _register(failing: str, exc: Exception | None = None) -> FailingFileStore:
broken = FailingFileStore(failing, exc)
register_file_store_factory(lambda: broken)
created.append(broken)
return broken
yield _register
reset_file_store_factory()
def test_default_store_is_local():
assert isinstance(resolve_file_store(), LocalFileStore)
def test_memory_store_satisfies_the_protocol(store):
assert isinstance(store, FileStore)
def test_writer_writes_through_the_store(store, tmp_path, monkeypatch):
# cwd is a real, empty directory: nothing may touch it.
monkeypatch.chdir(tmp_path)
result = FileWriterTool()._run(
filename="report.md", content="# Report", overwrite=True
)
assert "successfully written" in result
assert store.files == {"/ws/report.md": "# Report"}
assert list(tmp_path.iterdir()) == []
def test_reader_reads_through_the_store(store, tmp_path, monkeypatch):
monkeypatch.chdir(tmp_path)
store.files["/ws/notes.txt"] = "line 1\nline 2\nline 3\n"
assert FileReadTool()._run(file_path="notes.txt") == "line 1\nline 2\nline 3\n"
def test_round_trip_between_the_two_tools(store, tmp_path, monkeypatch):
"""The reader must see what the writer wrote — the asymmetry that makes
an ephemeral runtime unusable."""
monkeypatch.chdir(tmp_path)
FileWriterTool()._run(filename="out/data.csv", content="a,b\n1,2\n", overwrite=True)
read_back = FileReadTool()._run(file_path="out/data.csv")
assert read_back == "a,b\n1,2\n"
def test_line_windows_work_on_a_remote_store(store, tmp_path, monkeypatch):
monkeypatch.chdir(tmp_path)
store.files["/ws/big.log"] = "".join(f"L{i}\n" for i in range(1, 101))
assert (
FileReadTool()._run(file_path="big.log", start_line=3, line_count=2)
== "L3\nL4\n"
)
def test_store_containment_is_honoured(store, tmp_path, monkeypatch):
monkeypatch.chdir(tmp_path)
result = FileReadTool()._run(file_path="/etc/passwd")
assert "Invalid file path" in result
assert "base_dir" in result
def test_writer_containment_is_honoured(store, tmp_path, monkeypatch):
monkeypatch.chdir(tmp_path)
result = FileWriterTool()._run(
filename="../escape.txt", content="x", overwrite=True
)
assert "Error" in result
assert store.files == {}
def test_overwrite_false_is_reported_by_the_store(store, tmp_path, monkeypatch):
monkeypatch.chdir(tmp_path)
tool = FileWriterTool()
assert "successfully written" in tool._run(filename="a.txt", content="one")
assert "already exists" in tool._run(filename="a.txt", content="two")
assert store.files["/ws/a.txt"] == "one"
def test_directory_that_is_a_file_reports_clearly(store, tmp_path, monkeypatch):
monkeypatch.chdir(tmp_path)
store.files["/ws/notadir"] = "x"
result = FileWriterTool()._run(
filename="f.txt", directory="notadir", content="x", overwrite=True
)
assert "a file already exists where a directory is needed" in result
def test_factory_returning_none_falls_back_to_local():
register_file_store_factory(lambda: None)
try:
assert isinstance(resolve_file_store(), LocalFileStore)
finally:
reset_file_store_factory()
def test_factory_that_raises_falls_back_to_local(caplog):
"""A broken integration must not take file I/O down with it."""
def boom() -> FileStore | None:
raise RuntimeError("backing service unreachable")
register_file_store_factory(boom)
try:
assert isinstance(resolve_file_store(), LocalFileStore)
finally:
reset_file_store_factory()
assert "falling back to the local filesystem" in caplog.text
def test_store_is_bound_once_per_tool(store, tmp_path, monkeypatch):
"""Swapping the factory mid-run must not move an existing tool's files."""
monkeypatch.chdir(tmp_path)
tool = FileWriterTool()
other = MemoryFileStore(root="/other")
register_file_store_factory(lambda: other)
tool._run(filename="a.txt", content="one", overwrite=True)
assert store.files == {"/ws/a.txt": "one"}
assert other.files == {}
def test_writer_anchors_base_dir_through_the_store(store, tmp_path, monkeypatch):
"""base_dir must be normalised by the store, not by os.path.realpath.
A remote store's paths are not filesystem paths, so anchoring with local
semantics would compute a sandbox root that its own resolve()/
resolve_within() do not agree with.
"""
monkeypatch.chdir(tmp_path)
tool = FileWriterTool(base_dir="scoped")
# The memory store roots at /ws, so its normalisation is what must show up.
assert tool.base_dir == "/ws/scoped"
assert str(tmp_path) not in str(tool.base_dir)
def test_writer_base_dir_confines_writes_under_a_remote_store(
store, tmp_path, monkeypatch
):
monkeypatch.chdir(tmp_path)
tool = FileWriterTool(base_dir="scoped")
assert "successfully written" in tool._run(
filename="in.txt", content="x", overwrite=True
)
assert "/ws/scoped/in.txt" in store.files
escaped = tool._run(filename="x.txt", directory="/ws/elsewhere", content="x")
assert "Error" in escaped
def test_reader_and_writer_agree_on_a_relative_base_dir(store, tmp_path, monkeypatch):
"""Both tools have to land on the same sandbox root for the same input."""
monkeypatch.chdir(tmp_path)
assert FileWriterTool(base_dir="shared").base_dir == (
FileReadTool(base_dir="shared").base_dir
)
def test_os_error_message_does_not_leak_an_absolute_path(tmp_path, monkeypatch):
"""resolve_within wraps OSError, and the writer returns that text verbatim.
str() on an OSError carries the absolute filename, so it has to be reduced
to the reason before it reaches an agent.
"""
monkeypatch.chdir(tmp_path)
result = FileWriterTool()._run(
filename="a\x00b.txt", content="x", overwrite=True
)
assert "Error" in result
assert str(tmp_path) not in result
# --- reconstruction through pydantic -----------------------------------------
#
# BaseTool rebuilds a serialized tool with `model_validate` (see
# `_resolve_tool_dict`), which skips `__init__` entirely. Anything derived only
# in `__init__` comes back missing, and for the store that means every read
# raising AttributeError on a None.
def test_reconstructed_reader_reads_through_the_store(store, tmp_path, monkeypatch):
monkeypatch.chdir(tmp_path)
store.files["/ws/notes.txt"] = "reconstructed\n"
tool = FileReadTool.model_validate({"file_path": "notes.txt"})
assert tool._store is store
assert tool._run() == "reconstructed\n"
def test_reconstructed_writer_writes_through_the_store(store, tmp_path, monkeypatch):
monkeypatch.chdir(tmp_path)
tool = FileWriterTool.model_validate({"base_dir": "scoped"})
assert tool._store is store
assert tool.base_dir == "/ws/scoped"
assert "successfully written" in tool._run(
filename="a.txt", content="x", overwrite=True
)
assert store.files == {"/ws/scoped/a.txt": "x"}
def test_reconstructed_reader_keeps_its_declared_file(store, tmp_path, monkeypatch):
"""The declared path is pinned in the hook, so a rebuild pins it too."""
monkeypatch.chdir(tmp_path)
store.files["/ws/sub/pinned.txt"] = "kept\n"
fresh = FileReadTool(file_path="sub/pinned.txt")
rebuilt = FileReadTool.model_validate({"file_path": "sub/pinned.txt"})
assert rebuilt._declared_realpath == fresh._declared_realpath == "/ws/sub/pinned.txt"
assert rebuilt._declared_label == fresh._declared_label
assert rebuilt.description == fresh.description
# Addressable by the label the description shows, same as a fresh tool.
assert rebuilt._run(file_path=rebuilt._declared_label) == "kept\n"
def test_reader_round_trips_through_model_dump(store, tmp_path, monkeypatch):
"""The full serialize/deserialize cycle, not just a hand-built dict."""
monkeypatch.chdir(tmp_path)
store.files["/ws/notes.txt"] = "dumped\n"
original = FileReadTool(file_path="notes.txt")
rebuilt = FileReadTool.model_validate(original.model_dump())
assert rebuilt._run() == "dumped\n"
# --- a store that fails while a tool is being built --------------------------
#
# `normalize` and `display` are specified as pure and non-raising, precisely
# because the tools call them during construction — including when pydantic
# rebuilds a serialized crew. A store that breaks that contract anyway must not
# take a whole crew down with it, but the two derivations are not equal: the
# declared default file is a convenience, `base_dir` is a containment
# guarantee. So one degrades and one does not.
def test_a_store_failing_on_the_declared_file_still_builds_the_tool(
failing_store, tmp_path, monkeypatch, caplog
):
"""Convenience: no default file rather than no crew."""
monkeypatch.chdir(tmp_path)
store = failing_store("display")
tool = FileReadTool(file_path="notes.txt")
assert tool._declared_realpath is None
assert tool._declared_label is None
assert "no default file" in caplog.text
# Still a working tool — explicit paths go through the store as usual.
store.failing = ""
store.files["/ws/other.txt"] = "still working\n"
assert tool._run(file_path="other.txt") == "still working\n"
# And omitting the path reports the missing default rather than raising.
assert "No file path provided" in tool._run()
def test_a_rebuild_that_loses_the_pin_stops_advertising_a_default(
store, failing_store, tmp_path, monkeypatch
):
"""What the tool says must match what it does.
The description is serialized, so on a rebuild it arrives already naming a
default file. If the pin is then lost, leaving that text would tell the LLM
it can omit `file_path` — and it would get "No file path provided" back.
"""
monkeypatch.chdir(tmp_path)
dumped = FileReadTool(file_path="notes.txt").model_dump()
assert "The default file is" in dumped["description"]
# Same tool, rebuilt against a store that can no longer label the path.
failing_store("display")
rebuilt = FileReadTool.model_validate(dumped)
assert rebuilt._declared_label is None
assert "The default file is" not in rebuilt.description
assert rebuilt.description == FileReadTool.model_fields["description"].default
assert "No file path provided" in rebuilt._run()
def test_a_store_failing_on_base_dir_is_not_swallowed(
failing_store, tmp_path, monkeypatch
):
"""Containment: a sandbox root that cannot be anchored must not be faked.
Degrading here would leave `base_dir` relative, so a later chdir could move
the sandbox — a weaker guarantee than the caller asked for, arrived at
silently. Failing loudly is the point.
"""
monkeypatch.chdir(tmp_path)
failing_store("normalize")
with pytest.raises((FileStoreError, OSError)):
FileReadTool(base_dir="scoped")
with pytest.raises((FileStoreError, OSError)):
FileWriterTool(base_dir="scoped")
# --- what the declared-path pin means across a rebuild -----------------------
#
# The pin is derived from what was *declared*, so whether it survives a rebuild
# in a different working directory depends on whether the declaration named
# somewhere absolute. These three cases are the whole story; they are pinned
# here so the behavior is a decision rather than an accident. Note the local
# store is the one that makes this observable, since its `normalize` is the one
# that consults the process cwd.
def test_an_absolute_declared_path_survives_a_rebuild_elsewhere(tmp_path, monkeypatch):
"""The strongest case: an absolute declaration is cwd-independent."""
here, there = tmp_path / "here", tmp_path / "there"
here.mkdir(), there.mkdir()
(here / "notes.txt").write_text("from here\n")
(there / "notes.txt").write_text("from there\n")
monkeypatch.chdir(here)
dumped = FileReadTool(file_path=str(here / "notes.txt")).model_dump()
monkeypatch.chdir(there)
rebuilt = FileReadTool.model_validate(dumped)
assert rebuilt._run() == "from here\n"
def test_a_declared_base_dir_pins_a_relative_path_across_a_rebuild(tmp_path, monkeypatch):
"""base_dir is anchored at construction, so it carries the pin with it."""
here, there = tmp_path / "here", tmp_path / "there"
here.mkdir(), there.mkdir()
(here / "notes.txt").write_text("from here\n")
(there / "notes.txt").write_text("from there\n")
monkeypatch.chdir(here)
dumped = FileReadTool(file_path="notes.txt", base_dir=str(here)).model_dump()
monkeypatch.chdir(there)
rebuilt = FileReadTool.model_validate(dumped)
assert rebuilt._run() == "from here\n"
def test_a_bare_relative_declared_path_reanchors_on_rebuild(tmp_path, monkeypatch):
"""A relative path with no base_dir names nothing absolute to preserve.
It re-anchors to the rebuilding process's working directory — the same file
the same arguments would name there. Documented rather than "fixed": the
alternative is pinning a path from a working directory that, for a rebuild
in a fresh container, no longer exists. Callers needing the pin to survive
pass `base_dir`, which the test above covers.
"""
here, there = tmp_path / "here", tmp_path / "there"
here.mkdir(), there.mkdir()
(here / "notes.txt").write_text("from here\n")
(there / "notes.txt").write_text("from there\n")
monkeypatch.chdir(here)
dumped = FileReadTool(file_path="notes.txt").model_dump()
monkeypatch.chdir(there)
rebuilt = FileReadTool.model_validate(dumped)
assert rebuilt._run() == "from there\n"
# And it is a real re-anchor, not a stale absolute path that happens to read.
# resolve() because the store canonicalizes, and /tmp is a symlink on macOS.
assert rebuilt._declared_realpath == str((there / "notes.txt").resolve())
# --- a store that fails ------------------------------------------------------
#
# A store may fail where the local filesystem never could. Every exit from
# these tools is an agent-visible string, so a store failure has to become one
# too rather than raising into the agent's step.
@pytest.mark.parametrize(
"failing", ["resolve", "resolve_within", "display", "exists", "ensure_parent"]
)
def test_writer_reports_a_store_failure_instead_of_raising(
failing, failing_store, tmp_path, monkeypatch
):
monkeypatch.chdir(tmp_path)
failing_store(failing)
result = FileWriterTool()._run(filename="a.txt", content="x", overwrite=True)
assert "error occurred while writing" in result
assert "memory" in result
assert "workspace endpoint unreachable" in result
def test_writer_reports_a_failing_write(failing_store, tmp_path, monkeypatch):
"""write_text already had a handler; it must keep its own wording."""
monkeypatch.chdir(tmp_path)
failing_store("write_text")
result = FileWriterTool()._run(filename="a.txt", content="x", overwrite=True)
assert "An error occurred while writing to the file" in result
assert "workspace endpoint unreachable" in result
@pytest.mark.parametrize("failing", ["resolve", "display"])
def test_reader_reports_a_store_failure_instead_of_raising(
failing, failing_store, tmp_path, monkeypatch
):
monkeypatch.chdir(tmp_path)
failing_store(failing)
result = FileReadTool()._run(file_path="notes.txt")
assert "Error" in result
assert "memory" in result
assert "workspace endpoint unreachable" in result
def test_reader_reports_a_failing_open(failing_store, tmp_path, monkeypatch):
"""open_text already had a handler; it must keep its own wording."""
monkeypatch.chdir(tmp_path)
failing_store("open_text")
result = FileReadTool()._run(file_path="notes.txt")
assert "Failed to read file" in result
assert "workspace endpoint unreachable" in result
def _run_either(tool_cls, tool):
"""Drive whichever tool this is with its minimal arguments."""
if tool_cls is FileReadTool:
return tool._run(file_path="notes.txt")
return tool._run(filename="a.txt", content="x", overwrite=True)
@pytest.mark.parametrize("tool_cls", [FileReadTool, FileWriterTool])
def test_an_os_error_from_the_store_is_reported_too(
tool_cls, failing_store, tmp_path, monkeypatch
):
"""Not every store failure arrives as FileStoreError.
A store wrapping a socket or a subprocess can raise OSError from resolve,
which is neither a ValueError nor something the read/write handlers see.
``strerror`` is what survives redaction, so that is what an agent gets.
"""
monkeypatch.chdir(tmp_path)
failing_store("resolve", OSError(104, "Connection reset by peer"))
result = _run_either(tool_cls, tool_cls())
assert "Connection reset by peer" in result
@pytest.mark.parametrize("tool_cls", [FileReadTool, FileWriterTool])
def test_a_bare_os_error_degrades_to_its_type_without_raising(
tool_cls, failing_store, tmp_path, monkeypatch
):
"""A single-arg OSError has no strerror, so only its type survives.
That is `format_error_for_display` holding the line it was given in #6692:
an OS-populated OSError renders its absolute filename into `str()`, so the
message is never passed through wholesale. The cost is a thin report for a
hand-raised `OSError("...")`; stores wanting a legible message should raise
`FileStoreError`, whose text is preserved. What matters here is that the
tool still returns rather than raising.
"""
monkeypatch.chdir(tmp_path)
failing_store("resolve", OSError("connection reset by peer"))
result = _run_either(tool_cls, tool_cls())
assert "OSError" in result
assert "store failed" in result
def test_a_store_failure_does_not_leak_an_absolute_path(
failing_store, tmp_path, monkeypatch
):
"""The store-failure path is agent-visible, so it gets the same redaction."""
monkeypatch.chdir(tmp_path)
failing_store("resolve", OSError(2, "No such file", str(tmp_path / "secret.txt")))
result = FileWriterTool()._run(filename="a.txt", content="x", overwrite=True)
assert str(tmp_path) not in result

View File

@@ -10558,7 +10558,7 @@
"type": "string"
}
},
"description": "A tool for reading file contents.\n\nThis tool inherits its schema handling from BaseTool to avoid recursive schema\ndefinition issues. The args_schema is set to FileReadToolSchema, whose\nfile_path parameter is optional so the tool's default file can be read by\nomitting it. The schema should not be overridden in the constructor as it\nwould break the inheritance chain and cause infinite loops.\n\nThe tool supports two ways of specifying the file path:\n1. At construction time via the file_path parameter\n2. At runtime via the file_path parameter in the tool's input\n\nPaths supplied at runtime must resolve inside ``base_dir`` (the current\nworking directory by default), since they are typically chosen by an LLM.\nA ``file_path`` given at construction time is developer-declared intent and\nis always allowed past the containment check, even when it lives outside\n``base_dir`` (the read itself can still fail). It is pinned at\nconstruction, so a later chdir cannot repoint it, and it can be addressed\neither by omitting ``file_path`` or by the label shown in the description.\n\nArgs:\n file_path (Optional[str]): Path to the file to be read. If provided,\n this becomes the default file path for the tool.\n base_dir (Optional[str]): Directory that runtime paths must stay inside.\n Defaults to the current working directory.\n encoding (str): Text encoding used to decode the file. Defaults to UTF-8.\n **kwargs: Additional keyword arguments passed to BaseTool.\n\nExample:\n >>> tool = FileReadTool(file_path=\"/path/to/file.txt\")\n >>> content = tool.run() # Reads /path/to/file.txt\n >>> content = tool.run(file_path=\"/path/to/other.txt\") # Reads other.txt\n >>> content = tool.run(\n ... file_path=\"/path/to/file.txt\", start_line=100, line_count=50\n ... ) # Reads lines 100-149\n >>> # Widen the sandbox so the agent may read anything under /data:\n >>> tool = FileReadTool(base_dir=\"/data\")",
"description": "A tool for reading file contents.\n\nThis tool inherits its schema handling from BaseTool to avoid recursive schema\ndefinition issues. The args_schema is set to FileReadToolSchema, whose\nfile_path parameter is optional so the tool's default file can be read by\nomitting it. The schema should not be overridden in the constructor as it\nwould break the inheritance chain and cause infinite loops.\n\nThe tool supports two ways of specifying the file path:\n1. At construction time via the file_path parameter\n2. At runtime via the file_path parameter in the tool's input\n\nPaths supplied at runtime must resolve inside ``base_dir`` (the current\nworking directory by default), since they are typically chosen by an LLM.\nA ``file_path`` given at construction time is developer-declared intent and\nis always allowed past the containment check, even when it lives outside\n``base_dir`` (the read itself can still fail). It is pinned at\nconstruction, so a later chdir cannot repoint it, and it can be addressed\neither by omitting ``file_path`` or by the label shown in the description.\n\nThat pin is anchored from what was declared, which matters when a tool is\nrebuilt from a serialized crew in a different working directory. An\nabsolute ``file_path``, or a relative one with ``base_dir`` set, names the\nsame file after the rebuild as before it. A *relative* ``file_path`` with\nno ``base_dir`` names nothing absolute, so it re-anchors to the working\ndirectory of the process doing the rebuilding \u2014 the same file the same\narguments would have named there. Pass ``base_dir`` when a declared\nrelative path must survive a move.\n\nArgs:\n file_path (Optional[str]): Path to the file to be read. If provided,\n this becomes the default file path for the tool.\n base_dir (Optional[str]): Directory that runtime paths must stay inside.\n Defaults to the current working directory.\n encoding (str): Text encoding used to decode the file. Defaults to UTF-8.\n **kwargs: Additional keyword arguments passed to BaseTool.\n\nExample:\n >>> tool = FileReadTool(file_path=\"/path/to/file.txt\")\n >>> content = tool.run() # Reads /path/to/file.txt\n >>> content = tool.run(file_path=\"/path/to/other.txt\") # Reads other.txt\n >>> content = tool.run(\n ... file_path=\"/path/to/file.txt\", start_line=100, line_count=50\n ... ) # Reads lines 100-149\n >>> # Widen the sandbox so the agent may read anything under /data:\n >>> tool = FileReadTool(base_dir=\"/data\")",
"properties": {
"base_dir": {
"anyOf": [