fix(tools): preserve file encoding on overwrite

internal/tools/write_file.go

@@ -1,6 +1,7 @@
 package tools
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"os"
@@ -22,9 +23,11 @@ func NewScopedWriteFileTool(workspaceRoot string, scope PathScope) Tool {
 			parameters: Schema{
 				Type: "object",
 				Properties: map[string]PropertySchema{
-					"path":      {Type: "string", Description: "Absolute or relative path of the file to write."},
-					"content":   {Type: "string", Description: "Full file contents to write."},
-					"overwrite": {Type: "boolean", Description: "Whether to allow overwriting an existing file.", Default: false},
+					"path":         {Type: "string", Description: "Absolute or relative path of the file to write."},
+					"content":      {Type: "string", Description: "Full file contents to write."},
+					"overwrite":    {Type: "boolean", Description: "Whether to allow overwriting an existing file.", Default: false},
+					"bom":          {Type: "string", Enum: []string{"auto", "add", "remove"}, Default: "auto", Description: "Existing-file overwrites only: auto preserves an existing or supplied UTF-8 BOM; add/remove explicitly sets its presence. New files retain content bytes. Applied before optional formatting."},
+					"line_endings": {Type: "string", Enum: []string{"auto", "lf", "crlf"}, Default: "auto", Description: "Existing-file overwrites only: auto preserves dominant existing endings (or supplied dominant CRLF); lf/crlf explicitly selects endings, independently of bom. New files retain content bytes. Applied before optional formatting."},
 				},
 				Required:             []string{"path", "content"},
 				AdditionalProperties: false,
@@ -54,6 +57,20 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an
 	if err != nil {
 		return errorResult("Error: Invalid arguments for write_file: " + err.Error())
 	}
+	bom, err := stringArg(args, "bom", "auto", false)
+	if err != nil {
+		return errorResult("Error: Invalid arguments for write_file: " + err.Error())
+	}
+	if bom != "auto" && bom != "add" && bom != "remove" {
+		return errorResult("Error: Invalid arguments for write_file: bom must be auto, add, or remove")
+	}
+	lineEndings, err := stringArg(args, "line_endings", "auto", false)
+	if err != nil {
+		return errorResult("Error: Invalid arguments for write_file: " + err.Error())
+	}
+	if lineEndings != "auto" && lineEndings != "lf" && lineEndings != "crlf" {
+		return errorResult("Error: Invalid arguments for write_file: line_endings must be auto, lf, or crlf")
+	}
 
 	absolutePath, relativePath, err := resolveScopedTargetPath(tool.workspaceRoot, tool.scope, requestedPath)
 	if err != nil {
@@ -93,13 +110,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, bom, lineEndings)
 	}
+	modelEquivalentContent := content
 
 	if err := os.MkdirAll(filepath.Dir(absolutePath), 0o755); err != nil {
 		return errorResult("Error writing file " + relativePath + ": " + err.Error())
@@ -110,7 +137,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.
@@ -120,7 +146,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 {
@@ -149,6 +175,44 @@ 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. Explicit BOM and line-ending intent independently
+// overrides this automatic behavior, before optional formatting.
+func preserveWriteFileEncoding(existing []byte, content, bom, lineEndings string) string {
+	updated := []byte(content)
+	if bom == "remove" {
+		updated = bytes.TrimPrefix(updated, utf8BOM)
+	} else if (bom == "add" || 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
+	}
+	if lineEndings != "auto" {
+		useCRLF = lineEndings == "crlf"
+	}
+	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)
+}