From be7b5e321a4575e03d5b81c9484776d549a1237f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 9 May 2026 05:16:26 +0000 Subject: [PATCH 1/3] Initial plan From 1836db71d1801d23eb4ecf58d460c977ef8ca624 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 9 May 2026 05:22:48 +0000 Subject: [PATCH 2/3] Fix CopyFile close error handling Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/fileutil/fileutil.go | 37 +++++++++++++++------ pkg/fileutil/fileutil_test.go | 61 +++++++++++++++++++++++++++++++++++ 2 files changed, 88 insertions(+), 10 deletions(-) diff --git a/pkg/fileutil/fileutil.go b/pkg/fileutil/fileutil.go index da5353ec12b..4cbd971ad7d 100644 --- a/pkg/fileutil/fileutil.go +++ b/pkg/fileutil/fileutil.go @@ -116,6 +116,29 @@ func IsDirEmpty(path string) bool { return len(files) == 0 } +type syncWriteCloser interface { + io.Writer + Sync() error + Close() error +} + +func copyFileContents(in io.ReadCloser, out syncWriteCloser, dst string) (err error) { + defer func() { + if closeErr := out.Close(); closeErr != nil && err == nil { + err = closeErr + } + }() + + if _, err = io.Copy(out, in); err != nil { + if removeErr := os.Remove(dst); removeErr != nil { + log.Printf("Failed to remove partial destination file during cleanup: %s", removeErr) + } + return err + } + + return out.Sync() +} + // CopyFile copies a file from src to dst using buffered IO. func CopyFile(src, dst string) error { log.Printf("Copying file: src=%s, dst=%s", src, dst) @@ -131,17 +154,11 @@ func CopyFile(src, dst string) error { log.Printf("Failed to create destination file: %s", err) return err } - defer func() { _ = out.Close() }() - - if _, err = io.Copy(out, in); err != nil { - if closeErr := out.Close(); closeErr != nil { - log.Printf("Failed to close destination file during cleanup: %s", closeErr) - } - if removeErr := os.Remove(dst); removeErr != nil { - log.Printf("Failed to remove partial destination file during cleanup: %s", removeErr) - } + err = copyFileContents(in, out, dst) + if err != nil { return err } + log.Printf("File copied successfully: src=%s, dst=%s", src, dst) - return out.Sync() + return nil } diff --git a/pkg/fileutil/fileutil_test.go b/pkg/fileutil/fileutil_test.go index fcc8d0a0e56..ca86d82eec5 100644 --- a/pkg/fileutil/fileutil_test.go +++ b/pkg/fileutil/fileutil_test.go @@ -5,6 +5,8 @@ package fileutil import ( "archive/tar" "bytes" + "errors" + "io" "os" "path/filepath" "runtime" @@ -15,6 +17,34 @@ import ( "github.com/stretchr/testify/require" ) +type stubSyncWriteCloser struct { + buf bytes.Buffer + writeErr error + syncErr error + closeErr error + closeCalls int +} + +func (s *stubSyncWriteCloser) Write(p []byte) (int, error) { + if s.writeErr != nil { + return 0, s.writeErr + } + return s.buf.Write(p) +} + +func (s *stubSyncWriteCloser) Sync() error { + return s.syncErr +} + +func (s *stubSyncWriteCloser) Close() error { + s.closeCalls++ + return s.closeErr +} + +func (s *stubSyncWriteCloser) String() string { + return s.buf.String() +} + func TestValidateAbsolutePath(t *testing.T) { tests := []struct { name string @@ -344,6 +374,37 @@ func TestCopyFile(t *testing.T) { }) } +func TestCopyFileContents(t *testing.T) { + t.Run("returns close error after successful sync", func(t *testing.T) { + closeErr := errors.New("close failed") + out := &stubSyncWriteCloser{closeErr: closeErr} + + err := copyFileContents(io.NopCloser(strings.NewReader("hello")), out, filepath.Join(t.TempDir(), "dst.txt")) + + require.ErrorIs(t, err, closeErr) + assert.Equal(t, 1, out.closeCalls, "destination should be closed once") + assert.Equal(t, "hello", out.String(), "content should be copied before close") + }) + + t.Run("preserves copy error and closes destination once", func(t *testing.T) { + writeErr := errors.New("write failed") + closeErr := errors.New("close failed") + out := &stubSyncWriteCloser{ + writeErr: writeErr, + closeErr: closeErr, + } + + dst := filepath.Join(t.TempDir(), "dst.txt") + require.NoError(t, os.WriteFile(dst, []byte("partial"), 0600), "Should create destination placeholder") + + err := copyFileContents(io.NopCloser(strings.NewReader("hello")), out, dst) + + require.ErrorIs(t, err, writeErr) + assert.Equal(t, 1, out.closeCalls, "destination should be closed once during cleanup") + assert.NoFileExists(t, dst, "partial destination should be removed after copy failure") + }) +} + func TestValidatePathWithinBase(t *testing.T) { base := t.TempDir() From e65499e232f1d948b2c4292f15d4049a2ec034bb Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 9 May 2026 05:45:46 +0000 Subject: [PATCH 3/3] Address CopyFile review feedback Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com> --- pkg/fileutil/fileutil.go | 13 +++++++++---- pkg/fileutil/fileutil_test.go | 5 ++--- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/pkg/fileutil/fileutil.go b/pkg/fileutil/fileutil.go index 4cbd971ad7d..34419bc6d86 100644 --- a/pkg/fileutil/fileutil.go +++ b/pkg/fileutil/fileutil.go @@ -122,17 +122,22 @@ type syncWriteCloser interface { Close() error } -func copyFileContents(in io.ReadCloser, out syncWriteCloser, dst string) (err error) { +func copyFileContents(in io.Reader, out syncWriteCloser, dst string) (err error) { + removePartial := false + defer func() { if closeErr := out.Close(); closeErr != nil && err == nil { err = closeErr } + if removePartial { + if removeErr := os.Remove(dst); removeErr != nil { + log.Printf("Failed to remove partial destination file during cleanup: %s", removeErr) + } + } }() if _, err = io.Copy(out, in); err != nil { - if removeErr := os.Remove(dst); removeErr != nil { - log.Printf("Failed to remove partial destination file during cleanup: %s", removeErr) - } + removePartial = true return err } diff --git a/pkg/fileutil/fileutil_test.go b/pkg/fileutil/fileutil_test.go index ca86d82eec5..cfb78f50421 100644 --- a/pkg/fileutil/fileutil_test.go +++ b/pkg/fileutil/fileutil_test.go @@ -6,7 +6,6 @@ import ( "archive/tar" "bytes" "errors" - "io" "os" "path/filepath" "runtime" @@ -379,7 +378,7 @@ func TestCopyFileContents(t *testing.T) { closeErr := errors.New("close failed") out := &stubSyncWriteCloser{closeErr: closeErr} - err := copyFileContents(io.NopCloser(strings.NewReader("hello")), out, filepath.Join(t.TempDir(), "dst.txt")) + err := copyFileContents(strings.NewReader("hello"), out, filepath.Join(t.TempDir(), "dst.txt")) require.ErrorIs(t, err, closeErr) assert.Equal(t, 1, out.closeCalls, "destination should be closed once") @@ -397,7 +396,7 @@ func TestCopyFileContents(t *testing.T) { dst := filepath.Join(t.TempDir(), "dst.txt") require.NoError(t, os.WriteFile(dst, []byte("partial"), 0600), "Should create destination placeholder") - err := copyFileContents(io.NopCloser(strings.NewReader("hello")), out, dst) + err := copyFileContents(strings.NewReader("hello"), out, dst) require.ErrorIs(t, err, writeErr) assert.Equal(t, 1, out.closeCalls, "destination should be closed once during cleanup")