diff --git a/tests/tools/test_file_tools_cwd_resolution.py b/tests/tools/test_file_tools_cwd_resolution.py index eaab8ab47d..82f2cf045e 100644 --- a/tests/tools/test_file_tools_cwd_resolution.py +++ b/tests/tools/test_file_tools_cwd_resolution.py @@ -15,6 +15,7 @@ Core invariant these tests pin: never left to resolve against whatever the process cwd happens to be. """ +import json import os from pathlib import Path, PurePosixPath @@ -259,8 +260,6 @@ def test_v4a_patch_applies_to_resolved_workspace_not_backend_cwd( landed in a different directory than everything the tool reported. The fix rewrites headers to the resolved absolute paths before apply. """ - import json - workspace, decoy = _isolated_cwd task_id = "sess-v4a" @@ -304,18 +303,20 @@ def test_v4a_patch_applies_to_resolved_workspace_not_backend_cwd( @pytest.mark.platforms("posix") -@pytest.mark.parametrize("header", [ - "*** Delete File: local.yaml", - "*** Move File: local.yaml -> old.yaml", - "*** Update File: local.yaml\n@@\n-shared: true\n+shared: false\n*** Move File: local.yaml -> old.yaml", +@pytest.mark.parametrize("header,safe_root,content,dest", [ + ("*** Delete File: local.yaml", False, "shared: true\n", None), + ("*** Move File: local.yaml -> old.yaml", False, "shared: true\n", "old.yaml"), + ("*** Update File: local.yaml\n@@\n-shared: true\n+shared: false\n*** Move File: local.yaml -> old.yaml", + False, "shared: false\n", "old.yaml"), + # The link lives outside HERMES_WRITE_SAFE_ROOT but points inside it: guarding + # only the target would let the delete remove an entry outside the root. + ("*** Delete File: local.yaml", True, "shared: true\n", None), ]) -def test_v4a_delete_and_move_act_on_a_symlink_not_its_target(_isolated_cwd, monkeypatch, header): +def test_v4a_delete_and_move_act_on_a_symlink_not_its_target( + _isolated_cwd, monkeypatch, header, safe_root, content, dest): """Delete and Move act on the directory entry. Resolving a symlinked header to its target deleted or renamed the real file and left the link dangling; an Update still - edits the target's content through the link. The contract holds whatever the move - primitive allows: the target is never deleted or moved, and the link is never lost.""" - import json - + edits the target's content through the link. The write guards cover the entry too.""" from tools.environments.local import LocalEnvironment from tools.file_operations import ShellFileOperations from tools.registry import registry @@ -327,26 +328,24 @@ def test_v4a_delete_and_move_act_on_a_symlink_not_its_target(_isolated_cwd, monk terminal_tool.register_task_env_overrides(task_id, {"cwd": str(workspace)}) env = LocalEnvironment(cwd=str(workspace)) monkeypatch.setattr(ft, "_get_file_ops", lambda task_id="default": ShellFileOperations(env)) - target, link = workspace / "base.yaml", workspace / "local.yaml" - target.write_text("shared: true\n") - link.symlink_to("base.yaml") + (workspace / "safe").mkdir() + target, link = workspace / "safe" / "base.yaml", workspace / "local.yaml" + target.write_text("shared: true\n", encoding="utf-8") + link.symlink_to("safe/base.yaml") + if safe_root: + monkeypatch.setenv("HERMES_WRITE_SAFE_ROOT", str(workspace / "safe")) out = json.loads(registry.dispatch( "patch", {"mode": "patch", "patch": f"*** Begin Patch\n{header}\n*** End Patch\n"}, task_id=task_id)) assert target.is_file() and not target.is_symlink() - assert target.read_text() == ("shared: false\n" if "Update" in header else "shared: true\n") - if "Move" not in header: - assert out.get("success"), out - assert not os.path.lexists(link) - assert str(link) in out["files_modified"] + assert target.read_text(encoding="utf-8") == content + if safe_root: + assert not out.get("success") and "HERMES_WRITE_SAFE_ROOT" in json.dumps(out), out + assert link.is_symlink() return - # A Move renames the link itself; a no-clobber move primitive may refuse a symlink - # source instead, which leaves it in place. It ends up under exactly one name. - moved = os.path.lexists(workspace / "old.yaml") - assert moved != os.path.lexists(link) - kept = workspace / "old.yaml" if moved else link - assert kept.is_symlink() and os.readlink(kept) == "base.yaml" - if moved: - assert out.get("success"), out - assert str(link) in out["files_modified"] + assert out.get("success"), out + assert not os.path.lexists(link) + assert str(link) in out["files_modified"] + if dest: + assert os.readlink(workspace / dest) == "safe/base.yaml" diff --git a/tools/file_operations.py b/tools/file_operations.py index 56437d62dc..7f87aa7d5d 100644 --- a/tools/file_operations.py +++ b/tools/file_operations.py @@ -1251,9 +1251,13 @@ class ShellFileOperations(LintMixin, SearchMixin, FileOperations): """Delete a single file (directories rejected) via the backend's ``python -c`` so one code path works on local/docker/ssh AND Windows shells (no ``rm``).""" path = self._expand_path(path) - denied = get_write_denied_error(path, verb="Delete") - if denied: - return WriteResult(error=denied) + # Delete removes the directory entry (a symlink itself, not its target), so + # the guards also run on the entry's directory; realpath(path) alone would + # clear a link outside the safe root that points inside it. + for p in (path, os.path.dirname(path) or "."): + denied = get_write_denied_error(p, verb="Delete") + if denied: + return WriteResult(error=denied) # Path baked in via repr() for shell-independent quoting; no # ``unlink(missing_ok=True)`` (a 3.7 remote interpreter lacks it). snippet = ( @@ -1288,7 +1292,8 @@ class ShellFileOperations(LintMixin, SearchMixin, FileOperations): def move_file(self, src: str, dst: str) -> WriteResult: src = self._expand_path(src) dst = self._expand_path(dst) - for p in (src, dst): + # Entry-level op like delete_file: guard both entries' directories too. + for p in (src, dst, os.path.dirname(src) or ".", os.path.dirname(dst) or "."): denied = get_write_denied_error(p, verb="Move") if denied: return WriteResult(error=denied)