fix(tools): preserve file encoding on overwrite

internal/tools/write_file.go

@@ -1,6 +1,7 @@
 package tools
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"os"
@@ -93,13 +94,23 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an
 	}
 
 	// Capture the prior content (before we replace it) so an overwrite can show a
-	// real diff; a fresh create stays "" and previews as all-additions.
+	// real diff, and so the bytes read_file normalizes away — a BOM, CRLF endings —
+	// survive the rewrite. A fresh create stays "" and previews as all-additions.
+	//
+	// Fail CLOSED when an existing target cannot be read: those bytes are the only
+	// evidence of the convention to restore, so overwriting without them would
+	// write the model's normalized content over a CRLF/BOM file and silently
+	// destroy exactly what this read exists to preserve.
 	priorContent := ""
 	if existed {
-		if prev, rerr := os.ReadFile(absolutePath); rerr == nil {
-			priorContent = string(prev)
+		prev, rerr := os.ReadFile(absolutePath)
+		if rerr != nil {
+			return errorResult("Error writing file " + relativePath + ": cannot read the existing file to preserve its line endings and BOM: " + rerr.Error())
 		}
+		priorContent = string(prev)
+		content = preserveWriteFileEncoding(prev, content)
 	}
+	modelEquivalentContent := content
 
 	if err := os.MkdirAll(filepath.Dir(absolutePath), 0o755); err != nil {
 		return errorResult("Error writing file " + relativePath + ": " + err.Error())
@@ -110,7 +121,6 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an
 	if err := os.WriteFile(absolutePath, []byte(content), 0o644); err != nil {
 		return errorResult("Error writing file " + relativePath + ": " + err.Error())
 	}
-	modelKnownContent := content
 	// Optional format-on-write (ZERO_FORMAT_ON_WRITE). Must run BEFORE the
 	// FileTracker baseline: recording pre-format content would make the very
 	// next edit look like an external modification and trip the conflict guard.
@@ -119,7 +129,7 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an
 	// session compares against what is now on disk.
 	newInfo, _ := os.Stat(absolutePath)
 	options.FileTracker.Record(absolutePath, []byte(content), newInfo)
-	if content == modelKnownContent {
+	if content == modelEquivalentContent {
 		options.FileTracker.RecordSeenRange(absolutePath, 1, trackedLineTotal(content), trackedLineTotal(content))
 	}
 	if !existed {
@@ -147,6 +157,38 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an
 	return result
 }
 
+var utf8BOM = []byte{0xef, 0xbb, 0xbf}
+
+// preserveWriteFileEncoding restores byte-level features hidden by read_file's
+// normalized text view. It keeps line endings consistent with the existing
+// file, while still allowing an LF file to be explicitly replaced with
+// consistently CRLF content.
+func preserveWriteFileEncoding(existing []byte, content string) string {
+	updated := []byte(content)
+	if bytes.HasPrefix(existing, utf8BOM) && !bytes.HasPrefix(updated, utf8BOM) {
+		updated = append(append([]byte(nil), utf8BOM...), updated...)
+	}
+
+	existingCRLF, existingLF := lineEndingCounts(existing)
+	updatedCRLF, updatedLF := lineEndingCounts(updated)
+	useCRLF := existingCRLF > existingLF
+	if !useCRLF && updatedCRLF > updatedLF {
+		// Unlike LF returned by read_file, caller-supplied dominant CRLF is an
+		// unambiguous request to change an LF file's convention.
+		useCRLF = true
+	}
+	updated = bytes.ReplaceAll(updated, []byte("\r\n"), []byte("\n"))
+	if useCRLF {
+		updated = bytes.ReplaceAll(updated, []byte("\n"), []byte("\r\n"))
+	}
+	return string(updated)
+}
+
+func lineEndingCounts(content []byte) (crlf, loneLF int) {
+	crlf = bytes.Count(content, []byte("\r\n"))
+	return crlf, bytes.Count(content, []byte("\n")) - crlf
+}
+
 // fileContentArg reads the file body from "content" or a common alias that weaker
 // models sometimes use instead (contents/text/body/data/file_content). It
 // delegates to the shared aliasedStringArg so the present-but-non-string type

internal/tools/write_file_unreadable_other_test.go

@@ -0,0 +1,29 @@
+//go:build !windows
+
+package tools
+
+import (
+	"os"
+	"testing"
+)
+
+// makeFileWriteOnly drops read permission while leaving the file writable, the
+// shape that lets an overwrite succeed even though its prior bytes cannot be
+// captured. The returned func restores the original mode so the test can read
+// the file back and the temp dir can be cleaned up.
+func makeFileWriteOnly(t *testing.T, path string) func() {
+	t.Helper()
+	info, err := os.Stat(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	mode := info.Mode().Perm()
+	if err := os.Chmod(path, 0o200); err != nil {
+		t.Skipf("cannot drop read permission on this filesystem: %v", err)
+	}
+	return func() {
+		if err := os.Chmod(path, mode); err != nil {
+			t.Fatal(err)
+		}
+	}
+}

internal/tools/write_file_unreadable_windows_test.go

@@ -0,0 +1,60 @@
+//go:build windows
+
+package tools
+
+import (
+	"testing"
+
+	"golang.org/x/sys/windows"
+)
+
+// writeOnlyFileMask is FILE_GENERIC_WRITE, and deliberately not FILE_READ_DATA:
+// os.Stat still reports the file and os.WriteFile still replaces it, but
+// os.ReadFile is denied. Windows has no chmod, so the write-only shape has to be
+// expressed as a DACL.
+//
+// FILE_READ_ATTRIBUTES keeps os.Stat cheap, DELETE lets t.TempDir clean up, and
+// WRITE_DAC is required for the restore: an OWNER_RIGHTS ACE replaces the
+// owner's implicit right to rewrite the descriptor, so it must be granted here.
+const writeOnlyFileMask = "0x170196"
+
+// makeFileWriteOnly replaces the file's DACL with a protected owner-only ACE
+// that grants everything except reading its bytes, and returns a func restoring
+// the descriptor it found.
+func makeFileWriteOnly(t *testing.T, path string) func() {
+	t.Helper()
+	original, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION)
+	if err != nil {
+		t.Skipf("cannot read the current DACL: %v", err)
+	}
+	originalDACL, _, err := original.DACL()
+	if err != nil {
+		t.Skipf("cannot parse the current DACL: %v", err)
+	}
+	writeOnly, err := windows.SecurityDescriptorFromString("D:P(A;;" + writeOnlyFileMask + ";;;OW)")
+	if err != nil {
+		t.Skipf("cannot build a write-only security descriptor: %v", err)
+	}
+	dacl, _, err := writeOnly.DACL()
+	if err != nil {
+		t.Skipf("cannot read the write-only DACL: %v", err)
+	}
+	if err := setFileDACL(path, dacl, true); err != nil {
+		t.Skipf("cannot apply a write-only DACL on this filesystem: %v", err)
+	}
+	return func() {
+		if err := setFileDACL(path, originalDACL, false); err != nil {
+			t.Fatalf("cannot restore the original DACL: %v", err)
+		}
+	}
+}
+
+func setFileDACL(path string, dacl *windows.ACL, protected bool) error {
+	info := windows.SECURITY_INFORMATION(windows.DACL_SECURITY_INFORMATION)
+	if protected {
+		info |= windows.PROTECTED_DACL_SECURITY_INFORMATION
+	} else {
+		info |= windows.UNPROTECTED_DACL_SECURITY_INFORMATION
+	}
+	return windows.SetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, info, nil, nil, dacl, nil)
+}

internal/tools/write_tools_test.go

@@ -1,6 +1,7 @@
 package tools
 
 import (
+	"bytes"
 	"context"
 	"errors"
 	"io"
@@ -139,6 +140,152 @@ func TestWriteFileToolCreatesAndProtectsExistingFiles(t *testing.T) {
 	}
 }
 
+func TestWriteFileToolOverwritePreservesExistingEncoding(t *testing.T) {
+	tests := []struct {
+		name     string
+		existing []byte
+		content  string
+		want     []byte
+	}{
+		{name: "LF", existing: []byte("old\ntext\n"), content: "new\ntext\n", want: []byte("new\ntext\n")},
+		{name: "CRLF", existing: []byte("old\r\ntext\r\n"), content: "new\ntext\n", want: []byte("new\r\ntext\r\n")},
+		{name: "BOM and CRLF", existing: []byte("\xef\xbb\xbfold\r\ntext\r\n"), content: "new\ntext\n", want: []byte("\xef\xbb\xbfnew\r\ntext\r\n")},
+		{name: "explicit CRLF", existing: []byte("old\ntext\n"), content: "new\r\ntext\r\n", want: []byte("new\r\ntext\r\n")},
+		{name: "mixed content follows existing CRLF", existing: []byte("old\r\ntext\r\n"), content: "new\r\ntext\nmore\n", want: []byte("new\r\ntext\r\nmore\r\n")},
+		{name: "mixed content follows existing LF", existing: []byte("old\ntext\n"), content: "new\r\ntext\nmore\n", want: []byte("new\ntext\nmore\n")},
+		{name: "explicit BOM", existing: []byte("old\n"), content: "\xef\xbb\xbfnew\n", want: []byte("\xef\xbb\xbfnew\n")},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			root := t.TempDir()
+			path := filepath.Join(root, "example.txt")
+			if err := os.WriteFile(path, tt.existing, 0o644); err != nil {
+				t.Fatal(err)
+			}
+
+			result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{
+				"path": "example.txt", "content": tt.content, "overwrite": true,
+			})
+			if result.Status != StatusOK {
+				t.Fatalf("overwrite failed: %s", result.Output)
+			}
+			got, err := os.ReadFile(path)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if !bytes.Equal(got, tt.want) {
+				t.Fatalf("written bytes = %q, want %q", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestWriteFileToolEncodingPreservationKeepsWholeFileObservation(t *testing.T) {
+	t.Setenv("ZERO_FORMAT_ON_WRITE", "")
+	tests := []struct {
+		name     string
+		existing []byte
+	}{
+		{name: "CRLF", existing: []byte("old\r\ntext\r\n")},
+		{name: "BOM and CRLF", existing: []byte("\xef\xbb\xbfold\r\ntext\r\n")},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			root := t.TempDir()
+			path := filepath.Join(root, "example.txt")
+			if err := os.WriteFile(path, tt.existing, 0o644); err != nil {
+				t.Fatal(err)
+			}
+			trackedPath, err := filepath.EvalSymlinks(path)
+			if err != nil {
+				t.Fatal(err)
+			}
+			tracker := NewFileTracker()
+			options := RunOptions{FileTracker: tracker}
+
+			read := NewScopedReadFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{
+				"path": "example.txt",
+			}, options)
+			if read.Status != StatusOK {
+				t.Fatalf("initial read failed: %s", read.Output)
+			}
+
+			writeTool := NewScopedWriteFileTool(root, nil).(optionsAwareTool)
+			for _, content := range []string{"new\ntext\n", "newer\ntext\n"} {
+				result := writeTool.RunWithOptions(context.Background(), map[string]any{
+					"path": "example.txt", "content": content, "overwrite": true,
+				}, options)
+				if result.Status != StatusOK {
+					t.Fatalf("overwrite with %q failed: %s", content, result.Output)
+				}
+				if !tracker.SeenWhole(trackedPath) {
+					t.Fatalf("transparent encoding preservation discarded the whole-file observation after writing %q", content)
+				}
+			}
+		})
+	}
+}
+
+func TestWriteFileToolNewFileRetainsCallerBytes(t *testing.T) {
+	root := t.TempDir()
+	want := []byte("\xef\xbb\xbfnew\r\ntext\n")
+	result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{
+		"path": "example.txt", "content": string(want),
+	})
+	if result.Status != StatusOK {
+		t.Fatalf("write failed: %s", result.Output)
+	}
+	got, err := os.ReadFile(filepath.Join(root, "example.txt"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !bytes.Equal(got, want) {
+		t.Fatalf("written bytes = %q, want %q", got, want)
+	}
+}
+
+// TestWriteFileToolFailsClosedWhenExistingTargetIsUnreadable covers the gap the
+// encoding-preservation change opens: the overwrite path has already proven the
+// target exists, so a failed read of its bytes leaves no evidence of the BOM and
+// CRLF endings to restore. Writing anyway would push the model's normalized
+// content over the file and destroy the very convention this change preserves,
+// so an unreadable existing target has to be a write error, not a silent
+// fallback to the unpreserved bytes.
+func TestWriteFileToolFailsClosedWhenExistingTargetIsUnreadable(t *testing.T) {
+	root := t.TempDir()
+	path := filepath.Join(root, "example.txt")
+	existing := []byte("\xef\xbb\xbfold\r\ntext\r\n")
+	if err := os.WriteFile(path, existing, 0o644); err != nil {
+		t.Fatal(err)
+	}
+	restore := makeFileWriteOnly(t, path)
+	if _, err := os.ReadFile(path); err == nil {
+		restore()
+		t.Skip("this environment still allows reading a write-only file")
+	}
+
+	result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{
+		"path": "example.txt", "content": "new\ntext\n", "overwrite": true,
+	})
+	restore()
+
+	if result.Status != StatusError {
+		t.Fatalf("overwrite of an unreadable existing file reported %v, want an error: %s", result.Status, result.Output)
+	}
+	if !strings.Contains(result.Output, "cannot read the existing file") {
+		t.Fatalf("error = %q, want the fail-closed encoding-preservation message", result.Output)
+	}
+	got, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !bytes.Equal(got, existing) {
+		t.Fatalf("file bytes = %q, want the original %q left untouched", got, existing)
+	}
+}
+
 func TestWriteFileToolRecordsCreatedFileButNotOverwrite(t *testing.T) {
 	root := t.TempDir()
 	registry := NewRegistry()