From a2e9bdf7c6161b3b91f5054cb242ebe8ceb7f5a4 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 27 Aug 2026 22:18:09 +0200 Subject: [PATCH 1/7] fix(tools): preserve file encoding on overwrite Amp-Thread-ID: https://ampcode.com/threads/T-01a0448f-5860-721c-8a47-5119fc57f685 Co-authored-by: Amp Co-authored-by: Pierre Bruno --- internal/tools/write_file.go | 40 +++++++++++++++++++- internal/tools/write_tools_test.go | 60 ++++++++++++++++++++++++++++++ 2 files changed, 99 insertions(+), 1 deletion(-) diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index 937d0c02a..5741a7b50 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -1,6 +1,7 @@ package tools import ( + "bytes" "context" "fmt" "os" @@ -100,12 +101,18 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an // real diff; a fresh create stays "" and previews as all-additions. priorContent := "" priorContentKnown := !existed + var priorBytes []byte if existed { if prev, rerr := tool.readFile(absolutePath); rerr == nil { + priorBytes = prev priorContent = string(prev) priorContentKnown = true } } + modelKnownContent := content + if priorBytes != nil { + content = preserveWriteFileEncoding(priorBytes, content) + } if err := os.MkdirAll(filepath.Dir(absolutePath), 0o755); err != nil { return errorResult("Error writing file " + relativePath + ": " + err.Error()) @@ -120,7 +127,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. @@ -186,6 +192,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 diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index 29afd57b2..b3fc06bd1 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -1,6 +1,7 @@ package tools import ( + "bytes" "context" "errors" "io" @@ -141,6 +142,65 @@ 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 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) + } +} + func TestWriteFileToolRecordsCreatedFileButNotOverwrite(t *testing.T) { root := t.TempDir() registry := NewRegistry() From 721d2c93a89228491d2fc2e8b01ed1ea3be3350c Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Fri, 28 Aug 2026 20:50:20 +0200 Subject: [PATCH 2/7] fix(tools): retain write observation after encoding restore Co-authored-by: Pierre Bruno --- internal/tools/write_file.go | 4 +-- internal/tools/write_tools_test.go | 47 ++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index 5741a7b50..56c9ef10b 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -109,10 +109,10 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an priorContentKnown = true } } - modelKnownContent := content if priorBytes != nil { content = preserveWriteFileEncoding(priorBytes, content) } + modelEquivalentContent := content if err := os.MkdirAll(filepath.Dir(absolutePath), 0o755); err != nil { return errorResult("Error writing file " + relativePath + ": " + err.Error()) @@ -148,7 +148,7 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an } else { options.FileTracker.Forget(absolutePath) } - if finalContentKnown && content == modelKnownContent { + if finalContentKnown && content == modelEquivalentContent { options.FileTracker.RecordSeenRange(absolutePath, 1, trackedLineTotal(content), trackedLineTotal(content)) } if !existed { diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index b3fc06bd1..9cf80516c 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -183,6 +183,53 @@ func TestWriteFileToolOverwritePreservesExistingEncoding(t *testing.T) { } } +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") From 55b1be55e21af01efe852ee7b705c8f75ab57456 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 3 Sep 2026 21:27:42 +0200 Subject: [PATCH 3/7] fix(tools): fail closed when an overwrite target cannot be read The overwrite path proves the target exists, then treated a failed os.ReadFile as if there were no prior bytes: priorBytes stayed nil, preserveWriteFileEncoding was skipped, and os.WriteFile replaced the file anyway. A write-only existing CRLF/BOM file was therefore overwritten successfully with the model's normalized bytes, losing the exact convention this change exists to preserve. Those prior bytes are both the diff source and the only evidence of the encoding to restore, so capturing them can no longer be optional once we are on the existing-file path. An unreadable existing target is now a write error before os.WriteFile; a fresh create still passes the caller's bytes through untouched. The regression covers a writable-but-unreadable target on both shapes of platform: chmod 0o200 elsewhere, and a protected owner-only DACL without FILE_READ_DATA on Windows, which has no chmod to express it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01REzorhNj3F1DGPXyn5Uq7j Co-authored-by: Pierre Bruno --- internal/tools/write_file.go | 22 ++++--- .../tools/write_file_unreadable_other_test.go | 29 +++++++++ .../write_file_unreadable_windows_test.go | 60 +++++++++++++++++++ internal/tools/write_tools_test.go | 40 +++++++++++++ 4 files changed, 142 insertions(+), 9 deletions(-) create mode 100644 internal/tools/write_file_unreadable_other_test.go create mode 100644 internal/tools/write_file_unreadable_windows_test.go diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index 56c9ef10b..4f7101121 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -98,19 +98,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 := "" priorContentKnown := !existed - var priorBytes []byte if existed { - if prev, rerr := tool.readFile(absolutePath); rerr == nil { - priorBytes = prev - 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()) } - } - if priorBytes != nil { - content = preserveWriteFileEncoding(priorBytes, content) + priorContent = string(prev) + priorContentKnown = true + content = preserveWriteFileEncoding(prev, content) } modelEquivalentContent := content diff --git a/internal/tools/write_file_unreadable_other_test.go b/internal/tools/write_file_unreadable_other_test.go new file mode 100644 index 000000000..6c0f987a4 --- /dev/null +++ b/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) + } + } +} diff --git a/internal/tools/write_file_unreadable_windows_test.go b/internal/tools/write_file_unreadable_windows_test.go new file mode 100644 index 000000000..d5a44d733 --- /dev/null +++ b/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) +} diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index 9cf80516c..720128f3e 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -248,6 +248,46 @@ func TestWriteFileToolNewFileRetainsCallerBytes(t *testing.T) { } } +// 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() From 8adf75887bdc18bce629aaa76a1eb3d0c0e32f7d Mon Sep 17 00:00:00 2001 From: Amp Date: Mon, 7 Sep 2026 18:06:04 +0000 Subject: [PATCH 4/7] fix(tools): allow explicit overwrite encoding intent Amp-Thread-ID: https://ampcode.com/threads/T-01a07cfb-fb1b-7787-820c-f39c38d497a6 Co-authored-by: Pierre Bruno --- internal/tools/write_file.go | 36 ++++++++++--- internal/tools/write_tools_test.go | 84 ++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 7 deletions(-) diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index 4f7101121..abb67c655 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -24,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. 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, @@ -57,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 { @@ -114,7 +130,7 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an } priorContent = string(prev) priorContentKnown = true - content = preserveWriteFileEncoding(prev, content) + content = preserveWriteFileEncoding(prev, content, bom, lineEndings) } modelEquivalentContent := content @@ -201,10 +217,13 @@ 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 { +// 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 bytes.HasPrefix(existing, utf8BOM) && !bytes.HasPrefix(updated, utf8BOM) { + 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...) } @@ -216,6 +235,9 @@ func preserveWriteFileEncoding(existing []byte, content string) string { // 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")) diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index 720128f3e..05696e1f9 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -183,6 +183,89 @@ func TestWriteFileToolOverwritePreservesExistingEncoding(t *testing.T) { } } +func TestWriteFileToolExplicitEncodingIntent(t *testing.T) { + t.Setenv("ZERO_FORMAT_ON_WRITE", "") + // Explicit intent wins independently; omitted intent retains automatic + // restoration. Every case also exercises a second tracked overwrite. + for _, tt := range []struct { + name, existing, content, bom, endings, want string + }{ + {"default both", "\ufeffold\r\n", "new\n", "", "", "\ufeffnew\r\n"}, + {"remove BOM retain CRLF", "\ufeffold\r\n", "new\n", "remove", "", "new\r\n"}, + {"LF retain BOM", "\ufeffold\r\n", "new\n", "", "lf", "\ufeffnew\n"}, + {"LF without BOM", "old\r\n", "new\n", "", "lf", "new\n"}, + {"remove and LF", "\ufeffold\r\n", "\ufeffnew\r\n", "remove", "lf", "new\n"}, + {"add BOM retain LF", "old\n", "new\n", "add", "", "\ufeffnew\n"}, + {"CRLF retain BOM", "\ufeffold\n", "new\n", "", "crlf", "\ufeffnew\r\n"}, + {"CRLF without BOM", "old\n", "new\n", "", "crlf", "new\r\n"}, + {"add and CRLF", "old\n", "new\n", "add", "crlf", "\ufeffnew\r\n"}, + {"empty remove BOM", "\ufeffold\r\n", "", "remove", "", ""}, + {"empty default", "\ufeffold\r\n", "", "", "", "\ufeff"}, + } { + t.Run(tt.name, func(t *testing.T) { + root := t.TempDir() + path := filepath.Join(root, "example.txt") + if err := os.WriteFile(path, []byte(tt.existing), 0o644); err != nil { + t.Fatal(err) + } + path, err := filepath.EvalSymlinks(path) + if err != nil { + t.Fatal(err) + } + options := RunOptions{FileTracker: NewFileTracker()} + read := NewScopedReadFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{"path": path}, options) + if read.Status != StatusOK { + t.Fatal(read.Output) + } + args := map[string]any{"path": path, "content": tt.content, "overwrite": true} + if tt.bom != "" { + args["bom"] = tt.bom + } + if tt.endings != "" { + args["line_endings"] = tt.endings + } + for i := 0; i < 2; i++ { + result := NewScopedWriteFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), args, options) + if result.Status != StatusOK { + t.Fatal(result.Output) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if string(got) != tt.want { + t.Fatalf("written bytes = %q, want %q", got, tt.want) + } + if !options.FileTracker.SeenWhole(path) { + t.Fatal("encoding intent discarded whole-file observation") + } + } + }) + } +} + +func TestWriteFileToolRejectsInvalidEncodingIntent(t *testing.T) { + for _, key := range []string{"bom", "line_endings"} { + for _, value := range []any{"invalid", true} { + root := t.TempDir() + path := filepath.Join(root, "example.txt") + if err := os.WriteFile(path, []byte("original"), 0o644); err != nil { + t.Fatal(err) + } + result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ + "path": path, "content": "replacement", "overwrite": true, key: value, + }) + if result.Status != StatusError { + t.Fatalf("accepted %s=%v", key, value) + } + got, err := os.ReadFile(path) + if err != nil || string(got) != "original" { + t.Fatalf("invalid intent changed target: %q, %v", got, err) + } + } + } +} + func TestWriteFileToolEncodingPreservationKeepsWholeFileObservation(t *testing.T) { t.Setenv("ZERO_FORMAT_ON_WRITE", "") tests := []struct { @@ -235,6 +318,7 @@ func TestWriteFileToolNewFileRetainsCallerBytes(t *testing.T) { want := []byte("\xef\xbb\xbfnew\r\ntext\n") result := NewScopedWriteFileTool(root, nil).Run(context.Background(), map[string]any{ "path": "example.txt", "content": string(want), + "bom": "remove", "line_endings": "lf", }) if result.Status != StatusOK { t.Fatalf("write failed: %s", result.Output) From 93df93b46ae4314b065cece3b858764ec2877aa1 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Sat, 19 Sep 2026 21:06:21 +0200 Subject: [PATCH 5/7] test(tools): align unreadable overwrite coverage after rebase --- internal/tools/write_tools_test.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index 05696e1f9..5a0c9fdce 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -681,7 +681,7 @@ func TestWriteFileToolOverwriteEmitsRedGreenDiff(t *testing.T) { } } -func TestWriteFileToolOmitsDiffWhenOverwritePreimageCannotBeRead(t *testing.T) { +func TestWriteFileToolRejectsOverwriteWhenPreimageCannotBeRead(t *testing.T) { root := t.TempDir() path := filepath.Join(root, "private.txt") writeTestFile(t, path, "before\n") @@ -692,14 +692,14 @@ func TestWriteFileToolOmitsDiffWhenOverwritePreimageCannotBeRead(t *testing.T) { result := registry.RunWithOptions(context.Background(), tool.Name(), map[string]any{ "path": "private.txt", "content": "after\n", "overwrite": true, }, RunOptions{PermissionGranted: true}) - if result.Status != StatusOK { - t.Fatalf("write = %s", result.Output) + if result.Status != StatusError { + t.Fatalf("unreadable preimage must reject overwrite: %s", result.Output) } if len(result.FileDiffs) != 0 { t.Fatalf("unreadable preimage must not produce a create-like diff: %#v", result.FileDiffs) } - if got, err := os.ReadFile(path); err != nil || string(got) != "after\n" { - t.Fatalf("written content = %q, err = %v", got, err) + if got, err := os.ReadFile(path); err != nil || string(got) != "before\n" { + t.Fatalf("original content = %q, err = %v", got, err) } } From e0c8647ffd25060d38d7ec495ec6808c35cb3da8 Mon Sep 17 00:00:00 2001 From: Amp Date: Sat, 19 Sep 2026 20:22:42 +0000 Subject: [PATCH 6/7] fix(tools): normalize lone CR during overwrite conversion Amp-Thread-ID: https://ampcode.com/threads/T-01a0bb3f-2ac7-7755-984b-3753ae32d3d3 Co-authored-by: Pierre Bruno --- internal/tools/write_file.go | 1 + internal/tools/write_tools_test.go | 2 ++ 2 files changed, 3 insertions(+) diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index abb67c655..8ebae4486 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -239,6 +239,7 @@ func preserveWriteFileEncoding(existing []byte, content, bom, lineEndings string useCRLF = lineEndings == "crlf" } updated = bytes.ReplaceAll(updated, []byte("\r\n"), []byte("\n")) + updated = bytes.ReplaceAll(updated, []byte("\r"), []byte("\n")) if useCRLF { updated = bytes.ReplaceAll(updated, []byte("\n"), []byte("\r\n")) } diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index 5a0c9fdce..239086c5c 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -199,6 +199,8 @@ func TestWriteFileToolExplicitEncodingIntent(t *testing.T) { {"CRLF retain BOM", "\ufeffold\n", "new\n", "", "crlf", "\ufeffnew\r\n"}, {"CRLF without BOM", "old\n", "new\n", "", "crlf", "new\r\n"}, {"add and CRLF", "old\n", "new\n", "add", "crlf", "\ufeffnew\r\n"}, + {"lone CR to LF", "old\r\n", "first\rsecond\r\nthird\nfourth\r", "", "lf", "first\nsecond\nthird\nfourth\n"}, + {"lone CR to CRLF", "old\n", "first\rsecond\r\nthird\nfourth\r", "", "crlf", "first\r\nsecond\r\nthird\r\nfourth\r\n"}, {"empty remove BOM", "\ufeffold\r\n", "", "remove", "", ""}, {"empty default", "\ufeffold\r\n", "", "", "", "\ufeff"}, } { From 56983740592b3836188052e7d633a90eef53bf3e Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Fri, 25 Sep 2026 14:16:12 +0200 Subject: [PATCH 7/7] fix(tools): reapply overwrite encoding after format-on-write With ZERO_FORMAT_ON_WRITE=1 the formatter ran after encoding preservation and was the last writer, so gofmt turned a preserved CRLF file back to LF and dropped its BOM. The result then no longer matched the model-equivalent content, so the whole-file observation was cleared and the next overwrite was refused as unread. Split preservation into a decision made once from the prior bytes and the model's content, and an idempotent apply. The apply now also runs on the formatter's output and publishes any difference through the guarded commit, so preservation is the last transformation. A formatter that only normalizes endings keeps the observation; one that really edits the content still clears it. Also align the unreadable-target test helper comment with the fail-closed overwrite behaviour. Co-Authored-By: Claude Opus 5.5 (1M context) --- internal/tools/write_file.go | 73 ++++++++++---- .../tools/write_file_unreadable_other_test.go | 10 +- internal/tools/write_tools_test.go | 94 +++++++++++++++++++ 3 files changed, 155 insertions(+), 22 deletions(-) diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index 8ebae4486..bd9aa6e5f 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -27,8 +27,8 @@ func NewScopedWriteFileTool(workspaceRoot string, scope PathScope) Tool { "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."}, + "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, @@ -123,6 +123,7 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an // destroy exactly what this read exists to preserve. priorContent := "" priorContentKnown := !existed + var encoding writeFileEncoding if existed { prev, rerr := tool.readFile(absolutePath) if rerr != nil { @@ -130,7 +131,8 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an } priorContent = string(prev) priorContentKnown = true - content = preserveWriteFileEncoding(prev, content, bom, lineEndings) + encoding = resolveWriteFileEncoding(prev, content, bom, lineEndings) + content = encoding.apply(content) } modelEquivalentContent := content @@ -163,6 +165,26 @@ 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 { @@ -214,33 +236,48 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an var utf8BOM = []byte{0xef, 0xbb, 0xbf} -// preserveWriteFileEncoding restores byte-level features hidden by read_file's +// 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, 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...) +// 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(updated) - useCRLF := existingCRLF > existingLF - if !useCRLF && updatedCRLF > updatedLF { + 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. - useCRLF = true + encoding.crlf = true } if lineEndings != "auto" { - useCRLF = lineEndings == "crlf" + 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 useCRLF { + if encoding.crlf { updated = bytes.ReplaceAll(updated, []byte("\n"), []byte("\r\n")) } return string(updated) diff --git a/internal/tools/write_file_unreadable_other_test.go b/internal/tools/write_file_unreadable_other_test.go index 6c0f987a4..10144fd89 100644 --- a/internal/tools/write_file_unreadable_other_test.go +++ b/internal/tools/write_file_unreadable_other_test.go @@ -7,10 +7,12 @@ import ( "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. +// 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) diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index 239086c5c..e55cac3d3 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -315,6 +315,100 @@ func TestWriteFileToolEncodingPreservationKeepsWholeFileObservation(t *testing.T } } +// Format-on-write runs after encoding preservation, and gofmt normalizes endings +// to LF. Preservation must be reapplied to the formatter's output, or the CRLF +// file comes out LF anyway and the whole-file observation is dropped, so the +// next overwrite is refused as unread. +func TestWriteFileToolEncodingPreservationSurvivesFormatOnWrite(t *testing.T) { + requireGofmt(t) + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + for _, tt := range []struct { + name, existing, bom string + }{ + {name: "CRLF", existing: "package a\r\n\r\nfunc Old() {}\r\n"}, + {name: "BOM and CRLF", existing: "\xef\xbb\xbfpackage a\r\n\r\nfunc Old() {}\r\n", bom: "\xef\xbb\xbf"}, + } { + t.Run(tt.name, func(t *testing.T) { + root := t.TempDir() + path := filepath.Join(root, "a.go") + if err := os.WriteFile(path, []byte(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": "a.go"}, options) + if read.Status != StatusOK { + t.Fatalf("initial read failed: %s", read.Output) + } + + // Already gofmt-clean, so the formatter's only change is the endings. + writeTool := NewScopedWriteFileTool(root, nil).(optionsAwareTool) + for _, name := range []string{"A", "B"} { + content := "package a\n\nfunc " + name + "() {}\n" + result := writeTool.RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", "content": content, "overwrite": true, + }, options) + if result.Status != StatusOK { + t.Fatalf("overwrite with func %s failed: %s", name, result.Output) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if want := tt.bom + "package a\r\n\r\nfunc " + name + "() {}\r\n"; string(got) != want { + t.Fatalf("written bytes = %q, want %q", got, want) + } + if !tracker.SeenWhole(trackedPath) { + t.Fatalf("format-on-write discarded the whole-file observation after writing func %s", name) + } + } + }) + } +} + +// A formatter that really changes the content still preserves the encoding, and +// still clears the whole-file observation, because the model has not seen the +// formatted bytes. +func TestWriteFileToolFormatterEditsKeepEncodingButRequireARead(t *testing.T) { + requireGofmt(t) + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + root := t.TempDir() + path := filepath.Join(root, "a.go") + if err := os.WriteFile(path, []byte("package a\r\n\r\nfunc Old() {}\r\n"), 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": "a.go"}, options) + if read.Status != StatusOK { + t.Fatalf("initial read failed: %s", read.Output) + } + result := NewScopedWriteFileTool(root, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", "content": "package a\n\nfunc A( ) { }\n", "overwrite": true, + }, options) + if result.Status != StatusOK { + t.Fatalf("overwrite failed: %s", result.Output) + } + got, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if want := "package a\r\n\r\nfunc A() {}\r\n"; string(got) != want { + t.Fatalf("written bytes = %q, want %q", got, want) + } + if tracker.SeenWhole(trackedPath) { + t.Fatal("formatter-changed content kept the whole-file observation") + } +} + func TestWriteFileToolNewFileRetainsCallerBytes(t *testing.T) { root := t.TempDir() want := []byte("\xef\xbb\xbfnew\r\ntext\n")