diff --git a/lib/crewai-tools/src/crewai_tools/file_storage/__init__.py b/lib/crewai-tools/src/crewai_tools/file_storage/__init__.py new file mode 100644 index 000000000..1e21c8ef0 --- /dev/null +++ b/lib/crewai-tools/src/crewai_tools/file_storage/__init__.py @@ -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", +] diff --git a/lib/crewai-tools/src/crewai_tools/file_storage/base.py b/lib/crewai-tools/src/crewai_tools/file_storage/base.py new file mode 100644 index 000000000..f8b078ef8 --- /dev/null +++ b/lib/crewai-tools/src/crewai_tools/file_storage/base.py @@ -0,0 +1,103 @@ +"""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. + """ + + 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. + """ + + 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. + """ diff --git a/lib/crewai-tools/src/crewai_tools/file_storage/local.py b/lib/crewai-tools/src/crewai_tools/file_storage/local.py new file mode 100644 index 000000000..5ff18bdee --- /dev/null +++ b/lib/crewai-tools/src/crewai_tools/file_storage/local.py @@ -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) diff --git a/lib/crewai-tools/src/crewai_tools/file_storage/registry.py b/lib/crewai-tools/src/crewai_tools/file_storage/registry.py new file mode 100644 index 000000000..0a2d73a8d --- /dev/null +++ b/lib/crewai-tools/src/crewai_tools/file_storage/registry.py @@ -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 diff --git a/lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py b/lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py index 5814ebbbb..6ff11ab0c 100644 --- a/lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py +++ b/lib/crewai-tools/src/crewai_tools/tools/file_read_tool/file_read_tool.py @@ -1,38 +1,16 @@ from itertools import islice -import os from typing import Any from crewai.tools import BaseTool from pydantic import BaseModel, Field, PrivateAttr +from crewai_tools.file_storage import FileStore, 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)) - - class FileReadToolSchema(BaseModel): """Input for FileReadTool.""" @@ -103,6 +81,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,14 +102,16 @@ class FileReadTool(BaseTool): encoding (str): Text encoding used to decode the file. **kwargs: Additional keyword arguments passed to BaseTool. """ + store = resolve_file_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. if base_dir is not None: - base_dir = os.path.realpath(base_dir) + base_dir = store.normalize(base_dir) display_path = None if file_path is not None: - display_path = format_path_for_display(file_path, base_dir) + display_path = store.display(store.normalize(file_path, base_dir), 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." ) @@ -137,10 +120,9 @@ class FileReadTool(BaseTool): self.file_path = file_path self.base_dir = base_dir self.encoding = encoding + self._store = store self._declared_realpath = ( - _resolve_against_base(file_path, base_dir) - if file_path is not None - else None + store.normalize(file_path, base_dir) if file_path is not None else None ) self._declared_label = display_path @@ -166,10 +148,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, @@ -195,9 +177,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() diff --git a/lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py b/lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py index 86a65b233..e1e4b79a7 100644 --- a/lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py +++ b/lib/crewai-tools/src/crewai_tools/tools/file_writer_tool/file_writer_tool.py @@ -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, 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, @@ -109,11 +118,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 +132,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 +154,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: diff --git a/lib/crewai-tools/tests/file_storage/test_file_store_seam.py b/lib/crewai-tools/tests/file_storage/test_file_store_seam.py new file mode 100644 index 000000000..fdda9fba1 --- /dev/null +++ b/lib/crewai-tools/tests/file_storage/test_file_store_seam.py @@ -0,0 +1,279 @@ +"""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, + 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 + + +@pytest.fixture +def store(): + memory = MemoryFileStore() + register_file_store_factory(lambda: memory) + yield memory + 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