diff --git a/docs/internal/fileops.md b/docs/internal/fileops.md index 1cc67d1..320fe8e 100644 --- a/docs/internal/fileops.md +++ b/docs/internal/fileops.md @@ -9,10 +9,11 @@ and destination-directory policy. Ordinary workspace replacement uses standard operating-system path semantics. Callers with a root-scoped safety contract own that validation at their boundary. -Remote object acquisition uses a writer supplied by the storage owner. The -writer receives a `fileops`-owned, already-open sibling temporary file rather -than a mutable destination path. Callers still own remote object selection, -validation, conflict handling, and final mode. +Remote object acquisition is owned by its domain caller. The caller writes a +sibling temporary file, validates the complete result, and passes that file to +`InstallSiblingTempFile` only when it is ready to become canonical. Callers own +remote object selection, validation, conflict handling, and temporary-file +cleanup. Directory bundles are not promoted by `internal/fileops`; extraction owns its validated Notarius bundle rename. @@ -48,6 +49,7 @@ Replacement follows the operating system's same-filesystem rename semantics. If a platform cannot replace an existing destination, the operation returns an error and never removes the old file as an emulation step. -`WriteFileAtomic`, copy helpers, and downloaded temporary-file installation -retain their compatibility behavior of creating the destination parent with -the repository's workspace permissions before using this contract. +`WriteFileAtomic` and the copy helpers create the destination parent with the +repository's workspace permissions. `InstallSiblingTempFile` requires the +caller to provide an existing temporary file in the existing destination +directory. diff --git a/internal/fileops/fileops_test.go b/internal/fileops/fileops_test.go index 8a02f59..dd613a9 100644 --- a/internal/fileops/fileops_test.go +++ b/internal/fileops/fileops_test.go @@ -3,6 +3,7 @@ package fileops import ( "crypto/sha256" "encoding/hex" + "errors" "os" "path/filepath" "strings" @@ -45,6 +46,33 @@ func TestWriteFileAtomicCleansTempFileOnInstallFailure(t *testing.T) { assertNoMatchingTempFiles(t, root, ".blocked.tmp-") } +func TestReplaceFileAtomicCancellationPreservesDestinationAndCleansTemp(t *testing.T) { + root := t.TempDir() + dst := filepath.Join(root, "value.txt") + if err := os.WriteFile(dst, []byte("old"), 0o644); err != nil { + t.Fatalf("WriteFile(destination) error = %v", err) + } + wantErr := errors.New("cancel replacement") + + err := ReplaceFileAtomic(dst, []byte("new"), ReplaceFileOptions{ + Mode: WorkspaceFileMode, + BeforeRename: func() error { + return wantErr + }, + }) + if !errors.Is(err, wantErr) { + t.Fatalf("ReplaceFileAtomic() error = %v, want wrapped cancellation", err) + } + data, err := os.ReadFile(dst) + if err != nil { + t.Fatalf("ReadFile(destination) error = %v", err) + } + if string(data) != "old" { + t.Fatalf("destination content = %q, want old content", data) + } + assertNoMatchingTempFiles(t, root, ".value.txt.tmp-") +} + func TestCopyFileAtomicWithChecksumMatchesDestination(t *testing.T) { root := t.TempDir() src := filepath.Join(root, "source.txt") diff --git a/internal/fileops/read_bounded.go b/internal/fileops/read_bounded.go index 03bb1e3..35d5e1e 100644 --- a/internal/fileops/read_bounded.go +++ b/internal/fileops/read_bounded.go @@ -3,6 +3,7 @@ package fileops import ( "fmt" "io" + "math" "os" "strings" ) @@ -21,7 +22,11 @@ func ReadFileBounded(path string, limit int64) ([]byte, error) { return nil, fmt.Errorf("open file %q: %w", path, err) } defer func() { _ = file.Close() }() - data, err := io.ReadAll(io.LimitReader(file, limit+1)) + readLimit := limit + if limit < math.MaxInt64 { + readLimit++ + } + data, err := io.ReadAll(io.LimitReader(file, readLimit)) if err != nil { return nil, fmt.Errorf("read file %q: %w", path, err) } diff --git a/internal/fileops/read_bounded_test.go b/internal/fileops/read_bounded_test.go new file mode 100644 index 0000000..1b8abcb --- /dev/null +++ b/internal/fileops/read_bounded_test.go @@ -0,0 +1,61 @@ +package fileops + +import ( + "math" + "os" + "path/filepath" + "strings" + "testing" +) + +func TestReadFileBoundedEnforcesLimitWithoutPartialContent(t *testing.T) { + root := t.TempDir() + path := filepath.Join(root, "value.txt") + if err := os.WriteFile(path, []byte("12345"), 0o644); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + + data, err := ReadFileBounded(path, 5) + if err != nil { + t.Fatalf("ReadFileBounded(exact limit) error = %v", err) + } + if string(data) != "12345" { + t.Fatalf("ReadFileBounded(exact limit) = %q, want complete content", data) + } + + data, err = ReadFileBounded(path, 4) + if err == nil || !strings.Contains(err.Error(), "4-byte limit") { + t.Fatalf("ReadFileBounded(limit plus one) data = %q error = %v, want limit error", data, err) + } + if data != nil { + t.Fatalf("ReadFileBounded(limit plus one) returned partial data %q", data) + } +} + +func TestReadFileBoundedHandlesBoundaryLimits(t *testing.T) { + root := t.TempDir() + emptyPath := filepath.Join(root, "empty.txt") + valuePath := filepath.Join(root, "value.txt") + if err := os.WriteFile(emptyPath, nil, 0o644); err != nil { + t.Fatalf("WriteFile(empty) error = %v", err) + } + if err := os.WriteFile(valuePath, []byte("value"), 0o644); err != nil { + t.Fatalf("WriteFile(value) error = %v", err) + } + + if data, err := ReadFileBounded(emptyPath, 0); err != nil || len(data) != 0 { + t.Fatalf("ReadFileBounded(empty, zero) data = %q error = %v", data, err) + } + if data, err := ReadFileBounded(valuePath, 0); err == nil || data != nil { + t.Fatalf("ReadFileBounded(value, zero) data = %q error = %v, want limit error", data, err) + } + if data, err := ReadFileBounded(valuePath, math.MaxInt64); err != nil || string(data) != "value" { + t.Fatalf("ReadFileBounded(max limit) data = %q error = %v", data, err) + } + if _, err := ReadFileBounded(valuePath, -1); err == nil || !strings.Contains(err.Error(), "invalid") { + t.Fatalf("ReadFileBounded(negative limit) error = %v, want invalid limit", err) + } + if _, err := ReadFileBounded(" ", 1); err == nil || !strings.Contains(err.Error(), "path is required") { + t.Fatalf("ReadFileBounded(empty path) error = %v, want required path", err) + } +}