Simplify atomic file replacement and bounded reads

docs/internal/fileops.md

@@ -1,17 +1,13 @@
 # Internal: File Operations
 
-`internal/fileops` owns the narrow mechanics for durable replacement of one
+`internal/fileops` owns the narrow mechanics for atomically visible replacement of one
 byte file. Callers keep ownership of serialization, validation, cancellation,
 and destination-directory policy.
 
-## Destination Confinement
+## Destination Paths
 
-Before it creates, replaces, or installs a destination file, `fileops` opens
+Ordinary workspace replacement uses standard operating-system path semantics.
-each ancestor from the filesystem root and rejects symbolic links or components
+Callers with a root-scoped safety contract own that validation at their boundary.
-that change during traversal. The resulting parent-directory handle is retained
-for sibling temporary-file creation and rename, so a later pathname swap cannot
-redirect the replacement. Existing destination symlinks are replaced as leaf
-entries; their targets are never followed.
 
 Remote object acquisition uses a writer supplied by the storage owner. The
 writer receives a `fileops`-owned, already-open sibling temporary file rather
@@ -31,41 +27,28 @@ tree through those directory handles. It rejects root deletion and any symlink
 encountered in the target path or tree; repeated removal of a missing target is
 successful. Command and post-publish policy remains owned by `internal/app`.
 
-## Confined Reads
+## Bounded Reads
 
-`ReadRegularFileUnderRoot` is the no-follow, bounded read primitive for a
+`ReadFileBounded` opens a normal path and reads at most its caller-supplied
-caller-selected root and relative file path; `ReadRegularFile` is its
+limit. Credential mode policy and environment precedence remain owned by
-path-based convenience wrapper. They verify every ancestor through directory
+`internal/app`.
-handles and admit only a stable regular-file handle. Callers enforce their own
-byte limits and access policy. Credential mode policy and environment
-precedence remain owned by `internal/app`.
 
 ## Replacement Contract
 
 `ReplaceFileAtomic` requires an existing destination directory. It creates a
 sibling temporary file, writes the complete byte sequence, applies the
-caller-supplied mode, syncs and closes the file, runs an optional pre-rename
+caller-supplied mode, closes the file, runs an optional pre-rename check, and
-check, replaces the destination with a rename, then syncs the containing
+replaces the destination with a rename.
-directory.
 
 The pre-rename check is the last point at which a caller can cancel without
 installing a new destination. A failure before the rename leaves the old
-destination unchanged and removes the temporary file; any cleanup failure is
+destination unchanged and removes the temporary file. Successful replacement
-returned alongside the primary failure. A failure after the rename may leave
+provides atomic visibility; it makes no crash- or power-loss durability claim.
-the new file visible, but it is not reported as crash-durable.
 
 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.
 
-## Directory-Sync Support
-
-Linux and macOS attempt to sync the destination directory. Windows opens the
-directory with backup semantics and flushes its buffers. If either operation
-is unavailable for the platform, directory handle, or filesystem,
-`ErrDirectorySyncUnsupported` is returned. Narratio does not treat that result
-as successful crash-durable replacement.
-
 `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.

docs/internal/manifest.md

@@ -155,14 +155,11 @@ path is removable after migration and is never used to write new state.
 
 - validates loaded documents;
 - normalizes missing maps/stage records;
-- writes through a sibling temporary file, syncing the completed file and
+- writes through a sibling temporary file and atomic replacement visibility;
-  destination directory after atomic replacement;
 - updates `updated_at` on save.
 
-If the operating system or filesystem cannot sync a directory, save returns an
+Manifest replacement provides atomic visibility, not a crash- or power-loss
-explicit error instead of claiming crash-durable replacement. A returned error
+durability guarantee. Callers reload after a returned save error before retrying.
-after the rename can therefore leave the new manifest visible but not confirmed
-durable; callers must reload it before retrying.
 
 ## Execution Semantics
 

docs/roadmap/implementation.md

@@ -173,7 +173,7 @@ During every stage:
 
 ## Stage 1 — Reduce Atomic Replacement, Modes, And Bounded Reads
 
-**Status: Pending**
+**Status: Completed**
 
 ### Goal
 

internal/fileops/fileops.go

@@ -12,9 +12,8 @@ import (
 	"strings"
 )
 
-// ErrDirectorySyncUnsupported reports that the platform or filesystem cannot
+// ErrDirectorySyncUnsupported remains for legacy confined migration paths.
-// make a renamed directory entry crash-durable. The replacement may be visible,
+// The retained replacement entry points do not perform directory syncing.
-// but callers must not treat it as having the same durability guarantee.
 var ErrDirectorySyncUnsupported = errors.New("directory sync is unsupported")
 
 // ReplaceFileOptions describes the caller-owned parts of a byte-file
@@ -26,13 +25,10 @@ type ReplaceFileOptions struct {
 	BeforeRename func() error
 }
 
-// ReplaceFileAtomic durably replaces dst with data. It creates a sibling
+// ReplaceFileAtomic replaces dst with data through a sibling temporary file.
-// temporary file, writes and syncs its bytes and mode, atomically renames it,
+// It provides atomic visibility, not a crash- or power-loss-durability claim.
-// then syncs the destination parent. On platforms where replacement of an
-// existing destination or directory syncing is unavailable, it returns an
-// error rather than claiming an equivalent durability guarantee.
 func ReplaceFileAtomic(dst string, data []byte, options ReplaceFileOptions) error {
-	return replaceFileFromReaderConfined(dst, bytes.NewReader(data), options)
+	return replaceFileFromReader(dst, bytes.NewReader(data), options)
 }
 
 // WriteFileAtomic creates the destination parent with workspace permissions
@@ -85,7 +81,7 @@ func WriteReaderAtomicWithChecksum(dst string, src io.Reader, perm os.FileMode)
 	}
 
 	digest := sha256.New()
-	if err := replaceFileFromReaderConfined(
+	if err := replaceFileFromReader(
 		dst,
 		io.TeeReader(src, digest),
 		ReplaceFileOptions{Mode: perm},
@@ -95,6 +91,46 @@ func WriteReaderAtomicWithChecksum(dst string, src io.Reader, perm os.FileMode)
 	return hex.EncodeToString(digest.Sum(nil)), nil
 }
 
+func replaceFileFromReader(dst string, source io.Reader, options ReplaceFileOptions) (resultErr error) {
+	if strings.TrimSpace(dst) == "" {
+		return fmt.Errorf("destination path is required")
+	}
+	tmp, err := os.CreateTemp(filepath.Dir(dst), "."+filepath.Base(dst)+".tmp-*")
+	if err != nil {
+		return fmt.Errorf("create temp file: %w", err)
+	}
+	tmpPath := tmp.Name()
+	removeTmp := true
+	defer func() {
+		if removeTmp {
+			if err := os.Remove(tmpPath); err != nil && !errors.Is(err, os.ErrNotExist) {
+				resultErr = errors.Join(resultErr, fmt.Errorf("remove temporary file: %w", err))
+			}
+		}
+	}()
+	if _, err := io.Copy(tmp, source); err != nil {
+		_ = tmp.Close()
+		return fmt.Errorf("write temp file: %w", err)
+	}
+	if err := tmp.Chmod(options.Mode); err != nil {
+		_ = tmp.Close()
+		return fmt.Errorf("set temp file permissions: %w", err)
+	}
+	if err := tmp.Close(); err != nil {
+		return fmt.Errorf("close temp file: %w", err)
+	}
+	if options.BeforeRename != nil {
+		if err := options.BeforeRename(); err != nil {
+			return fmt.Errorf("check before file replacement: %w", err)
+		}
+	}
+	if err := os.Rename(tmpPath, dst); err != nil {
+		return fmt.Errorf("install temp file: %w", err)
+	}
+	removeTmp = false
+	return nil
+}
+
 // InstallDownloadedTempFile installs a fully downloaded sibling temporary file
 // at dst. On failure before the rename, the caller retains ownership of tmpPath
 // and is responsible for cleanup.

internal/fileops/fileops_test.go

@@ -139,30 +139,23 @@ func TestInstallDownloadedTempFileSetsPermissions(t *testing.T) {
 	}
 }
 
-func TestWriteFileAtomicRejectsSymlinkedDestinationAncestor(t *testing.T) {
+func TestWriteFileAtomicUsesOrdinaryDestinationPathSemantics(t *testing.T) {
 	root := t.TempDir()
 	outside := t.TempDir()
-	sentinel := filepath.Join(outside, "sentinel.txt")
-	if err := os.WriteFile(sentinel, []byte("unchanged"), 0o600); err != nil {
-		t.Fatalf("WriteFile(sentinel) error = %v", err)
-	}
 	if err := os.Symlink(outside, filepath.Join(root, "redirect")); err != nil {
 		t.Skipf("Symlink unavailable: %v", err)
 	}
 
-	err := WriteFileAtomic(filepath.Join(root, "redirect", "output.txt"), []byte("new"), 0o640)
+	path := filepath.Join(root, "redirect", "output.txt")
-	if err == nil {
+	if err := WriteFileAtomic(path, []byte("new"), 0o640); err != nil {
-		t.Fatal("WriteFileAtomic() error = nil, want symlink ancestor rejection")
+		t.Fatalf("WriteFileAtomic() error = %v", err)
 	}
-	data, err := os.ReadFile(sentinel)
+	data, err := os.ReadFile(filepath.Join(outside, "output.txt"))
 	if err != nil {
-		t.Fatalf("ReadFile(sentinel) error = %v", err)
+		t.Fatalf("ReadFile(output) error = %v", err)
 	}
-	if string(data) != "unchanged" {
+	if string(data) != "new" {
-		t.Fatalf("outside sentinel = %q, want unchanged", data)
+		t.Fatalf("outside output = %q, want new", data)
-	}
-	if _, err := os.Stat(filepath.Join(outside, "output.txt")); !os.IsNotExist(err) {
-		t.Fatalf("outside output exists: stat err = %v", err)
 	}
 }
 

internal/fileops/modes.go

@@ -3,6 +3,7 @@ package fileops
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	"strings"
 )
 
@@ -22,8 +23,38 @@ func EnsureWorkspaceDirectory(directory string) error {
 		return fmt.Errorf("workspace directory is required")
 	}
 
-	if err := ensureConfinedDirectory(directory, WorkspaceDirectoryMode); err != nil {
+	clean := filepath.Clean(directory)
-		return fmt.Errorf("set workspace directory permissions %q: %w", directory, err)
+	ancestor := clean
+	var missing []string
+	for {
+		info, err := os.Stat(ancestor)
+		if err == nil {
+			if !info.IsDir() {
+				return fmt.Errorf("workspace directory %q is not a directory", ancestor)
+			}
+			break
+		}
+		if !os.IsNotExist(err) {
+			return fmt.Errorf("inspect workspace directory %q: %w", ancestor, err)
+		}
+		parent := filepath.Dir(ancestor)
+		if parent == ancestor {
+			return fmt.Errorf("find workspace directory ancestor for %q", clean)
+		}
+		missing = append(missing, ancestor)
+		ancestor = parent
+	}
+	for index := len(missing) - 1; index >= 0; index-- {
+		created := missing[index]
+		if err := os.Mkdir(created, WorkspaceDirectoryMode); err != nil && !os.IsExist(err) {
+			return fmt.Errorf("create workspace directory %q: %w", created, err)
+		}
+		if err := os.Chmod(created, WorkspaceDirectoryMode); err != nil {
+			return fmt.Errorf("set workspace directory permissions %q: %w", created, err)
+		}
+	}
+	if err := os.Chmod(clean, WorkspaceDirectoryMode); err != nil {
+		return fmt.Errorf("set workspace directory permissions %q: %w", clean, err)
 	}
 	return nil
 }

internal/fileops/read_bounded.go

@@ -0,0 +1,32 @@
+package fileops
+
+import (
+	"fmt"
+	"io"
+	"os"
+	"strings"
+)
+
+// ReadFileBounded reads at most limit bytes from path using ordinary workspace
+// path semantics. It does not return partial data when the limit is exceeded.
+func ReadFileBounded(path string, limit int64) ([]byte, error) {
+	if strings.TrimSpace(path) == "" {
+		return nil, fmt.Errorf("file path is required")
+	}
+	if limit < 0 {
+		return nil, fmt.Errorf("file read limit %d is invalid", limit)
+	}
+	file, err := os.Open(path)
+	if err != nil {
+		return nil, fmt.Errorf("open file %q: %w", path, err)
+	}
+	defer func() { _ = file.Close() }()
+	data, err := io.ReadAll(io.LimitReader(file, limit+1))
+	if err != nil {
+		return nil, fmt.Errorf("read file %q: %w", path, err)
+	}
+	if int64(len(data)) > limit {
+		return nil, fmt.Errorf("file %q exceeds %d-byte limit", path, limit)
+	}
+	return data, nil
+}