From f794fc38fc38d7258b75b7c5ccbc224b2c553d0e Mon Sep 17 00:00:00 2001 From: cl-ment Date: Sun, 23 Aug 2026 09:25:27 +0200 Subject: [PATCH 01/10] fix(tools): use atomic temp-and-replace writes for write_file and edit_file (fixes #921) Direct in-place writes via os.WriteFile risk leaving target files empty or truncated if the process is cancelled, killed, or crashes mid-write. This introduces fsutil.WriteFileAtomic, which writes to an adjacent temporary file, flushes and syncs to disk, and replaces the target file via atomic rename using ReplaceWithRetry to handle transient Windows lock issues. --- internal/fsutil/rename.go | 39 +++++++++++++++++++++++++++++++ internal/fsutil/rename_test.go | 42 ++++++++++++++++++++++++---------- internal/tools/edit_file.go | 4 +++- internal/tools/write_file.go | 4 +++- 4 files changed, 75 insertions(+), 14 deletions(-) diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index 4f74e8544..97ef977e2 100644 --- a/internal/fsutil/rename.go +++ b/internal/fsutil/rename.go @@ -5,11 +5,50 @@ import ( "errors" "fmt" "os" + "path/filepath" "runtime" "syscall" "time" ) +// WriteFileAtomic writes data to a temporary file in the same directory as filename, +// flushes and syncs it to disk, and replaces filename atomically via ReplaceWithRetry. +func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { + dir := filepath.Dir(filename) + if err := os.MkdirAll(dir, 0o755); err != nil { + return err + } + tmpFile, err := os.CreateTemp(dir, ".zero-tmp-*") + if err != nil { + return err + } + tmpName := tmpFile.Name() + closed := false + defer func() { + if !closed { + _ = tmpFile.Close() + } + _ = os.Remove(tmpName) + }() + + if err := tmpFile.Chmod(perm); err != nil { + return err + } + if _, err := tmpFile.Write(data); err != nil { + return err + } + if err := tmpFile.Sync(); err != nil { + return err + } + closed = true + if err := tmpFile.Close(); err != nil { + return err + } + + return ReplaceWithRetry(tmpName, filename, nil) +} + + // CommittedReplacementCleanupError reports that a replacement was committed, // but the old destination retained at BackupPath could not be removed. Callers // must treat the replacement itself as successful and surface the cleanup diff --git a/internal/fsutil/rename_test.go b/internal/fsutil/rename_test.go index 8a64852d2..986bb643c 100644 --- a/internal/fsutil/rename_test.go +++ b/internal/fsutil/rename_test.go @@ -1,7 +1,8 @@ package fsutil import ( - "errors" + "os" + "path/filepath" "runtime" "syscall" "testing" @@ -31,17 +32,34 @@ func TestRenameWithRetryRetriesOnWindows(t *testing.T) { } } -func TestRenameWithRetryNonRetryableError(t *testing.T) { - sentinel := errors.New("disk on fire") - var attempts int - err := RenameWithRetry("src", "dst", func(src, dst string) error { - attempts++ - return sentinel - }) - if !errors.Is(err, sentinel) { - t.Fatalf("expected sentinel error, got: %v", err) +func TestWriteFileAtomic(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "sub", "test.txt") + content := []byte("hello atomic world") + + if err := WriteFileAtomic(target, content, 0o644); err != nil { + t.Fatalf("WriteFileAtomic failed: %v", err) + } + + read, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile failed: %v", err) + } + if string(read) != string(content) { + t.Fatalf("read content mismatch: got %q, want %q", string(read), string(content)) } - if attempts != 1 { - t.Errorf("expected only 1 attempt for non-retryable error, got %d", attempts) + + // Overwrite test + newContent := []byte("overwritten atomic content") + if err := WriteFileAtomic(target, newContent, 0o644); err != nil { + t.Fatalf("WriteFileAtomic overwrite failed: %v", err) + } + read2, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile after overwrite failed: %v", err) + } + if string(read2) != string(newContent) { + t.Fatalf("read content mismatch: got %q, want %q", string(read2), string(newContent)) } } + diff --git a/internal/tools/edit_file.go b/internal/tools/edit_file.go index fb92ecfed..3fc30caba 100644 --- a/internal/tools/edit_file.go +++ b/internal/tools/edit_file.go @@ -6,6 +6,8 @@ import ( "fmt" "os" "strings" + + "github.com/Gitlawb/zero/internal/fsutil" ) type editFileTool struct { @@ -153,7 +155,7 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any if err := recheckScopedWriteTarget(tool.workspaceRoot, tool.scope, requestedPath); err != nil { return errorResult("Error writing " + relativePath + ": " + err.Error()) } - if err := os.WriteFile(absolutePath, []byte(updated), 0o644); err != nil { + if err := fsutil.WriteFileAtomic(absolutePath, []byte(updated), 0o644); err != nil { return errorResult("Error writing " + relativePath + ": " + err.Error()) } modelKnownContent := updated diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index ab303eb68..3f0ce876f 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -6,6 +6,8 @@ import ( "os" "path/filepath" "strings" + + "github.com/Gitlawb/zero/internal/fsutil" ) type writeFileTool struct { @@ -107,7 +109,7 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an if err := recheckScopedWriteTarget(tool.workspaceRoot, tool.scope, requestedPath); err != nil { return errorResult("Error writing file " + relativePath + ": " + err.Error()) } - if err := os.WriteFile(absolutePath, []byte(content), 0o644); err != nil { + if err := fsutil.WriteFileAtomic(absolutePath, []byte(content), 0o644); err != nil { return errorResult("Error writing file " + relativePath + ": " + err.Error()) } modelKnownContent := content From ddd71ddcf358de82a02a1fd3544b54f7a7d9a2df Mon Sep 17 00:00:00 2001 From: cl-ment Date: Sun, 23 Aug 2026 10:15:18 +0200 Subject: [PATCH 02/10] fix(tools): preserve dest mode and treat committed cleanup as success WriteFileAtomic now keeps existing Unix permission bits on replace and only applies perm for a new file. A failed replace leaves the destination intact and removes the temp file. Callers surface CommittedReplacementCleanupError as a warning after re-baselining, not as a failed write. --- internal/fsutil/rename.go | 13 ++++++-- internal/fsutil/rename_test.go | 61 ++++++++++++++++++++++++++++++++++ internal/tools/atomic_write.go | 23 +++++++++++++ internal/tools/edit_file.go | 8 +++-- internal/tools/write_file.go | 8 +++-- 5 files changed, 105 insertions(+), 8 deletions(-) create mode 100644 internal/tools/atomic_write.go diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index 97ef977e2..cf85bd9d1 100644 --- a/internal/fsutil/rename.go +++ b/internal/fsutil/rename.go @@ -18,6 +18,16 @@ func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { if err := os.MkdirAll(dir, 0o755); err != nil { return err } + mode := perm + info, err := os.Lstat(filename) + switch { + case err == nil: + if info.Mode().IsRegular() { + mode = info.Mode().Perm() + } + case !os.IsNotExist(err): + return err + } tmpFile, err := os.CreateTemp(dir, ".zero-tmp-*") if err != nil { return err @@ -31,7 +41,7 @@ func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { _ = os.Remove(tmpName) }() - if err := tmpFile.Chmod(perm); err != nil { + if err := tmpFile.Chmod(mode); err != nil { return err } if _, err := tmpFile.Write(data); err != nil { @@ -48,7 +58,6 @@ func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { return ReplaceWithRetry(tmpName, filename, nil) } - // CommittedReplacementCleanupError reports that a replacement was committed, // but the old destination retained at BackupPath could not be removed. Callers // must treat the replacement itself as successful and surface the cleanup diff --git a/internal/fsutil/rename_test.go b/internal/fsutil/rename_test.go index 986bb643c..d91e1cc9e 100644 --- a/internal/fsutil/rename_test.go +++ b/internal/fsutil/rename_test.go @@ -63,3 +63,64 @@ func TestWriteFileAtomic(t *testing.T) { } } +func TestWriteFileAtomicPreservesExistingMode(t *testing.T) { + dir := t.TempDir() + cases := []os.FileMode{0o600, 0o755} + for _, want := range cases { + target := filepath.Join(dir, "mode-"+want.String()+".txt") + if err := os.WriteFile(target, []byte("old"), want); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if err := os.Chmod(target, want); err != nil { + t.Fatalf("Chmod: %v", err) + } + if err := WriteFileAtomic(target, []byte("new"), 0o644); err != nil { + t.Fatalf("WriteFileAtomic: %v", err) + } + info, err := os.Stat(target) + if err != nil { + t.Fatalf("Stat: %v", err) + } + if got := info.Mode().Perm(); got != want { + t.Fatalf("mode = %04o, want %04o", got, want) + } + got, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(got) != "new" { + t.Fatalf("content = %q, want %q", got, "new") + } + } +} + +func TestWriteFileAtomicLeavesDestinationOnReplaceFailure(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "target") + if err := os.Mkdir(target, 0o755); err != nil { + t.Fatalf("Mkdir: %v", err) + } + marker := filepath.Join(target, "keep.txt") + if err := os.WriteFile(marker, []byte("keep"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + if err := WriteFileAtomic(target, []byte("should-not-land"), 0o644); err == nil { + t.Fatal("expected replace failure when destination is a directory") + } + + got, err := os.ReadFile(marker) + if err != nil { + t.Fatalf("original destination was disturbed: %v", err) + } + if string(got) != "keep" { + t.Fatalf("marker = %q, want keep", got) + } + leftovers, err := filepath.Glob(filepath.Join(dir, ".zero-tmp-*")) + if err != nil { + t.Fatalf("Glob: %v", err) + } + if len(leftovers) != 0 { + t.Fatalf("temporary files left behind: %v", leftovers) + } +} diff --git a/internal/tools/atomic_write.go b/internal/tools/atomic_write.go new file mode 100644 index 000000000..2f8d688a9 --- /dev/null +++ b/internal/tools/atomic_write.go @@ -0,0 +1,23 @@ +package tools + +import ( + "errors" + "os" + + "github.com/Gitlawb/zero/internal/fsutil" +) + +// committedWrite publishes data via WriteFileAtomic. A committed replacement +// whose backup cleanup failed is treated as a successful write; the warning is +// returned for the caller to surface without flipping the tool status to error. +func committedWrite(path string, data []byte, perm os.FileMode) (string, error) { + err := fsutil.WriteFileAtomic(path, data, perm) + if err == nil { + return "", nil + } + var committed *fsutil.CommittedReplacementCleanupError + if errors.As(err, &committed) { + return committed.Error(), nil + } + return "", err +} diff --git a/internal/tools/edit_file.go b/internal/tools/edit_file.go index 3fc30caba..8c242484e 100644 --- a/internal/tools/edit_file.go +++ b/internal/tools/edit_file.go @@ -6,8 +6,6 @@ import ( "fmt" "os" "strings" - - "github.com/Gitlawb/zero/internal/fsutil" ) type editFileTool struct { @@ -155,7 +153,8 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any if err := recheckScopedWriteTarget(tool.workspaceRoot, tool.scope, requestedPath); err != nil { return errorResult("Error writing " + relativePath + ": " + err.Error()) } - if err := fsutil.WriteFileAtomic(absolutePath, []byte(updated), 0o644); err != nil { + cleanupWarning, err := committedWrite(absolutePath, []byte(updated), 0o644) + if err != nil { return errorResult("Error writing " + relativePath + ": " + err.Error()) } modelKnownContent := updated @@ -197,6 +196,9 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any } summary := fmt.Sprintf("Successfully edited %s (replaced %d occurrence%s).", relativePath, replacedCount, suffix) summary += formatting.notice(relativePath) + if cleanupWarning != "" { + summary += " " + cleanupWarning + } summary += inlineDiagnostics(ctx, options, absolutePath, relativePath) result := okResult(summary) result.ChangedFiles = []string{relativePath} diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index 3f0ce876f..7f708894d 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -6,8 +6,6 @@ import ( "os" "path/filepath" "strings" - - "github.com/Gitlawb/zero/internal/fsutil" ) type writeFileTool struct { @@ -109,7 +107,8 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an if err := recheckScopedWriteTarget(tool.workspaceRoot, tool.scope, requestedPath); err != nil { return errorResult("Error writing file " + relativePath + ": " + err.Error()) } - if err := fsutil.WriteFileAtomic(absolutePath, []byte(content), 0o644); err != nil { + cleanupWarning, err := committedWrite(absolutePath, []byte(content), 0o644) + if err != nil { return errorResult("Error writing file " + relativePath + ": " + err.Error()) } modelKnownContent := content @@ -141,6 +140,9 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an } summary := fmt.Sprintf("%s %s (%d lines).", verb, relativePath, lines) summary += formatting.notice(relativePath) + if cleanupWarning != "" { + summary += " " + cleanupWarning + } summary += inlineDiagnostics(ctx, options, absolutePath, relativePath) result := okResult(summary) result.ChangedFiles = []string{relativePath} From cffebdc6f26582546bff37549dfa2fe33854a726 Mon Sep 17 00:00:00 2001 From: cl-ment Date: Mon, 24 Aug 2026 18:28:53 +0200 Subject: [PATCH 03/10] fix(fsutil): sync directory best-effort on replacement, adapt mode assertion on Windows and restore non-retryable test --- internal/fsutil/rename.go | 20 +++++++++++++++++++- internal/fsutil/rename_test.go | 25 +++++++++++++++++++++++-- internal/tools/atomic_write.go | 2 +- 3 files changed, 43 insertions(+), 4 deletions(-) diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index cf85bd9d1..9e6336a02 100644 --- a/internal/fsutil/rename.go +++ b/internal/fsutil/rename.go @@ -55,7 +55,25 @@ func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { return err } - return ReplaceWithRetry(tmpName, filename, nil) + replaceErr := ReplaceWithRetry(tmpName, filename, nil) + if replaceErr == nil || isCommittedReplacement(replaceErr) { + syncDir(dir) + } + return replaceErr +} + +func isCommittedReplacement(err error) bool { + var committed *CommittedReplacementCleanupError + return errors.As(err, &committed) +} + +func syncDir(dir string) { + d, err := os.Open(dir) + if err != nil { + return + } + defer d.Close() + _ = d.Sync() } // CommittedReplacementCleanupError reports that a replacement was committed, diff --git a/internal/fsutil/rename_test.go b/internal/fsutil/rename_test.go index d91e1cc9e..ae887d5b3 100644 --- a/internal/fsutil/rename_test.go +++ b/internal/fsutil/rename_test.go @@ -1,6 +1,7 @@ package fsutil import ( + "errors" "os" "path/filepath" "runtime" @@ -74,6 +75,11 @@ func TestWriteFileAtomicPreservesExistingMode(t *testing.T) { if err := os.Chmod(target, want); err != nil { t.Fatalf("Chmod: %v", err) } + initialInfo, err := os.Stat(target) + if err != nil { + t.Fatalf("Stat initial: %v", err) + } + expectedMode := initialInfo.Mode().Perm() if err := WriteFileAtomic(target, []byte("new"), 0o644); err != nil { t.Fatalf("WriteFileAtomic: %v", err) } @@ -81,8 +87,8 @@ func TestWriteFileAtomicPreservesExistingMode(t *testing.T) { if err != nil { t.Fatalf("Stat: %v", err) } - if got := info.Mode().Perm(); got != want { - t.Fatalf("mode = %04o, want %04o", got, want) + if got := info.Mode().Perm(); got != expectedMode { + t.Fatalf("mode = %04o, want %04o", got, expectedMode) } got, err := os.ReadFile(target) if err != nil { @@ -94,6 +100,21 @@ func TestWriteFileAtomicPreservesExistingMode(t *testing.T) { } } +func TestRenameWithRetryNonRetryableError(t *testing.T) { + var attempts int + expectedErr := os.ErrInvalid + err := RenameWithRetry("src", "dst", func(src, dst string) error { + attempts++ + return expectedErr + }) + if !errors.Is(err, expectedErr) { + t.Fatalf("got err %v, want %v", err, expectedErr) + } + if attempts != 1 { + t.Fatalf("attempts = %d, want 1", attempts) + } +} + func TestWriteFileAtomicLeavesDestinationOnReplaceFailure(t *testing.T) { dir := t.TempDir() target := filepath.Join(dir, "target") diff --git a/internal/tools/atomic_write.go b/internal/tools/atomic_write.go index 2f8d688a9..9ace1349b 100644 --- a/internal/tools/atomic_write.go +++ b/internal/tools/atomic_write.go @@ -17,7 +17,7 @@ func committedWrite(path string, data []byte, perm os.FileMode) (string, error) } var committed *fsutil.CommittedReplacementCleanupError if errors.As(err, &committed) { - return committed.Error(), nil + return "replacement committed, but backup cleanup failed", nil } return "", err } From c77e4bde30af950c5621dfbfe9bf18108699f977 Mon Sep 17 00:00:00 2001 From: cl-ment Date: Mon, 24 Aug 2026 18:45:38 +0200 Subject: [PATCH 04/10] fix(fsutil): honor process umask for new destination files in WriteFileAtomic --- internal/fsutil/rename.go | 34 +++++++++++++++++++++++++++++----- internal/fsutil/rename_test.go | 27 +++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index 9e6336a02..b8c5baadd 100644 --- a/internal/fsutil/rename.go +++ b/internal/fsutil/rename.go @@ -2,6 +2,7 @@ package fsutil import ( + "crypto/rand" "errors" "fmt" "os" @@ -13,22 +14,27 @@ import ( // WriteFileAtomic writes data to a temporary file in the same directory as filename, // flushes and syncs it to disk, and replaces filename atomically via ReplaceWithRetry. +// For new files, it honors the process umask by creating the temporary file with +// os.OpenFile(..., perm). For existing regular files, it preserves the existing file mode. func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { dir := filepath.Dir(filename) if err := os.MkdirAll(dir, 0o755); err != nil { return err } - mode := perm + + var existingMode *os.FileMode info, err := os.Lstat(filename) switch { case err == nil: if info.Mode().IsRegular() { - mode = info.Mode().Perm() + m := info.Mode().Perm() + existingMode = &m } case !os.IsNotExist(err): return err } - tmpFile, err := os.CreateTemp(dir, ".zero-tmp-*") + + tmpFile, err := createTempFile(dir, perm) if err != nil { return err } @@ -41,8 +47,10 @@ func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { _ = os.Remove(tmpName) }() - if err := tmpFile.Chmod(mode); err != nil { - return err + if existingMode != nil { + if err := tmpFile.Chmod(*existingMode); err != nil { + return err + } } if _, err := tmpFile.Write(data); err != nil { return err @@ -62,6 +70,22 @@ func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { return replaceErr } +func createTempFile(dir string, perm os.FileMode) (*os.File, error) { + for i := 0; i < 10000; i++ { + var b [8]byte + if _, err := rand.Read(b[:]); err != nil { + return nil, err + } + name := filepath.Join(dir, fmt.Sprintf(".zero-tmp-%x", b)) + f, err := os.OpenFile(name, os.O_RDWR|os.O_CREATE|os.O_EXCL, perm) + if errors.Is(err, os.ErrExist) { + continue + } + return f, err + } + return nil, errors.New("fsutil: failed to create temporary file after repeated attempts") +} + func isCommittedReplacement(err error) bool { var committed *CommittedReplacementCleanupError return errors.As(err, &committed) diff --git a/internal/fsutil/rename_test.go b/internal/fsutil/rename_test.go index ae887d5b3..af19f7f8a 100644 --- a/internal/fsutil/rename_test.go +++ b/internal/fsutil/rename_test.go @@ -100,6 +100,33 @@ func TestWriteFileAtomicPreservesExistingMode(t *testing.T) { } } +func TestWriteFileAtomicRespectsProcessUmask(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("umask semantics do not apply on Windows") + } + // Temporarily set umask to 0o077 + oldMask := syscall.Umask(0o077) + defer syscall.Umask(oldMask) + + dir := t.TempDir() + target := filepath.Join(dir, "umask_test.txt") + + // Write new file with 0o644 requested perm + if err := WriteFileAtomic(target, []byte("umask test"), 0o644); err != nil { + t.Fatalf("WriteFileAtomic: %v", err) + } + + info, err := os.Stat(target) + if err != nil { + t.Fatalf("Stat: %v", err) + } + + // Under umask 0o077, 0o644 & ~0o077 = 0o600 + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("created file perm = %04o, want %04o (honoring umask 0o077)", got, 0o600) + } +} + func TestRenameWithRetryNonRetryableError(t *testing.T) { var attempts int expectedErr := os.ErrInvalid From f978e8813c01f806d95a831d14b1763e918bd3fd Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Thu, 27 Aug 2026 15:13:11 +0200 Subject: [PATCH 05/10] fix(fsutil): isolate umask test under !windows build tag --- internal/fsutil/rename_test.go | 27 ------------------ internal/fsutil/rename_umask_unix_test.go | 34 +++++++++++++++++++++++ 2 files changed, 34 insertions(+), 27 deletions(-) create mode 100644 internal/fsutil/rename_umask_unix_test.go diff --git a/internal/fsutil/rename_test.go b/internal/fsutil/rename_test.go index af19f7f8a..ae887d5b3 100644 --- a/internal/fsutil/rename_test.go +++ b/internal/fsutil/rename_test.go @@ -100,33 +100,6 @@ func TestWriteFileAtomicPreservesExistingMode(t *testing.T) { } } -func TestWriteFileAtomicRespectsProcessUmask(t *testing.T) { - if runtime.GOOS == "windows" { - t.Skip("umask semantics do not apply on Windows") - } - // Temporarily set umask to 0o077 - oldMask := syscall.Umask(0o077) - defer syscall.Umask(oldMask) - - dir := t.TempDir() - target := filepath.Join(dir, "umask_test.txt") - - // Write new file with 0o644 requested perm - if err := WriteFileAtomic(target, []byte("umask test"), 0o644); err != nil { - t.Fatalf("WriteFileAtomic: %v", err) - } - - info, err := os.Stat(target) - if err != nil { - t.Fatalf("Stat: %v", err) - } - - // Under umask 0o077, 0o644 & ~0o077 = 0o600 - if got := info.Mode().Perm(); got != 0o600 { - t.Fatalf("created file perm = %04o, want %04o (honoring umask 0o077)", got, 0o600) - } -} - func TestRenameWithRetryNonRetryableError(t *testing.T) { var attempts int expectedErr := os.ErrInvalid diff --git a/internal/fsutil/rename_umask_unix_test.go b/internal/fsutil/rename_umask_unix_test.go new file mode 100644 index 000000000..eecc53a95 --- /dev/null +++ b/internal/fsutil/rename_umask_unix_test.go @@ -0,0 +1,34 @@ +//go:build !windows + +package fsutil + +import ( + "os" + "path/filepath" + "syscall" + "testing" +) + +func TestWriteFileAtomicRespectsProcessUmask(t *testing.T) { + // Temporarily set umask to 0o077 + oldMask := syscall.Umask(0o077) + defer syscall.Umask(oldMask) + + dir := t.TempDir() + target := filepath.Join(dir, "umask_test.txt") + + // Write new file with 0o644 requested perm + if err := WriteFileAtomic(target, []byte("umask test"), 0o644); err != nil { + t.Fatalf("WriteFileAtomic: %v", err) + } + + info, err := os.Stat(target) + if err != nil { + t.Fatalf("Stat: %v", err) + } + + // Under umask 0o077, 0o644 & ~0o077 = 0o600 + if got := info.Mode().Perm(); got != 0o600 { + t.Fatalf("created file perm = %04o, want %04o (honoring umask 0o077)", got, 0o600) + } +} From 679ec0b7eb675a0de0e246506ac4c1b81a73ccee Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Fri, 28 Aug 2026 15:42:58 +0200 Subject: [PATCH 06/10] docs(fsutil): document symlink platform differences and hardlink behavior in WriteFileAtomic --- internal/fsutil/rename.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index b8c5baadd..8cac7f8f6 100644 --- a/internal/fsutil/rename.go +++ b/internal/fsutil/rename.go @@ -16,6 +16,11 @@ import ( // flushes and syncs it to disk, and replaces filename atomically via ReplaceWithRetry. // For new files, it honors the process umask by creating the temporary file with // os.OpenFile(..., perm). For existing regular files, it preserves the existing file mode. +// +// Platform differences on symlink destinations: On Unix (Linux/macOS), replacing a +// symlink destination replaces the symlink itself with the new regular file. On Windows, +// ReplaceFileW refuses symlink destinations outright and returns an error. +// Hard links to destination files are broken by design (temp-and-rename publishes a new inode). func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { dir := filepath.Dir(filename) if err := os.MkdirAll(dir, 0o755); err != nil { From fef69289b77ebd0033fedc2748b1e98ecc35b46d Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Sat, 29 Aug 2026 10:15:00 +0200 Subject: [PATCH 07/10] fix(fsutil): chown the temp file to the destination owner on Unix Rename publishes a new inode. Copying only Perm() would leave the writer as owner. posixChown applies the Lstat uid/gid before publish. --- internal/fsutil/rename.go | 11 ++++++ internal/fsutil/rename_owner_unix_test.go | 41 +++++++++++++++++++++++ 2 files changed, 52 insertions(+) create mode 100644 internal/fsutil/rename_owner_unix_test.go diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index 8cac7f8f6..fc181fdb0 100644 --- a/internal/fsutil/rename.go +++ b/internal/fsutil/rename.go @@ -12,6 +12,10 @@ import ( "time" ) +var posixChown = func(f *os.File, uid, gid int) error { + return f.Chown(uid, gid) +} + // WriteFileAtomic writes data to a temporary file in the same directory as filename, // flushes and syncs it to disk, and replaces filename atomically via ReplaceWithRetry. // For new files, it honors the process umask by creating the temporary file with @@ -56,6 +60,13 @@ func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { if err := tmpFile.Chmod(*existingMode); err != nil { return err } + if stat, ok := info.Sys().(*syscall.Stat_t); ok { + if err := posixChown(tmpFile, int(stat.Uid), int(stat.Gid)); err != nil { + if int(stat.Uid) != os.Getuid() || int(stat.Gid) != os.Getgid() { + return err + } + } + } } if _, err := tmpFile.Write(data); err != nil { return err diff --git a/internal/fsutil/rename_owner_unix_test.go b/internal/fsutil/rename_owner_unix_test.go new file mode 100644 index 000000000..ae813cc82 --- /dev/null +++ b/internal/fsutil/rename_owner_unix_test.go @@ -0,0 +1,41 @@ +//go:build !windows + +package fsutil + +import ( + "os" + "path/filepath" + "syscall" + "testing" +) + +func TestWriteFileAtomicCallsChownWithDestOwner(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "dest") + if err := os.WriteFile(path, []byte("old"), 0o644); err != nil { + t.Fatal(err) + } + info, err := os.Lstat(path) + if err != nil { + t.Fatal(err) + } + stat := info.Sys().(*syscall.Stat_t) + var gotUID, gotGID int + called := false + orig := posixChown + t.Cleanup(func() { posixChown = orig }) + posixChown = func(f *os.File, uid, gid int) error { + called = true + gotUID, gotGID = uid, gid + return orig(f, uid, gid) + } + if err := WriteFileAtomic(path, []byte("new"), 0o600); err != nil { + t.Fatal(err) + } + if !called { + t.Fatal("posixChown was not called; owner would be the writer inode") + } + if gotUID != int(stat.Uid) || gotGID != int(stat.Gid) { + t.Fatalf("chown uid/gid = %d/%d, want %d/%d", gotUID, gotGID, stat.Uid, stat.Gid) + } +} From 64e262df1e1c9effc7f1c519624fb298a648ae02 Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Sat, 29 Aug 2026 10:18:13 +0200 Subject: [PATCH 08/10] fix(fsutil): keep Unix owner copy off the Windows build syscall.Stat_t is not defined for GOOS=windows. preserveOwner lives in rename_owner_unix.go; the Windows stub is a no-op. --- internal/fsutil/rename.go | 12 ++---------- internal/fsutil/rename_owner_unix.go | 25 +++++++++++++++++++++++++ internal/fsutil/rename_owner_windows.go | 9 +++++++++ 3 files changed, 36 insertions(+), 10 deletions(-) create mode 100644 internal/fsutil/rename_owner_unix.go create mode 100644 internal/fsutil/rename_owner_windows.go diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index fc181fdb0..0d8cfcf7a 100644 --- a/internal/fsutil/rename.go +++ b/internal/fsutil/rename.go @@ -12,10 +12,6 @@ import ( "time" ) -var posixChown = func(f *os.File, uid, gid int) error { - return f.Chown(uid, gid) -} - // WriteFileAtomic writes data to a temporary file in the same directory as filename, // flushes and syncs it to disk, and replaces filename atomically via ReplaceWithRetry. // For new files, it honors the process umask by creating the temporary file with @@ -60,12 +56,8 @@ func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { if err := tmpFile.Chmod(*existingMode); err != nil { return err } - if stat, ok := info.Sys().(*syscall.Stat_t); ok { - if err := posixChown(tmpFile, int(stat.Uid), int(stat.Gid)); err != nil { - if int(stat.Uid) != os.Getuid() || int(stat.Gid) != os.Getgid() { - return err - } - } + if err := preserveOwner(tmpFile, info); err != nil { + return err } } if _, err := tmpFile.Write(data); err != nil { diff --git a/internal/fsutil/rename_owner_unix.go b/internal/fsutil/rename_owner_unix.go new file mode 100644 index 000000000..42ce0716d --- /dev/null +++ b/internal/fsutil/rename_owner_unix.go @@ -0,0 +1,25 @@ +//go:build !windows + +package fsutil + +import ( + "os" + "syscall" +) + +var posixChown = func(f *os.File, uid, gid int) error { + return f.Chown(uid, gid) +} + +func preserveOwner(f *os.File, info os.FileInfo) error { + stat, ok := info.Sys().(*syscall.Stat_t) + if !ok { + return nil + } + if err := posixChown(f, int(stat.Uid), int(stat.Gid)); err != nil { + if int(stat.Uid) != os.Getuid() || int(stat.Gid) != os.Getgid() { + return err + } + } + return nil +} diff --git a/internal/fsutil/rename_owner_windows.go b/internal/fsutil/rename_owner_windows.go new file mode 100644 index 000000000..533e5b919 --- /dev/null +++ b/internal/fsutil/rename_owner_windows.go @@ -0,0 +1,9 @@ +//go:build windows + +package fsutil + +import "os" + +func preserveOwner(*os.File, os.FileInfo) error { + return nil +} From 5b68cd98527906c23780fe3dffd75aa9522fccb4 Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Sun, 30 Aug 2026 17:01:52 +0200 Subject: [PATCH 09/10] fix(fsutil): preserve ACL/xattrs, format before publish, refuse special files --- internal/fsutil/rename.go | 36 ++++- internal/fsutil/rename_acl_linux_test.go | 65 ++++++++ internal/fsutil/rename_owner_windows.go | 4 + internal/fsutil/rename_special_unix_test.go | 42 +++++ internal/fsutil/rename_test.go | 35 +++++ internal/fsutil/rename_xattr_stub.go | 9 ++ internal/fsutil/rename_xattr_unix.go | 95 +++++++++++ internal/tools/edit_file.go | 13 +- internal/tools/format_on_write.go | 60 ++++--- internal/tools/format_on_write_test.go | 165 ++++++++++++++++++++ internal/tools/write_file.go | 13 +- 11 files changed, 489 insertions(+), 48 deletions(-) create mode 100644 internal/fsutil/rename_acl_linux_test.go create mode 100644 internal/fsutil/rename_special_unix_test.go create mode 100644 internal/fsutil/rename_xattr_stub.go create mode 100644 internal/fsutil/rename_xattr_unix.go diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index 0d8cfcf7a..c865eb665 100644 --- a/internal/fsutil/rename.go +++ b/internal/fsutil/rename.go @@ -12,10 +12,19 @@ import ( "time" ) +// ErrNonRegularDestination is returned when WriteFileAtomic would replace a +// FIFO, device, socket, directory, or other special file with a regular file. +var ErrNonRegularDestination = errors.New("fsutil: destination exists and is not a regular file") + // WriteFileAtomic writes data to a temporary file in the same directory as filename, // flushes and syncs it to disk, and replaces filename atomically via ReplaceWithRetry. // For new files, it honors the process umask by creating the temporary file with -// os.OpenFile(..., perm). For existing regular files, it preserves the existing file mode. +// os.OpenFile(..., perm). For existing regular files, it requires write access to +// the current destination and copies that destination's authorization metadata onto +// the replacement (mode bits including setuid/setgid/sticky, owner, and xattrs such +// as POSIX ACLs and capabilities). If that metadata cannot be preserved, the call +// fails and leaves the destination unchanged. Non-regular, non-symlink destinations +// are refused before staging so a FIFO, device, or socket is not replaced. // // Platform differences on symlink destinations: On Unix (Linux/macOS), replacing a // symlink destination replaces the symlink itself with the new regular file. On Windows, @@ -31,9 +40,17 @@ func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { info, err := os.Lstat(filename) switch { case err == nil: - if info.Mode().IsRegular() { - m := info.Mode().Perm() - existingMode = &m + mode := info.Mode() + switch { + case mode.IsRegular(): + if err := ensureWritable(filename); err != nil { + return err + } + existingMode = &mode + case mode&os.ModeSymlink != 0: + // Documented platform-specific replacement of the symlink itself. + default: + return fmt.Errorf("%w: %s", ErrNonRegularDestination, filename) } case !os.IsNotExist(err): return err @@ -59,6 +76,9 @@ func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { if err := preserveOwner(tmpFile, info); err != nil { return err } + if err := preserveXattrs(tmpFile, filename); err != nil { + return err + } } if _, err := tmpFile.Write(data); err != nil { return err @@ -78,6 +98,14 @@ func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { return replaceErr } +func ensureWritable(path string) error { + f, err := os.OpenFile(path, os.O_WRONLY, 0) + if err != nil { + return err + } + return f.Close() +} + func createTempFile(dir string, perm os.FileMode) (*os.File, error) { for i := 0; i < 10000; i++ { var b [8]byte diff --git a/internal/fsutil/rename_acl_linux_test.go b/internal/fsutil/rename_acl_linux_test.go new file mode 100644 index 000000000..e1dac2669 --- /dev/null +++ b/internal/fsutil/rename_acl_linux_test.go @@ -0,0 +1,65 @@ +//go:build linux + +package fsutil + +import ( + "bytes" + "os" + "os/exec" + "path/filepath" + "testing" +) + +func TestWriteFileAtomicPreservesRestrictivePOSIXACL(t *testing.T) { + if _, err := exec.LookPath("setfacl"); err != nil { + t.Skip("setfacl not on PATH; POSIX ACL preservation is not exercised on this host") + } + if _, err := exec.LookPath("getfacl"); err != nil { + t.Skip("getfacl not on PATH; POSIX ACL preservation is not exercised on this host") + } + + dir := t.TempDir() + target := filepath.Join(dir, "restricted.txt") + if err := os.WriteFile(target, []byte("old"), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + set := exec.Command("setfacl", "-m", "u:65534:---", target) + if out, err := set.CombinedOutput(); err != nil { + t.Skipf("setfacl failed (filesystem may lack ACL support): %v\n%s", err, out) + } + + before, err := exec.Command("getfacl", "-cp", target).CombinedOutput() + if err != nil { + t.Fatalf("getfacl before: %v\n%s", err, before) + } + if !namedUserACLDenied(before) { + t.Skipf("named-user ACL did not stick; getfacl:\n%s", before) + } + + if err := WriteFileAtomic(target, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFileAtomic: %v", err) + } + + got, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(got) != "new" { + t.Fatalf("content = %q, want %q", got, "new") + } + + after, err := exec.Command("getfacl", "-cp", target).CombinedOutput() + if err != nil { + t.Fatalf("getfacl after: %v\n%s", err, after) + } + if !namedUserACLDenied(after) { + t.Fatalf("restrictive named-user ACL was lost after replacement\nbefore:\n%s\nafter:\n%s", before, after) + } +} + +func namedUserACLDenied(listing []byte) bool { + return bytes.Contains(listing, []byte("user:65534:---")) || + bytes.Contains(listing, []byte("user:nobody:---")) || + bytes.Contains(listing, []byte("user:nfsnobody:---")) +} diff --git a/internal/fsutil/rename_owner_windows.go b/internal/fsutil/rename_owner_windows.go index 533e5b919..6bafcc4ad 100644 --- a/internal/fsutil/rename_owner_windows.go +++ b/internal/fsutil/rename_owner_windows.go @@ -7,3 +7,7 @@ import "os" func preserveOwner(*os.File, os.FileInfo) error { return nil } + +func preserveXattrs(*os.File, string) error { + return nil +} diff --git a/internal/fsutil/rename_special_unix_test.go b/internal/fsutil/rename_special_unix_test.go new file mode 100644 index 000000000..dfeeebb3a --- /dev/null +++ b/internal/fsutil/rename_special_unix_test.go @@ -0,0 +1,42 @@ +//go:build !windows + +package fsutil + +import ( + "errors" + "os" + "path/filepath" + "syscall" + "testing" +) + +func TestWriteFileAtomicRefusesNamedPipe(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "endpoint.fifo") + if err := syscall.Mkfifo(target, 0o644); err != nil { + t.Fatalf("mkfifo: %v", err) + } + + err := WriteFileAtomic(target, []byte("should-not-replace-fifo"), 0o644) + if err == nil { + t.Fatal("expected WriteFileAtomic to refuse a FIFO destination") + } + if !errors.Is(err, ErrNonRegularDestination) { + t.Fatalf("error = %v, want ErrNonRegularDestination", err) + } + + info, err := os.Lstat(target) + if err != nil { + t.Fatalf("Lstat: %v", err) + } + if info.Mode()&os.ModeNamedPipe == 0 { + t.Fatalf("FIFO was replaced; mode = %s", info.Mode()) + } + leftovers, err := filepath.Glob(filepath.Join(dir, ".zero-tmp-*")) + if err != nil { + t.Fatalf("Glob: %v", err) + } + if len(leftovers) != 0 { + t.Fatalf("temporary files left behind: %v", leftovers) + } +} diff --git a/internal/fsutil/rename_test.go b/internal/fsutil/rename_test.go index ae887d5b3..bda089097 100644 --- a/internal/fsutil/rename_test.go +++ b/internal/fsutil/rename_test.go @@ -145,3 +145,38 @@ func TestWriteFileAtomicLeavesDestinationOnReplaceFailure(t *testing.T) { t.Fatalf("temporary files left behind: %v", leftovers) } } + +func TestWriteFileAtomicRefusesNonWritableTarget(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "readonly.txt") + if err := os.WriteFile(target, []byte("original"), 0o444); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if err := os.Chmod(target, 0o444); err != nil { + t.Fatalf("Chmod: %v", err) + } + probe, err := os.OpenFile(target, os.O_WRONLY, 0) + if err == nil { + _ = probe.Close() + t.Skip("this host allows writing a mode-0444 file (for example when running as root)") + } + + if err := WriteFileAtomic(target, []byte("replaced"), 0o644); err == nil { + t.Fatal("expected WriteFileAtomic to refuse a non-writable destination") + } + + got, err := os.ReadFile(target) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + if string(got) != "original" { + t.Fatalf("content = %q, want %q", got, "original") + } + info, err := os.Stat(target) + if err != nil { + t.Fatalf("Stat: %v", err) + } + if got := info.Mode().Perm(); got&0o222 != 0 { + t.Fatalf("destination became writable: perm=%04o", got) + } +} diff --git a/internal/fsutil/rename_xattr_stub.go b/internal/fsutil/rename_xattr_stub.go new file mode 100644 index 000000000..79a39002f --- /dev/null +++ b/internal/fsutil/rename_xattr_stub.go @@ -0,0 +1,9 @@ +//go:build !windows && !linux && !darwin && !freebsd && !netbsd + +package fsutil + +import "os" + +func preserveXattrs(*os.File, string) error { + return nil +} diff --git a/internal/fsutil/rename_xattr_unix.go b/internal/fsutil/rename_xattr_unix.go new file mode 100644 index 000000000..be9008207 --- /dev/null +++ b/internal/fsutil/rename_xattr_unix.go @@ -0,0 +1,95 @@ +//go:build linux || darwin || freebsd || netbsd + +package fsutil + +import ( + "errors" + "fmt" + "os" + + "golang.org/x/sys/unix" +) + +func preserveXattrs(f *os.File, srcPath string) error { + names, err := listXattrs(srcPath) + if err != nil { + if isXattrUnsupported(err) { + return nil + } + return fmt.Errorf("fsutil: listing xattrs of %s: %w", srcPath, err) + } + for _, name := range names { + data, err := getXattr(srcPath, name) + if err != nil { + if isXattrUnsupported(err) { + continue + } + return fmt.Errorf("fsutil: reading xattr %s from %s: %w", name, srcPath, err) + } + if err := unix.Fsetxattr(int(f.Fd()), name, data, 0); err != nil { + if name == "security.selinux" { + continue + } + return fmt.Errorf("fsutil: preserving xattr %s: %w", name, err) + } + } + return nil +} + +func listXattrs(path string) ([]string, error) { + dest := []byte(nil) + for { + size, err := unix.Listxattr(path, dest) + if err != nil { + return nil, err + } + if size == 0 { + return nil, nil + } + if len(dest) < size { + dest = make([]byte, size) + continue + } + return splitXattrNames(dest[:size]), nil + } +} + +func getXattr(path, name string) ([]byte, error) { + dest := []byte(nil) + for { + size, err := unix.Getxattr(path, name, dest) + if err != nil { + return nil, err + } + if size == 0 { + return []byte{}, nil + } + if len(dest) < size { + dest = make([]byte, size) + continue + } + return dest[:size], nil + } +} + +func splitXattrNames(buf []byte) []string { + names := make([]string, 0) + start := 0 + for i, b := range buf { + if b != 0 { + continue + } + if i > start { + names = append(names, string(buf[start:i])) + } + start = i + 1 + } + if start < len(buf) { + names = append(names, string(buf[start:])) + } + return names +} + +func isXattrUnsupported(err error) bool { + return errors.Is(err, unix.ENOTSUP) || errors.Is(err, unix.EOPNOTSUPP) +} diff --git a/internal/tools/edit_file.go b/internal/tools/edit_file.go index 8c242484e..cf2338e2d 100644 --- a/internal/tools/edit_file.go +++ b/internal/tools/edit_file.go @@ -153,16 +153,17 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any if err := recheckScopedWriteTarget(tool.workspaceRoot, tool.scope, requestedPath); err != nil { return errorResult("Error writing " + relativePath + ": " + err.Error()) } + modelKnownContent := updated + // Optional format-on-write (ZERO_FORMAT_ON_WRITE). Format staged bytes, then + // publish once. Recording pre-format content would make the next edit look + // like an external modification and trip the conflict guard; formatting the + // destination in place after publication would reintroduce partial writes. + formatting := maybeFormatWrittenFile(ctx, absolutePath, updated) + updated = formatting.Content cleanupWarning, err := committedWrite(absolutePath, []byte(updated), 0o644) if err != nil { return errorResult("Error writing " + relativePath + ": " + err.Error()) } - modelKnownContent := updated - // Optional format-on-write (ZERO_FORMAT_ON_WRITE). Must run BEFORE the - // FileTracker re-baseline: recording pre-format content would make the very - // next edit look like an external modification and trip the conflict guard. - formatting := maybeFormatWrittenFile(ctx, absolutePath, updated) - updated = formatting.Content // Re-baseline to the content we just wrote so subsequent edits in this session // compare against the current on-disk state, not the pre-edit version. newInfo, _ := os.Stat(absolutePath) diff --git a/internal/tools/format_on_write.go b/internal/tools/format_on_write.go index 33bbba952..1f64f9a1e 100644 --- a/internal/tools/format_on_write.go +++ b/internal/tools/format_on_write.go @@ -11,16 +11,16 @@ import ( ) // Format-on-write for the mutating file tools. When enabled, a successful -// edit_file/write_file runs the language's standard formatter on the file it -// just wrote, so the model's output always lands in project-canonical style +// edit_file/write_file formats staged content before the single atomic +// publication, so the model's output always lands in project-canonical style // and never fails a CI format check it cannot see. Off by default (set // ZERO_FORMAT_ON_WRITE=1): auto-reformatting changes bytes the model did not // write, which strict workflows may not want. // -// Ordering matters: formatting runs BEFORE the FileTracker re-baseline, and -// the caller records the POST-format content. Formatting after the baseline -// would make the very next edit look like an external modification and trip -// the conflict guard. +// Ordering matters: formatting runs on a sibling temporary file BEFORE +// publication and BEFORE the FileTracker re-baseline. The caller records the +// POST-format content that was actually published. Formatting the destination +// in place after publication would reintroduce partial-file writes. // formatOnWriteTimeout bounds one formatter run; a wedged formatter must never // hang a tool call. On timeout the unformatted write stands, and the caller @@ -117,8 +117,9 @@ func formatOnWriteEnabled() bool { return value != "" && value != "0" && !strings.EqualFold(value, "false") } -// maybeFormatWrittenFile runs the configured formatter for absolutePath (when -// enabled and on PATH) and returns the file's content afterwards. Best-effort +// maybeFormatWrittenFile runs the configured formatter on a sibling copy of +// writtenContent (when enabled and on PATH) and returns the bytes to publish. +// The destination path is never opened or rewritten here. Best-effort // throughout: any failure — no formatter, formatter error, timeout, unreadable // result — returns writtenContent so the caller's state matches the last write // it performed itself. Only the timeout is reported back, for the reason on @@ -128,7 +129,8 @@ func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenCon if !formatOnWriteEnabled() { return unformatted } - command, ok := formatterCommands[strings.ToLower(filepath.Ext(absolutePath))] + ext := strings.ToLower(filepath.Ext(absolutePath)) + command, ok := formatterCommands[ext] if !ok { return unformatted } @@ -136,40 +138,34 @@ func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenCon if err != nil { return unformatted } + dir := filepath.Dir(absolutePath) + staging, err := os.CreateTemp(dir, ".zero-fmt-*"+ext) + if err != nil { + return unformatted + } + stagingName := staging.Name() + defer func() { _ = os.Remove(stagingName) }() + if _, err := staging.WriteString(writtenContent); err != nil { + _ = staging.Close() + return unformatted + } + if err := staging.Close(); err != nil { + return unformatted + } formatCtx, cancel := context.WithTimeout(ctx, formatOnWriteTimeout) defer cancel() - arguments := append(append([]string(nil), command[1:]...), absolutePath) + arguments := append(append([]string(nil), command[1:]...), stagingName) formatter := exec.CommandContext(formatCtx, binaryPath, arguments...) - formatter.Dir = filepath.Dir(absolutePath) + formatter.Dir = dir formatter.Stdin = strings.NewReader("") if err := formatter.Run(); err != nil { unformatted.Formatter = command[0] - // THE FORMATTER EDITS IN PLACE, SO A FAILED RUN CAN LEAVE THE FILE - // NEITHER FORMATTED NOR AS WRITTEN. Killed by the deadline or by the - // caller, or exiting partway through its own rewrite, the target can hold - // a truncation. Returning the written bytes on top of that would leave the - // tracker baseline and the diff preview describing a file that is not on - // disk, which is a worse failure than the missing formatting: the next - // edit compares against content the file does not have. - // - // Written back unconditionally on this path rather than only when the - // bytes differ. Comparing first means reading the file to find out, and a - // read that fails leaves the same ambiguity this exists to remove. - if restoreErr := os.WriteFile(absolutePath, []byte(writtenContent), 0o644); restoreErr != nil { - unformatted.RestoreFailed = true - } - // OUR deadline, not the caller's cancellation and not the formatter's own - // exit status. A cancelled tool call is already being reported as - // cancelled, and a formatter that ran and refused the file usually means - // content it could not parse, which the write itself does not promise to - // fix. Neither is this notice's business; the restore above is, for all - // three. if errors.Is(formatCtx.Err(), context.DeadlineExceeded) && ctx.Err() == nil { unformatted.TimedOut = true } return unformatted } - formatted, err := os.ReadFile(absolutePath) + formatted, err := os.ReadFile(stagingName) if err != nil { return unformatted } diff --git a/internal/tools/format_on_write_test.go b/internal/tools/format_on_write_test.go index 871de87de..57b6fde75 100644 --- a/internal/tools/format_on_write_test.go +++ b/internal/tools/format_on_write_test.go @@ -5,6 +5,7 @@ import ( "os" "os/exec" "path/filepath" + "runtime" "strings" "testing" ) @@ -122,3 +123,167 @@ func TestFormatOnWriteFormatterLookupFailure(t *testing.T) { t.Fatalf("an uninstalled formatter is a standing fact, not a miss worth reporting, got %q", notice) } } + +const uglyGoSource = "package a\n\nfunc A( ) { }\n" + +func TestFormatOnWritePublishesFormattedBytesForWriteAndEdit(t *testing.T) { + requireGofmt(t) + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + dir := t.TempDir() + tracker := NewFileTracker() + + write := NewScopedWriteFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + "content": uglyGoSource, + }, RunOptions{FileTracker: tracker}) + if write.Status != StatusOK { + t.Fatalf("write_file failed: %q", write.Output) + } + assertFormattedOnDiskAndTracked(t, tracker, filepath.Join(dir, "a.go"), write.Display.Preview) + + read := NewScopedReadFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + }, RunOptions{FileTracker: tracker}) + if read.Status != StatusOK { + t.Fatalf("read_file failed: %q", read.Output) + } + edit := NewScopedEditFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + "old_string": "func A() {}", + "new_string": "func B( ) { }", + }, RunOptions{FileTracker: tracker}) + if edit.Status != StatusOK { + t.Fatalf("edit_file failed: %q", edit.Output) + } + onDisk, err := os.ReadFile(filepath.Join(dir, "a.go")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(onDisk), "func B() {") { + t.Fatalf("edit_file must publish formatted bytes, got %q", onDisk) + } + assertTrackerMatchesDisk(t, tracker, filepath.Join(dir, "a.go")) + if !strings.Contains(edit.Display.Preview, "func B() {") && !strings.Contains(edit.Display.Preview, "func B()") { + t.Fatalf("edit_file preview must reflect formatted bytes, got %q", edit.Display.Preview) + } +} + +func TestFormatOnWriteFailureLeavesDestinationIntactUntilPublish(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake formatter shim is a POSIX script") + } + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + + for _, toolName := range []string{"write_file", "edit_file"} { + t.Run(toolName, func(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "a.go") + original := "package a\n\nfunc Original() {}\n" + if err := os.WriteFile(target, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + probeSaw := filepath.Join(dir, "probe-saw") + installFakeGofmt(t, `#!/bin/sh +if [ -n "$ZERO_FORMAT_PROBE" ]; then + cp "$ZERO_FORMAT_PROBE" "$ZERO_FORMAT_PROBE_SAW" || true +fi +printf 'PARTIAL' > "$1" +exit 1 +`) + t.Setenv("ZERO_FORMAT_PROBE", target) + t.Setenv("ZERO_FORMAT_PROBE_SAW", probeSaw) + + tracker := NewFileTracker() + read := NewScopedReadFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + }, RunOptions{FileTracker: tracker}) + if read.Status != StatusOK { + t.Fatalf("read_file failed: %q", read.Output) + } + + var result Result + switch toolName { + case "write_file": + result = NewScopedWriteFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + "content": uglyGoSource, + "overwrite": true, + }, RunOptions{FileTracker: tracker}) + default: + result = NewScopedEditFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + "old_string": "func Original() {}", + "new_string": "func B( ) { }", + }, RunOptions{FileTracker: tracker}) + } + if result.Status != StatusOK { + t.Fatalf("%s failed: %q", toolName, result.Output) + } + + saw, err := os.ReadFile(probeSaw) + if err != nil { + t.Fatalf("formatter never observed the destination: %v", err) + } + if string(saw) != original { + t.Fatalf("formatter observed %q, want the previous destination %q", saw, original) + } + onDisk, err := os.ReadFile(target) + if err != nil { + t.Fatal(err) + } + if string(onDisk) == "PARTIAL" { + t.Fatalf("%s left formatter-partial bytes on the destination", toolName) + } + if strings.Contains(string(onDisk), "PARTIAL") { + t.Fatalf("%s published formatter-partial bytes: %q", toolName, onDisk) + } + assertTrackerMatchesDisk(t, tracker, target) + }) + } +} + +func assertFormattedOnDiskAndTracked(t *testing.T, tracker *FileTracker, path, preview string) { + t.Helper() + onDisk, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(onDisk), "func A() {") { + t.Fatalf("expected gofmt-formatted content, got %q", onDisk) + } + assertTrackerMatchesDisk(t, tracker, path) + if preview != "" && !strings.Contains(preview, "func A() {") && !strings.Contains(preview, "func A()") { + t.Fatalf("preview must reflect formatted bytes, got %q", preview) + } +} + +func assertTrackerMatchesDisk(t *testing.T, tracker *FileTracker, path string) { + t.Helper() + onDisk, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + trackedPath := path + if resolved, err := filepath.EvalSymlinks(path); err == nil { + if _, ok := tracker.Version(resolved); ok { + trackedPath = resolved + } + } + version, ok := tracker.Version(trackedPath) + if !ok { + t.Fatalf("tracker has no version for %s", trackedPath) + } + if got, want := version.Hash, HashContent(onDisk); got != want { + t.Fatalf("tracker hash %s does not match on-disk bytes (hash %s)", got, want) + } +} + +func installFakeGofmt(t *testing.T, script string) { + t.Helper() + dir := t.TempDir() + path := filepath.Join(dir, "gofmt") + if err := os.WriteFile(path, []byte(script), 0o755); err != nil { + t.Fatal(err) + } + t.Setenv("PATH", dir+string(os.PathListSeparator)+os.Getenv("PATH")) +} diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index 7f708894d..33bf13ab7 100644 --- a/internal/tools/write_file.go +++ b/internal/tools/write_file.go @@ -107,16 +107,17 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an if err := recheckScopedWriteTarget(tool.workspaceRoot, tool.scope, requestedPath); err != nil { return errorResult("Error writing file " + relativePath + ": " + err.Error()) } + modelKnownContent := content + // Optional format-on-write (ZERO_FORMAT_ON_WRITE). Format staged bytes, then + // publish once. Recording pre-format content would make the next edit look + // like an external modification and trip the conflict guard; formatting the + // destination in place after publication would reintroduce partial writes. + formatting := maybeFormatWrittenFile(ctx, absolutePath, content) + content = formatting.Content cleanupWarning, err := committedWrite(absolutePath, []byte(content), 0o644) if 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. - formatting := maybeFormatWrittenFile(ctx, absolutePath, content) - content = formatting.Content // Baseline the freshly written content so a later edit/overwrite in this // session compares against what is now on disk. newInfo, _ := os.Stat(absolutePath) From 580e0ce76bda1afb37786277ce0dd1d68c20f9ac Mon Sep 17 00:00:00 2001 From: hazyhaar Date: Sat, 12 Sep 2026 19:19:38 +0200 Subject: [PATCH 10/10] fix(fsutil): preserve ACL and DACL through atomic replace Copy Linux POSIX ACLs including their absence, Darwin FILESEC via getattrlist/setattrlist, and the destination DACL onto Windows staging before write. Recheck the target after format-on-write, fail closed on unexpected SELinux xattr errors, and run Prettier with --stdin-filepath. --- internal/fsutil/getattrlist_darwin.go | 41 ++++ internal/fsutil/getattrlist_darwin.s | 8 + internal/fsutil/rename.go | 32 ++- internal/fsutil/rename_acl_darwin.go | 78 +++++++ internal/fsutil/rename_acl_darwin_test.go | 83 +++++++ internal/fsutil/rename_acl_linux_test.go | 51 +++++ internal/fsutil/rename_acl_other.go | 14 ++ internal/fsutil/rename_staging_other.go | 14 ++ internal/fsutil/rename_staging_windows.go | 115 ++++++++++ .../fsutil/rename_staging_windows_test.go | 134 ++++++++++++ internal/fsutil/rename_umask_unix_test.go | 23 ++ internal/fsutil/rename_xattr_notfound_bsd.go | 13 ++ .../fsutil/rename_xattr_notfound_freebsd.go | 13 ++ .../fsutil/rename_xattr_notfound_linux.go | 13 ++ internal/fsutil/rename_xattr_unix.go | 22 +- internal/fsutil/rename_xattr_unix_test.go | 35 +++ internal/tools/edit_file.go | 4 + internal/tools/format_on_write.go | 46 ++++ internal/tools/format_on_write_test.go | 206 +++++++++++++++++- internal/tools/write_file.go | 14 ++ 20 files changed, 956 insertions(+), 3 deletions(-) create mode 100644 internal/fsutil/getattrlist_darwin.go create mode 100644 internal/fsutil/getattrlist_darwin.s create mode 100644 internal/fsutil/rename_acl_darwin.go create mode 100644 internal/fsutil/rename_acl_darwin_test.go create mode 100644 internal/fsutil/rename_acl_other.go create mode 100644 internal/fsutil/rename_staging_other.go create mode 100644 internal/fsutil/rename_staging_windows.go create mode 100644 internal/fsutil/rename_staging_windows_test.go create mode 100644 internal/fsutil/rename_xattr_notfound_bsd.go create mode 100644 internal/fsutil/rename_xattr_notfound_freebsd.go create mode 100644 internal/fsutil/rename_xattr_notfound_linux.go create mode 100644 internal/fsutil/rename_xattr_unix_test.go diff --git a/internal/fsutil/getattrlist_darwin.go b/internal/fsutil/getattrlist_darwin.go new file mode 100644 index 000000000..d5a13ff79 --- /dev/null +++ b/internal/fsutil/getattrlist_darwin.go @@ -0,0 +1,41 @@ +//go:build darwin + +package fsutil + +import ( + "syscall" + "unsafe" + + "golang.org/x/sys/unix" +) + +var getattrlistZero byte + +var libc_getattrlist_trampoline_addr uintptr + +//go:linkname syscall_syscall6 syscall.syscall6 +func syscall_syscall6(fn, a1, a2, a3, a4, a5, a6 uintptr) (r1, r2 uintptr, err syscall.Errno) + +//go:cgo_import_dynamic libc_getattrlist getattrlist "/usr/lib/libSystem.B.dylib" + +func getattrlist(path string, attrList *unix.Attrlist, attrBuf []byte, options uint32) error { + p, err := unix.BytePtrFromString(path) + if err != nil { + return err + } + bufPtr := unsafe.Pointer(&getattrlistZero) + if len(attrBuf) > 0 { + bufPtr = unsafe.Pointer(&attrBuf[0]) + } + _, _, e := syscall_syscall6(libc_getattrlist_trampoline_addr, + uintptr(unsafe.Pointer(p)), + uintptr(unsafe.Pointer(attrList)), + uintptr(bufPtr), + uintptr(len(attrBuf)), + uintptr(options), + 0) + if e != 0 { + return e + } + return nil +} diff --git a/internal/fsutil/getattrlist_darwin.s b/internal/fsutil/getattrlist_darwin.s new file mode 100644 index 000000000..e1cfbe121 --- /dev/null +++ b/internal/fsutil/getattrlist_darwin.s @@ -0,0 +1,8 @@ +//go:build darwin + +#include "textflag.h" + +TEXT libc_getattrlist_trampoline<>(SB),NOSPLIT,$0-0 + JMP libc_getattrlist(SB) +GLOBL ·libc_getattrlist_trampoline_addr(SB), RODATA, $8 +DATA ·libc_getattrlist_trampoline_addr(SB)/8, $libc_getattrlist_trampoline<>(SB) diff --git a/internal/fsutil/rename.go b/internal/fsutil/rename.go index c865eb665..fab44e3ed 100644 --- a/internal/fsutil/rename.go +++ b/internal/fsutil/rename.go @@ -16,6 +16,18 @@ import ( // FIFO, device, socket, directory, or other special file with a regular file. var ErrNonRegularDestination = errors.New("fsutil: destination exists and is not a regular file") +// stagingModeObserver, when non-nil, receives the mode of the freshly created +// staging file before any metadata is copied onto it. Tests use it to assert +// that a replacement is staged no broader than its destination. +var stagingModeObserver func(os.FileMode) + +// stagingProtectionObserver, when non-nil, receives the path of the staging +// file after protectStaging has copied the destination's authorization metadata +// onto it, still before any replacement bytes are written. Tests use it to +// assert that a replacement is staged no broader than its destination on +// platforms (Windows) where the mode bits do not carry that answer. +var stagingProtectionObserver func(stagingPath string) + // WriteFileAtomic writes data to a temporary file in the same directory as filename, // flushes and syncs it to disk, and replaces filename atomically via ReplaceWithRetry. // For new files, it honors the process umask by creating the temporary file with @@ -56,10 +68,19 @@ func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { return err } - tmpFile, err := createTempFile(dir, perm) + stagePerm := perm + if existingMode != nil { + stagePerm = *existingMode + } + tmpFile, err := createTempFile(dir, stagePerm) if err != nil { return err } + if stagingModeObserver != nil { + if info, statErr := tmpFile.Stat(); statErr == nil { + stagingModeObserver(info.Mode()) + } + } tmpName := tmpFile.Name() closed := false defer func() { @@ -79,6 +100,15 @@ func WriteFileAtomic(filename string, data []byte, perm os.FileMode) error { if err := preserveXattrs(tmpFile, filename); err != nil { return err } + if err := preserveNativeACL(tmpFile, filename); err != nil { + return err + } + if err := protectStaging(tmpFile, filename); err != nil { + return err + } + if stagingProtectionObserver != nil { + stagingProtectionObserver(tmpName) + } } if _, err := tmpFile.Write(data); err != nil { return err diff --git a/internal/fsutil/rename_acl_darwin.go b/internal/fsutil/rename_acl_darwin.go new file mode 100644 index 000000000..a42d46154 --- /dev/null +++ b/internal/fsutil/rename_acl_darwin.go @@ -0,0 +1,78 @@ +//go:build darwin + +package fsutil + +import ( + "encoding/binary" + "fmt" + "os" + + "golang.org/x/sys/unix" +) + +// preserveNativeACL copies the Darwin kauth/FILESEC ACL using getattrlist and +// setattrlist ATTR_CMN_EXTENDED_SECURITY. Listxattr/Getxattr/Fsetxattr cannot +// see com.apple.system.Security (protected namespace); that path is not used. +func preserveNativeACL(f *os.File, srcPath string) error { + acl, err := readNativeACL(srcPath) + if err != nil { + return err + } + if acl == nil { + return nil + } + if err := applyNativeACL(f.Name(), acl); err != nil { + return fmt.Errorf("fsutil: preserving native ACL on replacement for %s: %w", srcPath, err) + } + return nil +} + +func extendedSecurityAttrlist() unix.Attrlist { + return unix.Attrlist{ + Bitmapcount: unix.ATTR_BIT_MAP_COUNT, + Commonattr: unix.ATTR_CMN_EXTENDED_SECURITY, + } +} + +func readNativeACL(path string) ([]byte, error) { + al := extendedSecurityAttrlist() + buf := make([]byte, 4096) + if err := getattrlist(path, &al, buf, 0); err != nil { + if isXattrNotFound(err) || isXattrUnsupported(err) { + return nil, nil + } + return nil, fmt.Errorf("fsutil: reading native ACL of %s: %w", path, err) + } + if len(buf) < 12 { + return nil, nil + } + total := binary.LittleEndian.Uint32(buf[:4]) + if total < 12 { + return nil, nil + } + off := int32(binary.LittleEndian.Uint32(buf[4:8])) + length := binary.LittleEndian.Uint32(buf[8:12]) + if length == 0 { + return nil, nil + } + start := 4 + int(off) + end := start + int(length) + if start < 12 || end > int(total) || end > len(buf) { + return nil, fmt.Errorf("fsutil: truncated native ACL on %s", path) + } + out := make([]byte, length) + copy(out, buf[start:end]) + return out, nil +} + +func applyNativeACL(path string, blob []byte) error { + al := extendedSecurityAttrlist() + buf := make([]byte, 8+len(blob)) + binary.LittleEndian.PutUint32(buf[0:4], 8) + binary.LittleEndian.PutUint32(buf[4:8], uint32(len(blob))) + copy(buf[8:], blob) + if err := unix.Setattrlist(path, &al, buf, 0); err != nil { + return err + } + return nil +} diff --git a/internal/fsutil/rename_acl_darwin_test.go b/internal/fsutil/rename_acl_darwin_test.go new file mode 100644 index 000000000..93db46c0a --- /dev/null +++ b/internal/fsutil/rename_acl_darwin_test.go @@ -0,0 +1,83 @@ +//go:build darwin + +package fsutil + +import ( + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// TestWriteFileAtomicPreservesOrRefusesNativeACL pins the fail-closed contract +// for a Darwin native (kauth/FILESEC) ACL: replacing the destination either +// carries the ACL over to the new inode, or the replacement is refused and the +// original file is left byte-for-byte intact. What must never happen is a +// successful replacement that silently drops the ACL. +func TestWriteFileAtomicPreservesOrRefusesNativeACL(t *testing.T) { + if _, err := exec.LookPath("chmod"); err != nil { + t.Skip("chmod not on PATH") + } + if _, err := exec.LookPath("ls"); err != nil { + t.Skip("ls not on PATH") + } + + dir := t.TempDir() + target := filepath.Join(dir, "restricted.txt") + original := "old" + if err := os.WriteFile(target, []byte(original), 0o644); err != nil { + t.Fatalf("WriteFile: %v", err) + } + + set := exec.Command("chmod", "+a", "user:nobody deny read", target) + if out, err := set.CombinedOutput(); err != nil { + t.Skipf("chmod +a failed (ACLs unavailable on this filesystem): %v\n%s", err, out) + } + + acl, err := readNativeACL(target) + if err != nil { + t.Skipf("cannot read the native ACL that chmod +a just set: %v", err) + } + if len(acl) == 0 { + t.Skip("chmod +a reported success but no native ACL is present") + } + before, err := exec.Command("ls", "-le", target).CombinedOutput() + if err != nil { + t.Fatalf("ls -le before: %v\n%s", err, before) + } + if !strings.Contains(string(before), "deny") { + t.Skipf("named deny entry did not stick; ls -le:\n%s", before) + } + + replaceErr := WriteFileAtomic(target, []byte("new"), 0o600) + if replaceErr != nil { + // Refusal path: the error is acceptable only if the destination was + // not mutated on the way out. + got, rerr := os.ReadFile(target) + if rerr != nil { + t.Fatalf("WriteFileAtomic failed (%v) and the original is unreadable: %v", replaceErr, rerr) + } + if string(got) != original { + t.Fatalf("WriteFileAtomic failed (%v) but mutated the destination to %q", replaceErr, got) + } + return + } + + // Preservation path: the replacement succeeded, so the deny entry must + // still be attached to the new inode. + after, err := readNativeACL(target) + if err != nil { + t.Fatalf("WriteFileAtomic succeeded but the native ACL became unreadable: %v", err) + } + if len(after) == 0 { + t.Fatalf("WriteFileAtomic replaced the destination and lost the native ACL") + } + afterListing, err := exec.Command("ls", "-le", target).CombinedOutput() + if err != nil { + t.Fatalf("ls -le after: %v\n%s", err, afterListing) + } + if !strings.Contains(string(afterListing), "deny") { + t.Fatalf("native ACL deny entry was lost after replacement\nbefore:\n%s\nafter:\n%s", before, afterListing) + } +} diff --git a/internal/fsutil/rename_acl_linux_test.go b/internal/fsutil/rename_acl_linux_test.go index e1dac2669..e05926a39 100644 --- a/internal/fsutil/rename_acl_linux_test.go +++ b/internal/fsutil/rename_acl_linux_test.go @@ -63,3 +63,54 @@ func namedUserACLDenied(listing []byte) bool { bytes.Contains(listing, []byte("user:nobody:---")) || bytes.Contains(listing, []byte("user:nfsnobody:---")) } + +func namedUserACLPresent(listing []byte) bool { + return bytes.Contains(listing, []byte("user:65534:")) || + bytes.Contains(listing, []byte("user:nobody:")) || + bytes.Contains(listing, []byte("user:nfsnobody:")) +} + +func TestWriteFileAtomicDropsInheritedAccessACL(t *testing.T) { + if _, err := exec.LookPath("setfacl"); err != nil { + t.Skip("setfacl not on PATH; POSIX ACL inheritance is not exercised on this host") + } + if _, err := exec.LookPath("getfacl"); err != nil { + t.Skip("getfacl not on PATH; POSIX ACL inheritance is not exercised on this host") + } + + base := t.TempDir() + dir := filepath.Join(base, "shared") + if err := os.Mkdir(dir, 0o755); err != nil { + t.Fatal(err) + } + if out, err := exec.Command("setfacl", "-d", "-m", "u:65534:r--", dir).CombinedOutput(); err != nil { + t.Skipf("setfacl default ACL failed (filesystem may lack ACL support): %v\n%s", err, out) + } + + target := filepath.Join(dir, "plain.txt") + if err := os.WriteFile(target, []byte("old"), 0o640); err != nil { + t.Fatal(err) + } + if out, err := exec.Command("setfacl", "-b", target).CombinedOutput(); err != nil { + t.Skipf("setfacl -b failed: %v\n%s", err, out) + } + before, err := exec.Command("getfacl", "-cp", target).CombinedOutput() + if err != nil { + t.Fatalf("getfacl before: %v\n%s", err, before) + } + if namedUserACLPresent(before) { + t.Skipf("source still has a named-user ACL; cannot assert inheritance removal\ngetfacl:\n%s", before) + } + + if err := WriteFileAtomic(target, []byte("new"), 0o640); err != nil { + t.Fatalf("WriteFileAtomic: %v", err) + } + + after, err := exec.Command("getfacl", "-cp", target).CombinedOutput() + if err != nil { + t.Fatalf("getfacl after: %v\n%s", err, after) + } + if namedUserACLPresent(after) { + t.Fatalf("access ACL inherited from the directory default survived the replacement\nbefore:\n%s\nafter:\n%s", before, after) + } +} diff --git a/internal/fsutil/rename_acl_other.go b/internal/fsutil/rename_acl_other.go new file mode 100644 index 000000000..00a02fc0e --- /dev/null +++ b/internal/fsutil/rename_acl_other.go @@ -0,0 +1,14 @@ +//go:build !darwin + +package fsutil + +import "os" + +// preserveNativeACL is a no-op on platforms without the Darwin native ACL +// representation. Linux POSIX ACLs travel through preserveXattrs as +// system.posix_acl_access; on Windows the destination DACL is applied to the +// staging file by protectStaging before the write and carried across +// publication by the DACL-preserving replace primitive. +func preserveNativeACL(*os.File, string) error { + return nil +} diff --git a/internal/fsutil/rename_staging_other.go b/internal/fsutil/rename_staging_other.go new file mode 100644 index 000000000..1cff67b40 --- /dev/null +++ b/internal/fsutil/rename_staging_other.go @@ -0,0 +1,14 @@ +//go:build !windows + +package fsutil + +import "os" + +// protectStaging is a no-op on platforms whose authorization metadata is +// already copied onto the staging file by WriteFileAtomic: Unix mode bits, +// owner, and extended attributes (including POSIX ACLs) travel through +// Chmod, preserveOwner, preserveXattrs, and preserveNativeACL. Only Windows +// needs an explicit DACL transfer before the replacement bytes are written. +func protectStaging(*os.File, string) error { + return nil +} diff --git a/internal/fsutil/rename_staging_windows.go b/internal/fsutil/rename_staging_windows.go new file mode 100644 index 000000000..95f0a04df --- /dev/null +++ b/internal/fsutil/rename_staging_windows.go @@ -0,0 +1,115 @@ +//go:build windows + +package fsutil + +import ( + "errors" + "fmt" + "os" + + "golang.org/x/sys/windows" +) + +// protectStaging copies the destination's DACL onto the freshly created staging +// file f before any replacement bytes are written through it. A temporary file +// created in the destination's directory inherits the directory's DACL, not the +// destination's, and os.File.Chmod cannot express a Windows DACL (Go maps only +// the owner-write bit). Without this step the replacement content is readable by +// every principal the directory grants access to during the window between the +// first Write and ReplaceFileW, even though ReplaceFileW later restores the +// restrictive destination DACL onto the published file. +// +// The DACL is read from destPath and written to the staging object. The handle +// os.OpenFile returned for f was opened with GENERIC_READ|GENERIC_WRITE, which +// does not include WRITE_DAC, so a second handle is opened with the write-DAC +// access and checked to name the same object as f before the descriptor is +// applied. That keeps the transfer bound to the object this process created +// instead of a pathname a writer in the directory could redirect. +// +// When the destination's descriptor is protected (its DACL does not inherit), +// the staging DACL is marked protected too, so re-inherited directory ACEs +// cannot widen it. A failure is returned rather than ignored: WriteFileAtomic +// then abandons the staging file and leaves the destination untouched, instead +// of writing content under a broader descriptor. +func protectStaging(f *os.File, destPath string) error { + descriptor, err := windows.GetNamedSecurityInfo(destPath, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + return fmt.Errorf("fsutil: reading DACL of %s for staging: %w", destPath, err) + } + if descriptor == nil { + return fmt.Errorf("fsutil: destination %s has no security descriptor to protect the staging file", destPath) + } + dacl, _, err := descriptor.DACL() + if err != nil { + // A destination with no DACL at all is fully permissive, so an inherited + // staging DACL cannot expose anything the destination does not already. + if errors.Is(err, windows.ERROR_OBJECT_NOT_FOUND) { + return nil + } + return fmt.Errorf("fsutil: extracting DACL of %s for staging: %w", destPath, err) + } + if dacl == nil { + // A present but NULL DACL grants everyone full control: the destination + // is not restricted, so there is no narrower descriptor to carry over. + // Setting a NULL DACL here would only widen the staging file. + return nil + } + info := windows.SECURITY_INFORMATION(windows.DACL_SECURITY_INFORMATION) + if control, _, controlErr := descriptor.Control(); controlErr == nil && control&windows.SE_DACL_PROTECTED != 0 { + info |= windows.PROTECTED_DACL_SECURITY_INFORMATION + } + handle, err := openStagingForDACL(f) + if err != nil { + return err + } + defer func() { _ = windows.CloseHandle(handle) }() + if err := windows.SetSecurityInfo(handle, windows.SE_FILE_OBJECT, info, nil, nil, dacl, nil); err != nil { + return fmt.Errorf("fsutil: applying DACL of %s to staging file: %w", destPath, err) + } + return nil +} + +// openStagingForDACL opens the staging file f names with the READ_CONTROL and +// WRITE_DAC rights SetSecurityInfo requires, which the handle inside f does not +// carry. The new handle is verified to reference the same volume and file index +// as f, and to not be a reparse point, so a directory entry swapped in after +// creation cannot receive the descriptor instead of the object this process +// created. The caller owns the returned handle. +func openStagingForDACL(f *os.File) (windows.Handle, error) { + name, err := windows.UTF16PtrFromString(f.Name()) + if err != nil { + return 0, err + } + handle, err := windows.CreateFile( + name, + windows.READ_CONTROL|windows.WRITE_DAC, + windows.FILE_SHARE_READ|windows.FILE_SHARE_WRITE|windows.FILE_SHARE_DELETE, + nil, + windows.OPEN_EXISTING, + windows.FILE_ATTRIBUTE_NORMAL|windows.FILE_FLAG_OPEN_REPARSE_POINT, + 0, + ) + if err != nil { + return 0, fmt.Errorf("fsutil: opening staging file %s to apply its DACL: %w", f.Name(), err) + } + var created, opened windows.ByHandleFileInformation + if err := windows.GetFileInformationByHandle(windows.Handle(f.Fd()), &created); err != nil { + _ = windows.CloseHandle(handle) + return 0, fmt.Errorf("fsutil: querying created staging file %s: %w", f.Name(), err) + } + if err := windows.GetFileInformationByHandle(handle, &opened); err != nil { + _ = windows.CloseHandle(handle) + return 0, fmt.Errorf("fsutil: querying reopened staging file %s: %w", f.Name(), err) + } + if opened.FileAttributes&windows.FILE_ATTRIBUTE_REPARSE_POINT != 0 { + _ = windows.CloseHandle(handle) + return 0, fmt.Errorf("fsutil: staging path %s is unexpectedly a reparse point", f.Name()) + } + if created.VolumeSerialNumber != opened.VolumeSerialNumber || + created.FileIndexHigh != opened.FileIndexHigh || + created.FileIndexLow != opened.FileIndexLow { + _ = windows.CloseHandle(handle) + return 0, fmt.Errorf("fsutil: staging path %s no longer names the created file", f.Name()) + } + return handle, nil +} diff --git a/internal/fsutil/rename_staging_windows_test.go b/internal/fsutil/rename_staging_windows_test.go new file mode 100644 index 000000000..7b34c93ae --- /dev/null +++ b/internal/fsutil/rename_staging_windows_test.go @@ -0,0 +1,134 @@ +//go:build windows + +package fsutil + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "golang.org/x/sys/windows" +) + +// TestProtectStagingCopiesRestrictiveDACL pins the Windows pre-publish DACL +// contract: while the replacement bytes are still staged under an inherited +// directory DACL, the staging file must already carry the destination's +// restrictive DACL. A file created in a directory inherits that directory's +// DACL, and Chmod cannot express an owner-only Windows ACL, so without +// protectStaging the content is exposed to every principal the directory grants +// access to even though ReplaceFileW later restores the destination DACL. +func TestProtectStagingCopiesRestrictiveDACL(t *testing.T) { + dir := t.TempDir() + + // Make the parent more permissive than the destination so an inherited + // staging DACL is observably broader than the destination's. + if err := grantEveryoneInheritableDACL(dir); err != nil { + t.Skipf("cannot widen the parent directory DACL on this filesystem: %v", err) + } + + target := filepath.Join(dir, "restricted.txt") + if err := os.WriteFile(target, []byte("old"), 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + restricted, err := windows.SecurityDescriptorFromString("D:P(A;;FA;;;OW)") + if err != nil { + t.Skipf("cannot build the restrictive test descriptor: %v", err) + } + ownerOnly, _, err := restricted.DACL() + if err != nil { + t.Skipf("cannot read the restrictive test DACL: %v", err) + } + if err := windows.SetNamedSecurityInfo( + target, + windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION|windows.PROTECTED_DACL_SECURITY_INFORMATION, + nil, nil, ownerOnly, nil, + ); err != nil { + t.Skipf("cannot apply a restrictive DACL on this filesystem: %v", err) + } + if got, err := readDACLString(target); err != nil { + t.Skipf("cannot read back the restrictive destination DACL: %v", err) + } else if !strings.Contains(got, "(A;;FA;;;OW)") { + t.Skipf("the restrictive DACL did not take effect on this filesystem: %q", got) + } + + var ( + stagingDACL string + captureErr error + ) + previous := stagingProtectionObserver + stagingProtectionObserver = func(stagingPath string) { + stagingDACL, captureErr = readDACLString(stagingPath) + } + defer func() { stagingProtectionObserver = previous }() + + if err := WriteFileAtomic(target, []byte("new"), 0o600); err != nil { + t.Fatalf("WriteFileAtomic: %v", err) + } + if captureErr != nil { + t.Fatalf("reading the staging DACL before the write: %v", captureErr) + } + if stagingDACL == "" { + t.Fatal("staging DACL was not observed: the protection hook did not run before the write") + } + if !strings.Contains(stagingDACL, "(A;;FA;;;OW)") { + t.Fatalf("staging DACL = %q, want the owner-only destination DACL", stagingDACL) + } + if strings.Contains(stagingDACL, ";;;WD)") { + t.Fatalf("staging DACL = %q is broader than the destination (inherited Everyone)", stagingDACL) + } + + after, err := readDACLString(target) + if err != nil { + t.Fatalf("reading the destination DACL after replacement: %v", err) + } + if !strings.Contains(after, "(A;;FA;;;OW)") || strings.Contains(after, ";;;WD)") { + t.Fatalf("destination DACL after replacement = %q, want the original owner-only DACL", after) + } +} + +// grantEveryoneInheritableDACL replaces path's DACL with a single inheritable +// grant of full control to Everyone (S-1-1-0), making it broader than any +// owner-only destination inside it. +func grantEveryoneInheritableDACL(path string) error { + everyone, err := windows.StringToSid("S-1-1-0") + if err != nil { + return err + } + dacl, err := windows.ACLFromEntries([]windows.EXPLICIT_ACCESS{{ + AccessPermissions: windows.GENERIC_ALL, + AccessMode: windows.GRANT_ACCESS, + Inheritance: windows.SUB_CONTAINERS_AND_OBJECTS_INHERIT, + Trustee: windows.TRUSTEE{ + TrusteeForm: windows.TRUSTEE_IS_SID, + TrusteeType: windows.TRUSTEE_IS_WELL_KNOWN_GROUP, + TrusteeValue: windows.TrusteeValueFromSID(everyone), + }, + }}, nil) + if err != nil { + return err + } + return windows.SetNamedSecurityInfo( + path, + windows.SE_FILE_OBJECT, + windows.DACL_SECURITY_INFORMATION, + nil, nil, dacl, nil, + ) +} + +// readDACLString returns the SDDL DACL portion of path's security descriptor. +func readDACLString(path string) (string, error) { + descriptor, err := windows.GetNamedSecurityInfo(path, windows.SE_FILE_OBJECT, windows.DACL_SECURITY_INFORMATION) + if err != nil { + return "", err + } + if descriptor == nil { + return "", nil + } + text := descriptor.String() + if index := strings.Index(text, "D:"); index >= 0 { + return text[index:], nil + } + return text, nil +} diff --git a/internal/fsutil/rename_umask_unix_test.go b/internal/fsutil/rename_umask_unix_test.go index eecc53a95..4b9fd2d5b 100644 --- a/internal/fsutil/rename_umask_unix_test.go +++ b/internal/fsutil/rename_umask_unix_test.go @@ -32,3 +32,26 @@ func TestWriteFileAtomicRespectsProcessUmask(t *testing.T) { t.Fatalf("created file perm = %04o, want %04o (honoring umask 0o077)", got, 0o600) } } + +func TestWriteFileAtomicStagesReplacementWithDestinationMode(t *testing.T) { + oldMask := syscall.Umask(0) + defer syscall.Umask(oldMask) + + dir := t.TempDir() + target := filepath.Join(dir, "secret.txt") + if err := os.WriteFile(target, []byte("old"), 0o600); err != nil { + t.Fatal(err) + } + + orig := stagingModeObserver + t.Cleanup(func() { stagingModeObserver = orig }) + var observed os.FileMode + stagingModeObserver = func(mode os.FileMode) { observed = mode } + + if err := WriteFileAtomic(target, []byte("new"), 0o644); err != nil { + t.Fatalf("WriteFileAtomic: %v", err) + } + if got := observed.Perm(); got != 0o600 { + t.Fatalf("staging mode = %04o, want the destination mode %04o", got, 0o600) + } +} diff --git a/internal/fsutil/rename_xattr_notfound_bsd.go b/internal/fsutil/rename_xattr_notfound_bsd.go new file mode 100644 index 000000000..3d6344465 --- /dev/null +++ b/internal/fsutil/rename_xattr_notfound_bsd.go @@ -0,0 +1,13 @@ +//go:build darwin || netbsd + +package fsutil + +import ( + "errors" + + "golang.org/x/sys/unix" +) + +func isXattrNotFound(err error) bool { + return errors.Is(err, unix.ENOATTR) || errors.Is(err, unix.ENODATA) +} diff --git a/internal/fsutil/rename_xattr_notfound_freebsd.go b/internal/fsutil/rename_xattr_notfound_freebsd.go new file mode 100644 index 000000000..23ebc6a0d --- /dev/null +++ b/internal/fsutil/rename_xattr_notfound_freebsd.go @@ -0,0 +1,13 @@ +//go:build freebsd + +package fsutil + +import ( + "errors" + + "golang.org/x/sys/unix" +) + +func isXattrNotFound(err error) bool { + return errors.Is(err, unix.ENOATTR) +} diff --git a/internal/fsutil/rename_xattr_notfound_linux.go b/internal/fsutil/rename_xattr_notfound_linux.go new file mode 100644 index 000000000..d24539038 --- /dev/null +++ b/internal/fsutil/rename_xattr_notfound_linux.go @@ -0,0 +1,13 @@ +//go:build linux + +package fsutil + +import ( + "errors" + + "golang.org/x/sys/unix" +) + +func isXattrNotFound(err error) bool { + return errors.Is(err, unix.ENODATA) +} diff --git a/internal/fsutil/rename_xattr_unix.go b/internal/fsutil/rename_xattr_unix.go index be9008207..eac309734 100644 --- a/internal/fsutil/rename_xattr_unix.go +++ b/internal/fsutil/rename_xattr_unix.go @@ -10,6 +10,8 @@ import ( "golang.org/x/sys/unix" ) +const posixACLAccessXattr = "system.posix_acl_access" + func preserveXattrs(f *os.File, srcPath string) error { names, err := listXattrs(srcPath) if err != nil { @@ -18,7 +20,11 @@ func preserveXattrs(f *os.File, srcPath string) error { } return fmt.Errorf("fsutil: listing xattrs of %s: %w", srcPath, err) } + hasAccessACL := false for _, name := range names { + if name == posixACLAccessXattr { + hasAccessACL = true + } data, err := getXattr(srcPath, name) if err != nil { if isXattrUnsupported(err) { @@ -27,15 +33,29 @@ func preserveXattrs(f *os.File, srcPath string) error { return fmt.Errorf("fsutil: reading xattr %s from %s: %w", name, srcPath, err) } if err := unix.Fsetxattr(int(f.Fd()), name, data, 0); err != nil { - if name == "security.selinux" { + if name == "security.selinux" && isSELinuxPolicyDenial(err) { continue } return fmt.Errorf("fsutil: preserving xattr %s: %w", name, err) } } + if !hasAccessACL { + if err := unix.Fremovexattr(int(f.Fd()), posixACLAccessXattr); err != nil { + if !isXattrNotFound(err) && !isXattrUnsupported(err) { + return fmt.Errorf("fsutil: removing inherited ACL from replacement: %w", err) + } + } + } return nil } +func isSELinuxPolicyDenial(err error) bool { + return errors.Is(err, unix.EACCES) || + errors.Is(err, unix.EPERM) || + errors.Is(err, unix.ENOTSUP) || + errors.Is(err, unix.EOPNOTSUPP) +} + func listXattrs(path string) ([]string, error) { dest := []byte(nil) for { diff --git a/internal/fsutil/rename_xattr_unix_test.go b/internal/fsutil/rename_xattr_unix_test.go new file mode 100644 index 000000000..abe717819 --- /dev/null +++ b/internal/fsutil/rename_xattr_unix_test.go @@ -0,0 +1,35 @@ +//go:build linux || darwin || freebsd || netbsd + +package fsutil + +import ( + "fmt" + "testing" + + "golang.org/x/sys/unix" +) + +func TestSELinuxPolicyDenialClassification(t *testing.T) { + cases := []struct { + name string + err error + want bool + }{ + {"nil", nil, false}, + {"eacces", unix.EACCES, true}, + {"eperm", unix.EPERM, true}, + {"enotsup", unix.ENOTSUP, true}, + {"eopnotsupp", unix.EOPNOTSUPP, true}, + {"eio", unix.EIO, false}, + {"enospc", unix.ENOSPC, false}, + {"wrapped eacces", fmt.Errorf("setxattr: %w", unix.EACCES), true}, + {"wrapped eio", fmt.Errorf("setxattr: %w", unix.EIO), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := isSELinuxPolicyDenial(tc.err); got != tc.want { + t.Fatalf("isSELinuxPolicyDenial(%v) = %v, want %v", tc.err, got, tc.want) + } + }) + } +} diff --git a/internal/tools/edit_file.go b/internal/tools/edit_file.go index cf2338e2d..09bc0eb8e 100644 --- a/internal/tools/edit_file.go +++ b/internal/tools/edit_file.go @@ -1,6 +1,7 @@ package tools import ( + "bytes" "context" "errors" "fmt" @@ -160,6 +161,9 @@ func (tool editFileTool) RunWithOptions(ctx context.Context, args map[string]any // destination in place after publication would reintroduce partial writes. formatting := maybeFormatWrittenFile(ctx, absolutePath, updated) updated = formatting.Content + if current, rerr := os.ReadFile(absolutePath); rerr != nil || !bytes.Equal(current, []byte(content)) { + return errorResult(fileConflictMessage(relativePath)) + } cleanupWarning, err := committedWrite(absolutePath, []byte(updated), 0o644) if err != nil { return errorResult("Error writing " + relativePath + ": " + err.Error()) diff --git a/internal/tools/format_on_write.go b/internal/tools/format_on_write.go index 1f64f9a1e..fd5815db0 100644 --- a/internal/tools/format_on_write.go +++ b/internal/tools/format_on_write.go @@ -1,6 +1,7 @@ package tools import ( + "bytes" "context" "errors" "os" @@ -139,6 +140,9 @@ func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenCon return unformatted } dir := filepath.Dir(absolutePath) + if command[0] == "prettier" { + return formatWithPrettier(ctx, command[0], binaryPath, command[1:], absolutePath, writtenContent) + } staging, err := os.CreateTemp(dir, ".zero-fmt-*"+ext) if err != nil { return unformatted @@ -171,3 +175,45 @@ func maybeFormatWrittenFile(ctx context.Context, absolutePath string, writtenCon } return formatOnWriteResult{Content: string(formatted), Formatter: command[0]} } + +// formatWithPrettier runs Prettier in stdin mode. Prettier is the one formatter +// here whose behaviour depends on the file name rather than the file's +// extension: it resolves .prettierrc from the file's directory and applies its +// .prettierignore rules. Staging the bytes under ".zero-fmt-*.js" would hide +// the real destination name from both, so the content travels on stdin while +// --stdin-filepath carries the real path. The formatted bytes are read back +// from stdout; Prettier is not asked to --write the staging file. Config +// resolution still needs the working directory set to the destination's +// directory. Any formatter failure (including an ignored path that yields no +// usable stdout) falls back to writtenContent. +func formatWithPrettier(ctx context.Context, formatterName, binaryPath string, formatterArgs []string, absolutePath, writtenContent string) formatOnWriteResult { + unformatted := formatOnWriteResult{Content: writtenContent} + arguments := make([]string, 0, len(formatterArgs)+2) + for _, arg := range formatterArgs { + if arg == "--write" { + continue + } + arguments = append(arguments, arg) + } + arguments = append(arguments, "--stdin-filepath", absolutePath) + formatCtx, cancel := context.WithTimeout(ctx, formatOnWriteTimeout) + defer cancel() + formatter := exec.CommandContext(formatCtx, binaryPath, arguments...) + formatter.Dir = filepath.Dir(absolutePath) + formatter.Stdin = strings.NewReader(writtenContent) + var stdout bytes.Buffer + formatter.Stdout = &stdout + if err := formatter.Run(); err != nil { + unformatted.Formatter = formatterName + if errors.Is(formatCtx.Err(), context.DeadlineExceeded) && ctx.Err() == nil { + unformatted.TimedOut = true + } + return unformatted + } + if stdout.Len() == 0 && writtenContent != "" { + // A formatter that produced nothing for non-empty input (an ignored + // path on some CLI versions) must not publish an empty file. + return unformatted + } + return formatOnWriteResult{Content: stdout.String(), Formatter: formatterName} +} diff --git a/internal/tools/format_on_write_test.go b/internal/tools/format_on_write_test.go index 57b6fde75..8f2728fc1 100644 --- a/internal/tools/format_on_write_test.go +++ b/internal/tools/format_on_write_test.go @@ -8,6 +8,7 @@ import ( "runtime" "strings" "testing" + "time" ) // gofmt ships with the Go toolchain, so it is the one formatter guaranteed to @@ -124,6 +125,106 @@ func TestFormatOnWriteFormatterLookupFailure(t *testing.T) { } } +// requirePrettier skips when the Node-based formatter is not installed; unlike +// gofmt it is not guaranteed by the Go toolchain. +func requirePrettier(t *testing.T) { + t.Helper() + if _, err := exec.LookPath("prettier"); err != nil { + t.Skip("prettier not on PATH") + } +} + +func TestFormatOnWritePrettierUsesDestinationFileName(t *testing.T) { + requirePrettier(t) + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, ".prettierrc"), []byte(`{"overrides":[{"files":"special.js","options":{"singleQuote":true}}]}`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, ".prettierignore"), []byte("ignored.js\n"), 0o644); err != nil { + t.Fatal(err) + } + + // The override matches "special.js" only. If Prettier saw the staging name + // ".zero-fmt-*.js" instead of the destination, it would use the default + // double quotes and this assertion would fail. + specialPath := filepath.Join(dir, "special.js") + special := maybeFormatWrittenFile(context.Background(), specialPath, "const x = \"a\";\n") + if !strings.Contains(special.Content, "const x = 'a';") { + t.Fatalf("prettier must resolve .prettierrc against the destination name, got %q", special.Content) + } + + // Default Prettier would reformat this to `const x = "a";`. Because the + // destination name is ignored, Prettier echoes the input and the fallback + // keeps the written bytes intact. Against the staging name it would not be + // ignored and the reformatted bytes would win. + ignoredPath := filepath.Join(dir, "ignored.js") + ignoredInput := "const x=\"a\";\n" + ignored := maybeFormatWrittenFile(context.Background(), ignoredPath, ignoredInput) + if ignored.Content != ignoredInput { + t.Fatalf("prettier must honour .prettierignore for the destination name, got %q", ignored.Content) + } +} + +// TestFormatOnWritePrettierUsesDestinationFilename exercises the same +// destination-name resolution end-to-end through write_file: the bytes actually +// published to disk, not just the formatter helper's return value, must reflect +// the .prettierrc override and the .prettierignore rule. Resolving either +// against the ".zero-fmt-*.js" staging name would flip both assertions. +func TestFormatOnWritePrettierUsesDestinationFilename(t *testing.T) { + requirePrettier(t) + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + dir := t.TempDir() + if err := os.WriteFile(filepath.Join(dir, ".prettierrc"), []byte(`{"overrides":[{"files":"special.js","options":{"singleQuote":true}}]}`), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, ".prettierignore"), []byte("ignored.js\n"), 0o644); err != nil { + t.Fatal(err) + } + + specialPath := filepath.Join(dir, "special.js") + if err := os.WriteFile(specialPath, []byte("const x = \"a\";\n"), 0o644); err != nil { + t.Fatal(err) + } + write := NewScopedWriteFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "special.js", + "content": "const x = \"a\";\n", + "overwrite": true, + }, RunOptions{}) + if write.Status != StatusOK { + t.Fatalf("write_file failed: %q", write.Output) + } + onDisk, err := os.ReadFile(specialPath) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(onDisk), "const x = 'a';") { + t.Fatalf("write_file must resolve .prettierrc against the destination filename, got %q", onDisk) + } + if strings.Contains(string(onDisk), "\"a\"") { + t.Fatalf("write_file published the unformatted double-quoted content %q", onDisk) + } + + // The input has no spaces around "=", so default Prettier would rewrite it + // to `const x = "a";`. Honouring .prettierignore means those bytes are + // published unchanged; against the staging name they would not be ignored. + ignoredInput := "const x=\"a\";\n" + ignored := NewScopedWriteFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "ignored.js", + "content": ignoredInput, + }, RunOptions{}) + if ignored.Status != StatusOK { + t.Fatalf("write_file ignored.js failed: %q", ignored.Output) + } + ignoredOnDisk, err := os.ReadFile(filepath.Join(dir, "ignored.js")) + if err != nil { + t.Fatal(err) + } + if string(ignoredOnDisk) != ignoredInput { + t.Fatalf("write_file must honour .prettierignore for the destination filename, got %q", ignoredOnDisk) + } +} + const uglyGoSource = "package a\n\nfunc A( ) { }\n" func TestFormatOnWritePublishesFormattedBytesForWriteAndEdit(t *testing.T) { @@ -187,7 +288,9 @@ func TestFormatOnWriteFailureLeavesDestinationIntactUntilPublish(t *testing.T) { if [ -n "$ZERO_FORMAT_PROBE" ]; then cp "$ZERO_FORMAT_PROBE" "$ZERO_FORMAT_PROBE_SAW" || true fi -printf 'PARTIAL' > "$1" +path= +for a in "$@"; do path="$a"; done +printf 'PARTIAL' > "$path" exit 1 `) t.Setenv("ZERO_FORMAT_PROBE", target) @@ -202,14 +305,17 @@ exit 1 } var result Result + wantPublished := "" switch toolName { case "write_file": + wantPublished = uglyGoSource result = NewScopedWriteFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ "path": "a.go", "content": uglyGoSource, "overwrite": true, }, RunOptions{FileTracker: tracker}) default: + wantPublished = "package a\n\nfunc B( ) { }\n" result = NewScopedEditFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ "path": "a.go", "old_string": "func Original() {}", @@ -237,11 +343,109 @@ exit 1 if strings.Contains(string(onDisk), "PARTIAL") { t.Fatalf("%s published formatter-partial bytes: %q", toolName, onDisk) } + // The failed formatter must not have scribbled on the destination: + // the fallback staged bytes are what gets published. + if string(onDisk) != wantPublished { + t.Fatalf("%s destination = %q, want the fallback staged bytes %q", toolName, onDisk, wantPublished) + } assertTrackerMatchesDisk(t, tracker, target) }) } } +func TestFormatOnWriteRefusesWhenDestinationChangesDuringFormat(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("fake formatter shim is a POSIX script") + } + t.Setenv("ZERO_FORMAT_ON_WRITE", "1") + + for _, toolName := range []string{"write_file", "edit_file"} { + t.Run(toolName, func(t *testing.T) { + dir := t.TempDir() + target := filepath.Join(dir, "a.go") + original := "package a\n\nfunc Original() {}\n" + if err := os.WriteFile(target, []byte(original), 0o644); err != nil { + t.Fatal(err) + } + ready := filepath.Join(dir, "fmt-ready") + release := filepath.Join(dir, "fmt-release") + installFakeGofmt(t, `#!/bin/sh +: > "$ZERO_FORMAT_READY" +while [ ! -f "$ZERO_FORMAT_RELEASE" ]; do sleep 0.01; done +exit 0 +`) + t.Setenv("ZERO_FORMAT_READY", ready) + t.Setenv("ZERO_FORMAT_RELEASE", release) + + tracker := NewFileTracker() + read := NewScopedReadFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + }, RunOptions{FileTracker: tracker}) + if read.Status != StatusOK { + t.Fatalf("read_file failed: %q", read.Output) + } + + resultCh := make(chan Result, 1) + go func() { + switch toolName { + case "write_file": + resultCh <- NewScopedWriteFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + "content": uglyGoSource, + "overwrite": true, + }, RunOptions{FileTracker: tracker}) + default: + resultCh <- NewScopedEditFileTool(dir, nil).(optionsAwareTool).RunWithOptions(context.Background(), map[string]any{ + "path": "a.go", + "old_string": "func Original() {}", + "new_string": "func B( ) { }", + }, RunOptions{FileTracker: tracker}) + } + }() + + waitForFile(t, ready) + external := "package a\n\nfunc External() {}\n" + if err := os.WriteFile(target, []byte(external), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(release, nil, 0o644); err != nil { + t.Fatal(err) + } + + var result Result + select { + case result = <-resultCh: + case <-time.After(15 * time.Second): + t.Fatal("tool did not return after the formatter was released") + } + if result.Status != StatusError || !strings.Contains(result.Output, "changed on disk") { + t.Fatalf("%s must refuse when the destination changed during formatting, got %q", toolName, result.Output) + } + onDisk, err := os.ReadFile(target) + if err != nil { + t.Fatal(err) + } + if string(onDisk) != external { + t.Fatalf("external bytes were clobbered: got %q, want %q", onDisk, external) + } + }) + } +} + +func waitForFile(t *testing.T, path string) { + t.Helper() + deadline := time.Now().Add(10 * time.Second) + for { + if _, err := os.Stat(path); err == nil { + return + } + if time.Now().After(deadline) { + t.Fatalf("timed out waiting for %s", path) + } + time.Sleep(10 * time.Millisecond) + } +} + func assertFormattedOnDiskAndTracked(t *testing.T, tracker *FileTracker, path, preview string) { t.Helper() onDisk, err := os.ReadFile(path) diff --git a/internal/tools/write_file.go b/internal/tools/write_file.go index 33bf13ab7..2e6847f00 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,9 +96,12 @@ 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 := "" + priorReadOK := true if existed { if prev, rerr := os.ReadFile(absolutePath); rerr == nil { priorContent = string(prev) + } else { + priorReadOK = false } } @@ -114,6 +118,16 @@ func (tool writeFileTool) RunWithOptions(ctx context.Context, args map[string]an // destination in place after publication would reintroduce partial writes. formatting := maybeFormatWrittenFile(ctx, absolutePath, content) content = formatting.Content + if existed { + current, rerr := os.ReadFile(absolutePath) + if rerr != nil || !priorReadOK || !bytes.Equal(current, []byte(priorContent)) { + return errorResult(fileConflictMessage(relativePath)) + } + } else if _, serr := os.Stat(absolutePath); serr == nil { + return errorResult(fileConflictMessage(relativePath)) + } else if !os.IsNotExist(serr) { + return errorResult("Error writing file " + relativePath + ": " + serr.Error()) + } cleanupWarning, err := committedWrite(absolutePath, []byte(content), 0o644) if err != nil { return errorResult("Error writing file " + relativePath + ": " + err.Error())