From d50efd8f0c2ac2ea5279a6ec82aa8b4431f0c514 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 27 Aug 2026 22:18:09 +0200 Subject: [PATCH 1/3] fix(tools): preserve file encoding on overwrite Amp-Thread-ID: https://ampcode.com/threads/T-01a0448f-5860-721c-8a47-5119fc57f685 Co-authored-by: Amp --- 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 76f5f1baa..2b4e47c48 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -1,6 +1,7 @@ package tools import ( + "bytes" "context" "fmt" "os" @@ -95,11 +96,17 @@ 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. priorContent := "" + var priorBytes []byte if existed { if prev, rerr := os.ReadFile(absolutePath); rerr == nil { + priorBytes = prev priorContent = string(prev) } } + 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()) @@ -110,7 +117,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. @@ -147,6 +153,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 87849e859..f7ec67dcf 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" @@ -139,6 +140,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 20bf299b88a68a9183fbe2f10cab60f9f1e75a7f Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Fri, 28 Aug 2026 20:50:20 +0200 Subject: [PATCH 2/3] fix(tools): retain write observation after encoding restore --- 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 2b4e47c48..f4df59b45 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -103,10 +103,10 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an priorContent = string(prev) } } - 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()) @@ -125,7 +125,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 { diff --git a/internal/tools/write_tools_test.go b/internal/tools/write_tools_test.go index f7ec67dcf..33264043e 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -181,6 +181,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 f33ec58f3dc0aca6d00dae931ac9ce64a288fa71 Mon Sep 17 00:00:00 2001 From: PierrunoYT Date: Thu, 3 Sep 2026 21:27:42 +0200 Subject: [PATCH 3/3] 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 --- internal/tools/write_file.go | 20 ++++--- .../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, 141 insertions(+), 8 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 f4df59b45..52c401103 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -94,17 +94,21 @@ 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 := "" - var priorBytes []byte if existed { - if prev, rerr := os.ReadFile(absolutePath); rerr == nil { - priorBytes = prev - 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()) } - } - if priorBytes != nil { - content = preserveWriteFileEncoding(priorBytes, content) + priorContent = string(prev) + 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 33264043e..6b5bfcca5 100644 --- a/internal/tools/write_tools_test.go +++ b/internal/tools/write_tools_test.go @@ -246,6 +246,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()