diff --git a/python/packages/core/agent_framework/_harness/_file_access.py b/python/packages/core/agent_framework/_harness/_file_access.py index 6191bb98f7d..59216eb148f 100644 --- a/python/packages/core/agent_framework/_harness/_file_access.py +++ b/python/packages/core/agent_framework/_harness/_file_access.py @@ -1319,6 +1319,8 @@ def _throw_if_contains_symlink(self, candidate: Path) -> None: is_link = _is_link_or_reparse_point(current) except FileNotFoundError: break + except NotADirectoryError as exc: + raise NotADirectoryError(f"Parent path is not a directory: {current}") from exc except OSError as exc: # Fail closed: if we cannot verify whether a segment is a # symlink/reparse point we refuse the operation rather than @@ -1345,7 +1347,23 @@ async def write(self, path: str, content: str, *, overwrite: bool = True) -> Non @staticmethod def _write_file_sync(full_path: Path, content: str, overwrite: bool) -> None: - full_path.parent.mkdir(parents=True, exist_ok=True) + try: + full_path.parent.mkdir(parents=True, exist_ok=True) + except FileExistsError as exc: + # ``mkdir(parents=True)`` reports FileExistsError when an existing + # file blocks any parent segment. Surface the path type so callers + # can distinguish this from an existing target file. + parent_file = next( + ( + candidate + for candidate in (full_path.parent, *full_path.parent.parents) + if candidate.exists() and not candidate.is_dir() + ), + None, + ) + if parent_file is not None: + raise NotADirectoryError(f"Parent path is not a directory: {parent_file}") from exc + raise encoded = content.encode("utf-8") flags = os.O_WRONLY | os.O_CREAT if overwrite: @@ -1360,6 +1378,10 @@ def _write_file_sync(full_path: Path, content: str, overwrite: bool) -> None: try: fd = os.open(full_path, flags, 0o644) except OSError as exc: + # Windows reports PermissionError when opening a directory for + # writing; POSIX exclusive creation can report FileExistsError. + if isinstance(exc, (FileExistsError, PermissionError)) and full_path.is_dir(): + raise IsADirectoryError(f"Path is a directory: {full_path}") from exc if not overwrite and isinstance(exc, FileExistsError): raise # ``ELOOP`` (POSIX): the open refused because the leaf is a @@ -2008,6 +2030,13 @@ async def file_access_write(file_name: str, content: str, overwrite: bool = Fals store_path = _session_path(normalized) async with self._write_lock: await self.store.write(store_path, content, overwrite=overwrite) + except NotADirectoryError: + return f"Could not write file '{file_name}': a parent path is already a file. Choose a different path." + except IsADirectoryError: + return ( + f"Could not write file '{file_name}': this path is already a directory. " + "Choose a different file name." + ) except FileExistsError: return f"File '{file_name}' already exists. To replace it, write again with overwrite set to true." except ValueError as exc: 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 8c01b1c6146..05d2117ce1c 100644 --- a/python/packages/core/tests/core/test_harness_file_access.py +++ b/python/packages/core/tests/core/test_harness_file_access.py @@ -1335,6 +1335,130 @@ async def _prepare_access_tools( return tools +async def test_file_access_write_reports_actionable_path_collision_errors( + chat_client_base: SupportsChatGetResponse, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Directory/file collisions should guide the model to a valid path.""" + store = InMemoryAgentFileStore() + tools = await _prepare_access_tools(chat_client_base, store=store) + save = _tool_by_name(tools, "file_access_write") + + await store.write("Reports", "keep") + await store.write("Archive/q1.txt", "keep") + original_write = store.write + + async def collision_aware_write(path: str, content: str, *, overwrite: bool = True) -> None: + if path.lower() == "reports/q1.txt": + raise NotADirectoryError("Reports is a file") + if path.lower() == "archive": + raise IsADirectoryError("Archive is a directory") + await original_write(path, content, overwrite=overwrite) + + monkeypatch.setattr(store, "write", collision_aware_write) + + parent_collision = await save.invoke(arguments={"file_name": "reports/q1.txt", "content": "nested"}) + parent_message = _text(parent_collision[0]) + assert "parent path is already a file" in parent_message + assert "Choose a different path" in parent_message + assert await store.read("reports") == "keep" + assert await store.read("reports/q1.txt") is None + + directory_collision = await save.invoke(arguments={"file_name": "ARCHIVE", "content": "replace"}) + directory_message = _text(directory_collision[0]) + assert "already a directory" in directory_message + assert "Choose a different file name" in directory_message + assert await store.read("archive/q1.txt") == "keep" + assert await store.read("archive") is None + + +@pytest.mark.parametrize("overwrite", [False, True]) +async def test_file_access_write_reports_filesystem_path_collision_errors( + chat_client_base: SupportsChatGetResponse, + tmp_path: Path, + overwrite: bool, +) -> None: + """The real filesystem store should distinguish path collisions from existing files.""" + store = FileSystemAgentFileStore(tmp_path) + tools = await _prepare_access_tools(chat_client_base, store=store) + save = _tool_by_name(tools, "file_access_write") + + await store.write("Reports", "keep") + await store.write("Archive/q1.txt", "keep") + await store.write("notes.md", "keep") + + parent_collision = await save.invoke(arguments={"file_name": "Reports/q1.txt", "content": "nested"}) + parent_message = _text(parent_collision[0]) + assert "parent path is already a file" in parent_message + assert "Choose a different path" in parent_message + assert await store.read("Reports") == "keep" + + directory_collision = await save.invoke( + arguments={"file_name": "Archive", "content": "replace", "overwrite": overwrite} + ) + directory_message = _text(directory_collision[0]) + assert "already a directory" in directory_message + assert "Choose a different file name" in directory_message + assert await store.read("Archive/q1.txt") == "keep" + + existing_file = await save.invoke(arguments={"file_name": "notes.md", "content": "replace"}) + existing_file_message = _text(existing_file[0]) + assert "already exists" in existing_file_message + assert "overwrite set to true" in existing_file_message + assert await store.read("notes.md") == "keep" + + +@pytest.mark.parametrize("overwrite", [False, True]) +@pytest.mark.parametrize("is_directory", [False, True]) +async def test_filesystem_store_write_classifies_permission_errors( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, overwrite: bool, is_directory: bool +) -> None: + """Windows directory-open errors are normalized without masking file permission failures.""" + store = FileSystemAgentFileStore(tmp_path) + target = tmp_path / "target" + if is_directory: + target.mkdir() + else: + target.write_text("keep", encoding="utf-8") + denied = PermissionError("access denied") + + def deny_open(path: str | os.PathLike[str], flags: int, mode: int = 0o777) -> int: + raise denied + + with monkeypatch.context() as patch: + patch.setattr(_file_access_module.os, "open", deny_open) + if is_directory: + with pytest.raises(IsADirectoryError) as caught: + await store.write("target", "replace", overwrite=overwrite) + assert caught.value.__cause__ is denied + else: + with pytest.raises(PermissionError) as caught_permission: + await store.write("target", "replace", overwrite=overwrite) + assert caught_permission.value is denied + + if is_directory: + assert target.is_dir() + else: + assert target.read_text(encoding="utf-8") == "keep" + + +async def test_file_access_write_preserves_exclusive_create_guidance( + chat_client_base: SupportsChatGetResponse, +) -> None: + """An existing file with overwrite disabled should retain its overwrite guidance.""" + store = InMemoryAgentFileStore() + tools = await _prepare_access_tools(chat_client_base, store=store) + save = _tool_by_name(tools, "file_access_write") + + await save.invoke(arguments={"file_name": "notes.md", "content": "keep"}) + collision = await save.invoke(arguments={"file_name": "NOTES.md", "content": "replace"}) + message = _text(collision[0]) + + assert "already exists" in message + assert "overwrite set to true" in message + assert await store.read("notes.md") == "keep" + + async def test_file_access_replace(chat_client_base: SupportsChatGetResponse) -> None: """``file_access_replace`` should substitute text and enforce match-count rules.""" tools = await _prepare_access_tools(chat_client_base)