diff --git a/tests/tools/test_file_operations.py b/tests/tools/test_file_operations.py index e80938d3ef7a6..7be49c51c31de 100644 --- a/tests/tools/test_file_operations.py +++ b/tests/tools/test_file_operations.py @@ -615,3 +615,61 @@ class TestAtomicWriteNewFilePermissions: assert result.error is None, f"write failed: {result.error}" assert dest.read_text() == "#!/bin/sh\necho updated\n" assert dest.stat().st_mode & 0o777 == 0o755 + + +class TestAtomicWriteThroughSymlink: + """_atomic_write must edit a symlink's target, not replace the link. + + Regression: the temp-file + ``mv`` swap replaced the symlink itself with a + plain file, orphaning the real target and destroying the link (data-loss). + """ + + def test_write_follows_symlink_and_preserves_link(self, tmp_path): + ops = ShellFileOperations(make_real_subprocess_env(str(tmp_path))) + real = tmp_path / "real.txt" + link = tmp_path / "link.txt" + real.write_text("original\n") + link.symlink_to(real) + + result = ops.write_file(str(link), "newcontent\n") + + assert result.error is None, f"write failed: {result.error}" + # The link must survive as a symlink... + assert link.is_symlink(), "symlink was replaced by a plain file" + # ...and the real target must carry the new content. + assert real.read_text() == "newcontent\n" + assert os.path.realpath(link) == str(real) + + def test_write_through_broken_symlink_falls_back(self, tmp_path): + """A broken link resolves through readlink -f and creates the target.""" + ops = ShellFileOperations(make_real_subprocess_env(str(tmp_path))) + target = tmp_path / "target.txt" + link = tmp_path / "broken.lnk" + link.symlink_to(target) # target does not exist yet + + result = ops.write_file(str(link), "data\n") + + assert result.error is None, f"write failed: {result.error}" + assert target.exists() + assert target.read_text() == "data\n" + + +class TestReadNonUtf8IsBinary: + """Non-UTF-8 content must be flagged binary, not returned as lossy text. + + Regression: the terminal env decodes stdout with errors="replace", turning + every non-UTF-8 byte into U+FFFD before _is_likely_binary sees it. U+FFFD is + "printable", so the non-printable ratio never caught it, and a + read→edit→write round-trip would overwrite the original bytes with mojibake. + """ + + def test_replacement_char_sample_flagged_binary(self, tmp_path): + ops = ShellFileOperations(make_real_subprocess_env(str(tmp_path))) + # A latin-1 file decoded with errors="replace" yields U+FFFD chars. + lossy_sample = "caf\ufffd r\ufffdsum\ufffd\n" + assert ops._is_likely_binary("notes.txt", lossy_sample) is True + + def test_plain_utf8_text_not_flagged(self, tmp_path): + ops = ShellFileOperations(make_real_subprocess_env(str(tmp_path))) + # Proper UTF-8 (including non-ASCII) must still read as text. + assert ops._is_likely_binary("notes.txt", "café résumé\nsecond\n") is False diff --git a/tools/file_operations.py b/tools/file_operations.py index d0d1f53a833ee..59224325d885c 100644 --- a/tools/file_operations.py +++ b/tools/file_operations.py @@ -884,6 +884,17 @@ class ShellFileOperations(FileOperations): # Content analysis: >30% non-printable chars = binary if content_sample: + # Undecodable bytes: the terminal env decodes stdout with + # errors="replace", so any non-UTF-8 byte arrives here already + # turned into U+FFFD. That char is "printable" (ord 65533), so the + # non-printable ratio below never catches it — and returning the + # lossy text would let a read→edit→write round-trip silently + # overwrite the original bytes with mojibake. Treat a file whose + # sample carries the replacement char as binary (read-only) so the + # agent can't corrupt it. Legitimate UTF-8 text effectively never + # contains U+FFFD. + if "\ufffd" in content_sample[:1000]: + return True non_printable = sum(1 for c in content_sample[:1000] if ord(c) < 32 and c not in '\n\r\t') return non_printable / min(len(content_sample), 1000) > 0.30 @@ -1022,11 +1033,21 @@ class ShellFileOperations(FileOperations): script = ( "set -e; " f"d={q_parent}; t={q_path}; " + # Follow a symlink target so we edit the file the link points at, + # rather than replacing the symlink itself with a plain file (which + # orphans the real target and destroys the link). Recompute the + # temp dir from the RESOLVED target so `mv` stays same-filesystem + # atomic. Best-effort: a broken link or missing readlink/realpath + # falls back to the original path (pre-fix behavior, no regression). + 'if [ -L "$t" ]; then ' + 'rt="$(readlink -f "$t" 2>/dev/null || realpath "$t" 2>/dev/null || true)"; ' + '[ -n "$rt" ] && { t="$rt"; d="$(dirname "$t")"; }; ' + "fi; " 'tmp="$(mktemp -p "$d" ' + tmpl + ' 2>/dev/null ' '|| mktemp "$d/.hermes-tmp.$$.XXXXXX" 2>/dev/null ' '|| { tmp="$d/.hermes-tmp.$$"; : > "$tmp" && echo "$tmp"; })"; ' '[ -n "$tmp" ] || { echo "atomic write: could not create temp file" >&2; exit 1; }; ' - "trap 'rm -f \"$tmp\"' EXIT; " + "trap 'rm -f \\\"$tmp\\\"' EXIT; " # preserve mode of an existing target (best-effort, never fatal) 'if [ -e "$t" ]; then ' 'm="$(stat -c%a "$t" 2>/dev/null || stat -f%Lp "$t" 2>/dev/null || true)"; '