fix(tools): preserve file encoding on overwrite

internal/tools/write_file.go

@@ -1,6 +1,7 @@
 package tools
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"os"
@@ -23,9 +24,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. Reapplied after 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. Reapplied after optional formatting."},
 				},
 				Required:             []string{"path", "content"},
 				AdditionalProperties: false,
@@ -56,6 +59,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 {
@@ -97,15 +114,27 @@ 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 := ""
 	priorContentKnown := !existed
+	var encoding writeFileEncoding
 	if existed {
-		if prev, rerr := tool.readFile(absolutePath); rerr == nil {
-			priorContent = string(prev)
-			priorContentKnown = true
+		prev, rerr := tool.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)
+		priorContentKnown = true
+		encoding = resolveWriteFileEncoding(prev, content, bom, lineEndings)
+		content = encoding.apply(content)
 	}
+	modelEquivalentContent := content
 
 	if err := os.MkdirAll(filepath.Dir(absolutePath), 0o755); err != nil {
 		return errorResult("Error writing file " + relativePath + ": " + err.Error())
@@ -120,7 +149,6 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an
 	if err := commitFileContents(absolutePath, priorInfo, expectedContent, content); 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.
@@ -137,12 +165,32 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an
 	if newInfo == nil {
 		newInfo, _ = os.Stat(absolutePath)
 	}
+	// Encoding preservation must be the last thing to touch the bytes. Formatters
+	// such as gofmt normalize endings to LF, so re-impose the same BOM and ending
+	// decision on their output and publish it through the same guarded commit.
+	// The result is then the preserved form of what the model sent whenever the
+	// formatter changed nothing else, which keeps the whole-file observation.
+	if existed && finalContentKnown {
+		if reencoded := encoding.apply(content); reencoded != content {
+			if newInfo == nil {
+				options.FileTracker.Forget(absolutePath)
+				return errorResult("Error writing file " + relativePath + ": cannot restore line endings and BOM after formatting: the formatted file could not be inspected")
+			}
+			formatted := content
+			if err := commitFileContents(absolutePath, newInfo, &formatted, reencoded); err != nil {
+				options.FileTracker.Forget(absolutePath)
+				return errorResult("Error writing file " + relativePath + ": cannot restore line endings and BOM after formatting: " + err.Error())
+			}
+			content = reencoded
+			newInfo, _ = os.Stat(absolutePath)
+		}
+	}
 	if finalContentKnown {
 		options.FileTracker.Record(absolutePath, []byte(content), newInfo)
 	} else {
 		options.FileTracker.Forget(absolutePath)
 	}
-	if finalContentKnown && content == modelKnownContent {
+	if finalContentKnown && content == modelEquivalentContent {
 		options.FileTracker.RecordSeenRange(absolutePath, 1, trackedLineTotal(content), trackedLineTotal(content))
 	}
 	if !existed {
@@ -186,6 +234,60 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an
 	return result
 }
 
+var utf8BOM = []byte{0xef, 0xbb, 0xbf}
+
+// writeFileEncoding is the BOM and line-ending convention an overwrite
+// publishes. It is decided once, from the existing bytes and the model's
+// content, so formatter output can be brought back to the same convention
+// without the formatter's own LF output looking like a request for LF.
+type writeFileEncoding struct {
+	bom  bool
+	crlf bool
+}
+
+// resolveWriteFileEncoding 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.
+func resolveWriteFileEncoding(existing []byte, content, bom, lineEndings string) writeFileEncoding {
+	encoding := writeFileEncoding{
+		bom: bom == "add" || (bom == "auto" && (bytes.HasPrefix(existing, utf8BOM) || strings.HasPrefix(content, string(utf8BOM)))),
+	}
+	existingCRLF, existingLF := lineEndingCounts(existing)
+	updatedCRLF, updatedLF := lineEndingCounts([]byte(content))
+	encoding.crlf = existingCRLF > existingLF
+	if !encoding.crlf && updatedCRLF > updatedLF {
+		// Unlike LF returned by read_file, caller-supplied dominant CRLF is an
+		// unambiguous request to change an LF file's convention.
+		encoding.crlf = true
+	}
+	if lineEndings != "auto" {
+		encoding.crlf = lineEndings == "crlf"
+	}
+	return encoding
+}
+
+// apply rewrites content into the convention. It is idempotent, so applying it
+// to bytes that already follow the convention returns them unchanged.
+func (encoding writeFileEncoding) apply(content string) string {
+	updated := bytes.TrimPrefix([]byte(content), utf8BOM)
+	if encoding.bom {
+		updated = append(append([]byte(nil), utf8BOM...), updated...)
+	}
+	updated = bytes.ReplaceAll(updated, []byte("\r\n"), []byte("\n"))
+	updated = bytes.ReplaceAll(updated, []byte("\r"), []byte("\n"))
+	if encoding.crlf {
+		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,31 @@
+//go:build !windows
+
+package tools
+
+import (
+	"os"
+	"testing"
+)
+
+// makeFileWriteOnly drops read permission while leaving the file writable, so
+// the target can still be opened for writing but its prior bytes cannot be read.
+// write_file must refuse that overwrite, since those bytes are the only evidence
+// of the BOM and line endings to preserve. 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)
+}