diff --git a/python/packages/core/agent_framework/_harness/_file_access.py b/python/packages/core/agent_framework/_harness/_file_access.py index 0aa2d139b34..f9cc46a2c4a 100644 --- a/python/packages/core/agent_framework/_harness/_file_access.py +++ b/python/packages/core/agent_framework/_harness/_file_access.py @@ -90,6 +90,15 @@ # regex match when building a result snippet. _SEARCH_SNIPPET_RADIUS = 50 +# Path complexity bounds shared by every store backend. Real filesystems cap +# these too (PATH_MAX and practical directory depth); the in-memory store's +# directory index materializes the full key of every ancestor directory, so an +# unbounded path would let a single write retain quadratic space in the path +# depth. Bounding here keeps that retention constant regardless of input, and +# rejects the same extremes the filesystem backend's OS layer would refuse. +_MAX_PATH_SEGMENTS: Final[int] = 255 +_MAX_PATH_LENGTH: Final[int] = 4096 + # Hard cap on the length of a user-supplied search regex. The cap is a coarse bound on # how much work a single pattern can describe; it does not stop a short pathological # pattern, which is what :class:`_BoundedSearchPattern` is for. @@ -296,6 +305,10 @@ def _normalize_relative_path(path: str, *, is_directory: bool = False) -> str: ValueError: When the path is rooted, starts with a drive letter, contains ``.``/``..`` segments, is empty for a file path, or ends with a separator for a file path. + OSError: With :data:`errno.ENAMETOOLONG` when the normalized path + exceeds ``_MAX_PATH_SEGMENTS`` segments or ``_MAX_PATH_LENGTH`` + characters, matching what a real filesystem raises for the same + input. """ if not path or not path.strip(): if not is_directory: @@ -333,6 +346,9 @@ def _normalize_relative_path(path: str, *, is_directory: bool = False) -> str: result = "/".join(clean_segments) if not is_directory and not result: raise ValueError(f"Invalid path: {path!r}. A file path must not be empty.") + if len(clean_segments) > _MAX_PATH_SEGMENTS or len(result) > _MAX_PATH_LENGTH: + # Check before any caller materializes ancestor prefixes of this path. + raise OSError(errno.ENAMETOOLONG, "File name too long", path) return result @@ -1048,6 +1064,23 @@ def scan_batch(pending: list[tuple[str, str]]) -> list[FileSearchResult]: async def create_directory(self, path: str) -> None: """Ensure ``path`` exists as a directory, creating it if necessary.""" + async def delete_directory(self, path: str) -> bool: + """Delete the empty directory at ``path``. + + Args: + path: The relative path of the directory to delete. + + Returns: + ``True`` if an empty directory was removed; ``False`` if it did not + exist, was not a directory, or still held entries. Backends that + cannot remove directories return ``False`` rather than raising. + + ``delete`` stays file-only so backends remain interchangeable; this is + the supported way to release an explicitly created directory (for + example a session-scoped working folder) once it is empty. + """ + return False + @experimental(feature_id=ExperimentalFeature.HARNESS) class InMemoryAgentFileStore(AgentFileStore): @@ -1067,12 +1100,24 @@ def __init__(self) -> None: # wrote, mirroring how :class:`FileSystemAgentFileStore` preserves the # on-disk casing. self._files: dict[str, tuple[str, str]] = {} + self._dir_counts: dict[str, int] = {} + self._explicit_dirs: dict[str, str] = {} self._lock = asyncio.Lock() @staticmethod def _key(path: str) -> str: return _normalize_relative_path(path).lower() + @staticmethod + def _ancestor_keys(key: str) -> list[str]: + """Return all strict ancestor directory keys in top-down order in linear time.""" + ancestors: list[str] = [] + idx = key.find("/") + while idx != -1: + ancestors.append(key[:idx]) + idx = key.find("/", idx + 1) + return ancestors + async def write(self, path: str, content: str, *, overwrite: bool = True) -> None: """Write ``content`` to the file at ``path``. @@ -1083,9 +1128,19 @@ async def write(self, path: str, content: str, *, overwrite: bool = True) -> Non display = _normalize_relative_path(path) key = display.lower() async with self._lock: + ancestors = self._ancestor_keys(key) + for ancestor in ancestors: + if ancestor in self._files: + raise NotADirectoryError(f"Not a directory: {path!r}") + if self._dir_counts.get(key, 0) > 0 or key in self._explicit_dirs: + raise IsADirectoryError(f"Is a directory: {path!r}") if not overwrite and key in self._files: raise FileExistsError(f"File already exists: {path!r}") + is_new_file = key not in self._files self._files[key] = (display, content) + if is_new_file: + for ancestor in ancestors: + self._dir_counts[ancestor] = self._dir_counts.get(ancestor, 0) + 1 async def read(self, path: str) -> str | None: """Return the file content, or ``None`` if the file does not exist.""" @@ -1095,10 +1150,48 @@ async def read(self, path: str) -> str | None: return entry[1] if entry is not None else None async def delete(self, path: str) -> bool: - """Delete the file and return whether anything was removed.""" + """Delete the file and return whether anything was removed. + + Deleting is file-only, matching :class:`FileSystemAgentFileStore`: a + path that names a directory (explicitly created or holding stored + descendants) is never removed and returns ``False``. Use + :meth:`delete_directory` to release an empty explicitly created + directory. + """ key = self._key(path) async with self._lock: - return self._files.pop(key, None) is not None + if key in self._explicit_dirs or self._dir_counts.get(key, 0) > 0: + return False + removed = self._files.pop(key, None) + if removed is None: + return False + for ancestor in self._ancestor_keys(key): + new_count = self._dir_counts[ancestor] - 1 + if new_count <= 0: + del self._dir_counts[ancestor] + else: + self._dir_counts[ancestor] = new_count + return True + + async def delete_directory(self, path: str) -> bool: + """Delete the empty explicitly created directory at ``path``. + + The directory is removed only when it is empty: nothing is stored under + it and no other explicit directory is nested below it. This keeps the + file-only ``delete`` contract while giving session-scoped directories a + lifecycle instead of staying in the shared map forever. + """ + key = self._key(_normalize_relative_path(path, is_directory=True)) + async with self._lock: + if key not in self._explicit_dirs: + return False + if self._dir_counts.get(key, 0) > 0: + return False + prefix = key + "/" + if any(other != key and other.startswith(prefix) for other in self._explicit_dirs): + return False + del self._explicit_dirs[key] + return True async def list_children(self, directory: str = "") -> list[FileStoreEntry]: """Return the direct child files and subdirectories of ``directory``. @@ -1118,11 +1211,12 @@ async def list_children(self, directory: str = "") -> list[FileStoreEntry]: prefix += "/" prefix_depth = prefix.count("/") async with self._lock: - entries = [(key, display) for key, (display, _) in self._files.items()] + file_entries = [(key, display) for key, (display, _) in self._files.items()] + dir_entries = list(self._explicit_dirs.items()) files: list[str] = [] directories: list[str] = [] seen_dirs: set[str] = set() - for key, display in entries: + for key, display in file_entries: if not key.startswith(prefix): continue # Unicode lowercasing can change character counts, so key offsets @@ -1137,6 +1231,17 @@ async def list_children(self, directory: str = "") -> list[FileStoreEntry]: continue seen_dirs.add(segment_key) directories.append(segment) + for key, display in dir_entries: + if not key.startswith(prefix) or key == prefix.rstrip("/"): + continue + remainder = display.split("/", prefix_depth)[-1] + segment, _, _ = remainder.partition("/") + if segment: + segment_key = segment.lower() + if segment_key in seen_dirs: + continue + seen_dirs.add(segment_key) + directories.append(segment) results: list[FileStoreEntry] = [FileStoreEntry(name, FileStoreEntry.DIRECTORY) for name in directories] results.extend(FileStoreEntry(name, FileStoreEntry.FILE) for name in files) return results @@ -1194,8 +1299,30 @@ def scan() -> list[FileSearchResult]: return await _run_search_with_timeout(asyncio.to_thread(scan)) async def create_directory(self, path: str) -> None: - """No-op: directories are implicit from file paths in the in-memory store.""" - del path + """Ensure the directory at ``path`` exists.""" + display = _normalize_relative_path(path, is_directory=True) + key = display.lower() + if not key: + return + async with self._lock: + ancestors = self._ancestor_keys(key) + for ancestor in ancestors: + if ancestor in self._files: + raise FileExistsError(f"File already exists: {ancestor!r}") + if key in self._files: + raise FileExistsError(f"File already exists: {key!r}") + if key not in self._explicit_dirs: + idx_display = 0 + for ancestor in ancestors: + next_slash = display.find("/", idx_display) + if next_slash != -1: + ancestor_display = display[:next_slash] + idx_display = next_slash + 1 + else: + ancestor_display = display + if ancestor not in self._explicit_dirs: + self._explicit_dirs[ancestor] = ancestor_display + self._explicit_dirs[key] = display @experimental(feature_id=ExperimentalFeature.HARNESS) @@ -1444,6 +1571,25 @@ def _delete_file_sync(cls, full_path: Path) -> bool: return False return True + async def delete_directory(self, path: str) -> bool: + """Delete the empty directory at ``path``. + + Returns ``True`` only when an empty directory was removed; a directory + that is missing, is a file, or still holds entries returns ``False`` + instead of raising, matching :class:`InMemoryAgentFileStore`. + """ + full_path = self._resolve_safe_directory_path(path) + return await asyncio.to_thread(self._delete_directory_sync, full_path) + + @staticmethod + def _delete_directory_sync(full_path: Path) -> bool: + try: + full_path.rmdir() + except OSError: + # Missing path, a file, or a directory that still holds entries. + return False + return True + async def list_children(self, directory: str = "") -> list[FileStoreEntry]: """Return the direct child files and subdirectories of ``directory``. diff --git a/python/packages/core/tests/core/test_harness_file_access.py b/python/packages/core/tests/core/test_harness_file_access.py index 377c5768f5d..7ee996016b1 100644 --- a/python/packages/core/tests/core/test_harness_file_access.py +++ b/python/packages/core/tests/core/test_harness_file_access.py @@ -313,6 +313,115 @@ async def test_in_memory_store_normalizes_paths() -> None: await store.write(bad, "boom") +async def test_in_memory_store_rejects_file_ancestor_collision() -> None: + """Writing a file whose ancestor is already a file must raise NotADirectoryError.""" + store = InMemoryAgentFileStore() + await store.write("Reports", "file content") + + with pytest.raises(NotADirectoryError): + await store.write("Reports/q1.txt", "nested content") + + with pytest.raises(NotADirectoryError): + await store.write("reports/deep/q2.txt", "deep nested content") + + assert await store.read("Reports") == "file content" + children = await store.list_children("") + assert len(children) == 1 + assert children[0].name == "Reports" + assert children[0].type == FileStoreEntry.FILE + + +async def test_in_memory_store_rejects_descendant_directory_collision() -> None: + """Writing a file whose path is already a directory must raise IsADirectoryError.""" + store = InMemoryAgentFileStore() + await store.write("Reports/q1.txt", "nested content") + + with pytest.raises(IsADirectoryError): + await store.write("Reports", "file content") + + with pytest.raises(IsADirectoryError): + await store.write("reports", "case insensitive collision") + + assert await store.read("Reports/q1.txt") == "nested content" + assert await store.read("Reports") is None + children = await store.list_children("") + assert len(children) == 1 + assert children[0].name == "Reports" + assert children[0].type == FileStoreEntry.DIRECTORY + + +async def test_in_memory_store_create_directory_rejects_existing_file() -> None: + """Creating a directory that collides with an existing file must raise FileExistsError.""" + store = InMemoryAgentFileStore() + await store.write("Reports", "file content") + + with pytest.raises(FileExistsError): + await store.create_directory("Reports") + + with pytest.raises(FileExistsError): + await store.create_directory("reports/nested") + + +async def test_in_memory_store_explicit_directory_reserves_path_against_write() -> None: + """Explicitly created directory must reserve the path and reject file writes.""" + store = InMemoryAgentFileStore() + await store.create_directory("Reports") + + with pytest.raises(IsADirectoryError): + await store.write("Reports", "file content") + + with pytest.raises(IsADirectoryError): + await store.write("reports", "case insensitive collision") + + children = await store.list_children("") + assert len(children) == 1 + assert children[0].name == "Reports" + assert children[0].type == FileStoreEntry.DIRECTORY + + nested_children = await store.list_children("Reports") + assert len(nested_children) == 0 + + +async def test_in_memory_store_explicit_nested_directory_creation() -> None: + """Explicit nested directory creation registers intermediate directories and preserves casing.""" + store = InMemoryAgentFileStore() + await store.create_directory("Reports/2026/Q1") + + root_children = await store.list_children("") + assert len(root_children) == 1 + assert root_children[0].name == "Reports" + assert root_children[0].type == FileStoreEntry.DIRECTORY + + reports_children = await store.list_children("Reports") + assert len(reports_children) == 1 + assert reports_children[0].name == "2026" + assert reports_children[0].type == FileStoreEntry.DIRECTORY + + q1_children = await store.list_children("Reports/2026") + assert len(q1_children) == 1 + assert q1_children[0].name == "Q1" + assert q1_children[0].type == FileStoreEntry.DIRECTORY + + with pytest.raises(IsADirectoryError): + await store.write("reports/2026", "collision") + + +async def test_in_memory_store_delete_file_cleans_up_implicit_directory_counts() -> None: + """Deleting a file unreserves implicit parent directories when no other files remain.""" + store = InMemoryAgentFileStore() + await store.write("data/temp/file.txt", "file content") + + with pytest.raises(IsADirectoryError): + await store.write("data/temp", "should fail while file exists") + + deleted = await store.delete("data/temp/file.txt") + assert deleted is True + + # Now that the implicit descendant is gone and temp was never explicitly created, writing succeeds + await store.write("data/temp", "now a file") + assert await store.read("data/temp") == "now a file" + + async def test_filesystem_store_round_trips_files(tmp_path: Path) -> None: """The filesystem store should round-trip files on disk and create parents on write.""" store = FileSystemAgentFileStore(tmp_path) @@ -2610,3 +2719,150 @@ async def test_scope_alone_enables_scoped_mode(chat_client_base: SupportsChatGet provider = FileAccessProvider(store=store, scope="tenant-1") context = SessionContext(session_id=None, input_messages=[]) await provider.before_run(agent=None, session=AgentSession(), context=context, state={}) + + +async def test_filesystem_delete_is_file_only_and_delete_directory_matches_in_memory( + tmp_path: Path, +) -> None: + """FileSystemAgentFileStore.delete must reject directories exactly like the + in-memory backend, and delete_directory must release an empty directory.""" + + async def in_memory_mirror() -> tuple[InMemoryAgentFileStore, FileSystemAgentFileStore]: + memory_store = InMemoryAgentFileStore() + fs_root = tmp_path / "root" + fs_store = FileSystemAgentFileStore(fs_root) + for store in (memory_store, fs_store): + await store.create_directory("sessions/s1") + await store.write("sessions/s1/state.txt", "data") + return memory_store, fs_store + + memory_store, fs_store = await in_memory_mirror() + + # Backend parity: deleting a directory returns False on both backends. + assert await fs_store.delete("sessions/s1") is False + assert await memory_store.delete("sessions/s1") is False + + # Deleting the file first leaves the (now empty) directory in place. + assert await fs_store.delete("sessions/s1/state.txt") is True + assert await memory_store.delete("sessions/s1/state.txt") is True + for store in (fs_store, memory_store): + assert [entry.name for entry in await store.list_children("sessions/s1")] == [] + + # delete_directory releases the empty directory on both backends. + assert await fs_store.delete_directory("sessions/s1") is True + assert await memory_store.delete_directory("sessions/s1") is True + for store in (fs_store, memory_store): + assert await store.list_children("sessions") == [] + + # A second release of the same path reports False on both backends. + assert await fs_store.delete_directory("sessions/s1") is False + assert await memory_store.delete_directory("sessions/s1") is False + + +async def test_in_memory_store_delete_is_file_only() -> None: + """delete() must stay file-only: a directory path is never removed, matching + FileSystemAgentFileStore.delete on the same input (backend parity).""" + store = InMemoryAgentFileStore() + await store.create_directory("sessions/s1") + + assert await store.delete("sessions/s1") is False + assert await store.delete("sessions/s1/state.txt") is False + children = await store.list_children("sessions") + assert [entry.name for entry in children] == ["s1"] + + await store.create_directory("sessions/s2") + await store.write("sessions/s2/state.txt", "data") + assert await store.delete("sessions/s2") is False + assert await store.delete("sessions/s2/state.txt") is True + assert await store.delete("sessions/s2") is False + assert await store.delete("sessions") is False + + # The emptied directory stays listed, exactly like an empty directory on a filesystem. + entries = await store.list_children("sessions") + assert [entry.name for entry in entries] == ["s1", "s2"] + + +async def test_in_memory_store_delete_directory_releases_empty_explicit_directory() -> None: + """delete_directory() releases an empty explicitly created directory so a + session-scoped working folder does not stay in the shared map forever.""" + store = InMemoryAgentFileStore() + await store.create_directory("sessions/s1") + + assert await store.delete_directory("sessions/s1") is True + children = await store.list_children("sessions") + assert children == [] + + await store.create_directory("sessions/s2") + await store.write("sessions/s2/state.txt", "data") + assert await store.delete_directory("sessions/s2") is False + assert await store.delete("sessions/s2/state.txt") is True + assert await store.delete_directory("sessions/s2") is True + assert await store.delete_directory("sessions") is True + + +async def test_in_memory_store_rejects_paths_beyond_the_complexity_bound() -> None: + """A path deeper than the shared bound is rejected before any ancestor + prefix is materialized, so one write cannot retain quadratic memory in the + path depth. The error matches what the OS raises for the same input on the + filesystem backend (ENAMETOOLONG).""" + import errno + + store = InMemoryAgentFileStore() + path = "/".join(["a"] * 8000 + ["f"]) + + with pytest.raises(OSError) as exc_info: + await store.write(path, "x") + + assert exc_info.value.errno == errno.ENAMETOOLONG + # Nothing was stored and no ancestor prefixes were retained. + assert store._files == {} + assert store._dir_counts == {} + # Reads of the same path hit the same validation, never the index. + with pytest.raises(OSError) as read_info: + await store.read(path) + assert read_info.value.errno == errno.ENAMETOOLONG + + # A total length past the bound is refused the same way even when shallow. + with pytest.raises(OSError) as long_info: + await store.write("/".join(["b" * 3000, "c" * 3000]), "x") + assert long_info.value.errno == errno.ENAMETOOLONG + assert store._files == {} + + +async def test_in_memory_store_accepts_deep_paths_within_the_bound() -> None: + """Legitimate deep paths (deeper than any realistic hierarchy) still work, + and the retention of the directory index stays linear in the stored path.""" + store = InMemoryAgentFileStore() + path = "/".join(["d"] * 200 + ["f"]) + + await store.write(path, "x") + + assert await store.read(path) == "x" + # Every retained ancestor key is a prefix of the one stored path, so the + # index is bounded by a small constant times the path length. + retained = sum(len(k) for k in store._dir_counts) + assert retained < 200 * len(path) + + +async def test_filesystem_store_matches_in_memory_complexity_bound(tmp_path) -> None: + """Both backends refuse the same over-bound path with the same errno, so + backend substitution is not observable on the bound either.""" + import errno + + fs_store = FileSystemAgentFileStore(str(tmp_path)) + mem_store = InMemoryAgentFileStore() + path = "/".join(["a"] * 8000 + ["f"]) + + with pytest.raises(OSError) as fs_info: + await fs_store.write(path, "x") + with pytest.raises(OSError) as mem_info: + await mem_store.write(path, "x") + + assert fs_info.value.errno == errno.ENAMETOOLONG + assert mem_info.value.errno == errno.ENAMETOOLONG + + within = "/".join(["d"] * 200 + ["f"]) + await fs_store.write(within, "x") + await mem_store.write(within, "x") + assert await fs_store.read(within) == "x" + assert await mem_store.read(within) == "x" diff --git a/python/packages/core/tests/core/test_harness_file_memory.py b/python/packages/core/tests/core/test_harness_file_memory.py index 58d75284f82..5d8f7394be2 100644 --- a/python/packages/core/tests/core/test_harness_file_memory.py +++ b/python/packages/core/tests/core/test_harness_file_memory.py @@ -307,8 +307,11 @@ async def test_tools_reject_nested_paths() -> None: saved = await tools["file_memory_write"].invoke(arguments={"file_name": "notes/plan.md", "content": "x"}) assert "subdirectory" in _text(saved) - # Nothing should have been written for the nested name. - assert await store.list_children("") == [] + # Nothing should have been written for the nested name. The session + # directory created during before_run is the only root entry, and the + # rejected write leaves it empty (no "notes" child). + assert [entry.name for entry in await store.list_children("")] == ["session-1"] + assert await store.list_children("session-1") == [] # Backslash separators are normalized to "/" and rejected the same way. saved_backslash = await tools["file_memory_write"].invoke(arguments={"file_name": "notes\\plan.md", "content": "x"})