diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index 76f5f1baa..52c401103 100644 --- a/internal/tools/write_file.go +++ b/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 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 87849e859..6b5bfcca5 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,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()