Bubble errors up from worker goroutines; exit non-zero on failure (#66)

* Bubble errors up from worker goroutines; exit non-zero on failure

Replace every log.Fatal* in find_replace.go and file_handling.go with
returned errors so deferred cleanup can run, then collect errors across
walker goroutines and surface them at main.

Architecture:

- File methods (NewFile, Info, Read, Write, plus a new error-returning
  Mode) now return (T, error). The walker calls Info on each child so
  Write can rely on cached Mode without re-statting.
- findReplace.{WalkDir,HandleFile,RenameFile,ReplaceContents} return
  errors (WalkDir records them on an embedded accumulator so its
  goroutines can stay fire-and-forget).
- errAccumulator is a sync.Mutex-guarded []error with an errors.Join
  exit point. Each error is also log.Print'd at the point of failure so
  the existing operator-visible stderr UX is preserved; the join exists
  so main can scrape stderr and so errors.Is unwraps the whole chain.
- main is now a thin os.Exit wrapper around a testable run(args, stderr)
  that returns 1 on bad arg count or any traversal error and 0 on a
  clean walk.
- File.Write now defer-Removes its temp file immediately after creating
  it; on a successful rename the file no longer exists at tempName so
  the deferred remove is a no-op, but on a rename failure the temp file
  is cleaned up instead of being leaked.

New tests (each was confirmed to fail for the right reason before the
matching production change was finalized):

- TestWalkDir_PermissionDeniedSubdirContinues: a chmod-0 subdirectory
  no longer aborts the walk; the sibling subtree is rewritten and the
  walker records an fs.ErrPermission referencing the failing path.
  Skips under root (where chmod 0 is bypassed) and Windows.
- TestRenameFile_ReturnsErrorOnExistingDestination: an occupied
  destination is refused with an error, not log.Fatal.
- TestWalkDir_BadRenameTargetDoesNotAbortSiblings: a sibling whose
  post-rename name is already occupied does not abort the rest of the
  tree; the walker logs and records the failure and continues.
- TestWriteCleansUpTempFileOnRenameFailure: forcing the rename step to
  fail (target is a non-empty directory; deterministic across users)
  leaves no stray temp file behind.
- TestRun_{ExitsZeroOnSuccess,ExitsNonZeroOnTraversalError,
  BadArgCountPrintsUsage}: run() returns 0 on a clean walk, non-zero
  when any error was recorded, and non-zero with a usage line on bad
  arg count.

Closes #6
Closes #11
Closes #5

https://claude.ai/code/session_01Tep5t8h97Q9KKbpLMbUEJr

* Bump go.mod to 1.20 to enable errors.Join

The error-aggregation primitive introduced in this PR uses errors.Join,
which was added in Go 1.20. CI was pinned to 1.19 via go.mod and 'go vet'
failed with 'Join not declared by package errors'.

This is the minimum bump needed for the chosen primitive. The broader
'be on a supported Go release' work (Go 1.19 has been EOL since August
2023) stays as #28.

* Remove deprecated rand.Seed call

Since Go 1.20 (the new toolchain floor in this PR), the math/rand global
generator is automatically seeded by the runtime; explicit rand.Seed is
a deprecated no-op and staticcheck (run by golangci-lint) flags it as
SA1019.

The line was already vestigial -- left in place by the original commit
to keep the diff narrow. The toolchain bump in the previous commit makes
that compromise no longer viable.

strings.go's RandomString still uses math/rand.Intn; the change to
crypto/rand for filesystem paths remains tracked under issue #3.

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: dolph

file_handling.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"fmt"
 	"io"
 	"log"
 	"os"
@@ -15,12 +16,15 @@ type File struct {
 	info os.FileInfo
 }
 
-func NewFile(path string) *File {
+// NewFile resolves path to an absolute path and wraps it in a *File. It
+// returns an error if the working directory cannot be determined (the only
+// failure mode of filepath.Abs).
+func NewFile(path string) (*File, error) {
 	absPath, err := filepath.Abs(path)
 	if err != nil {
-		log.Fatalf("Unable to resolve absolute path of %v: %v", path, err)
+		return nil, fmt.Errorf("resolve absolute path of %v: %w", path, err)
 	}
-	return &File{Path: absPath}
+	return &File{Path: absPath}, nil
 }
 
 func (f *File) Base() string {
@@ -31,58 +35,81 @@ func (f *File) Dir() string {
 	return filepath.Dir(f.Path)
 }
 
-func (f *File) Info() os.FileInfo {
+// Info lazily stats the file and caches the result. It returns an error if
+// the underlying os.Stat fails.
+func (f *File) Info() (os.FileInfo, error) {
 	if f.info == nil {
 		stat, err := os.Stat(f.Path)
 		if err != nil {
-			log.Fatalf("Failed to stat %v: %v", f.Path, err)
+			return nil, fmt.Errorf("stat %v: %w", f.Path, err)
 		}
 		f.info = stat
 	}
-	return f.info
+	return f.info, nil
 }
 
-func (f *File) Mode() os.FileMode {
-	return f.Info().Mode()
+// Mode returns the cached mode bits. It is only safe to call after Info() has
+// succeeded; callers that have a *File handed to them by the walker can rely
+// on that precondition because the walker calls Info() before dispatching.
+func (f *File) Mode() (os.FileMode, error) {
+	info, err := f.Info()
+	if err != nil {
+		return 0, err
+	}
+	return info.Mode(), nil
 }
 
-// Read the file into a string.
-func (f *File) Read() string {
+// Read reads the file into a string, or returns the empty string for binary
+// files. An error indicates the file could not be opened or fully read; the
+// caller should log-and-skip rather than abort.
+func (f *File) Read() (string, error) {
 	handle, err := os.Open(f.Path)
 	if err != nil {
-		log.Fatalf("Unable to open %v: %v", f.Path, err)
+		return "", fmt.Errorf("open %v: %w", f.Path, err)
 	}
 	defer handle.Close()
 
 	// Check if the file looks like text before reading the entire file.
 	var buf [1024]byte
 	n, err := handle.Read(buf[0:])
 	if err != nil || !util.IsText(buf[0:n]) {
-		return ""
+		return "", nil
 	}
 
 	// Reset file handle so we can read the entire file.
 	if _, err := handle.Seek(0, io.SeekStart); err != nil {
-		log.Fatalf("Failed to seek back to beginning of %v: %v", f.Path, err)
+		return "", fmt.Errorf("seek to start of %v: %w", f.Path, err)
 	}
 
 	builder := new(strings.Builder)
 	if _, err := io.Copy(builder, handle); err != nil {
-		log.Fatalf("Failed to read %v to a string: %v", f.Path, err)
+		return "", fmt.Errorf("read %v: %w", f.Path, err)
 	}
-	return builder.String()
+	return builder.String(), nil
 }
 
-// Write content to file atomically, by writing it to a temporary file first,
-// and then moving it to the destination, overwriting the original.
-func (f *File) Write(content string) {
+// Write atomically replaces the file with content, via a temp file + rename.
+// A deferred os.Remove(tempName) ensures the temp file is cleaned up if any
+// step after its creation fails (including the rename); on success the remove
+// is a no-op because the file has already been renamed away.
+func (f *File) Write(content string) error {
+	mode, err := f.Mode()
+	if err != nil {
+		return err
+	}
+
 	tempName := filepath.Join(f.Dir(), RandomString(20))
-	if err := os.WriteFile(tempName, []byte(content), f.Mode()); err != nil {
-		log.Fatalf("Error creating tempfile in %v: %v", f.Dir(), err)
+	if err := os.WriteFile(tempName, []byte(content), mode); err != nil {
+		return fmt.Errorf("create tempfile in %v: %w", f.Dir(), err)
 	}
+	// Make sure the temp file is removed if the rename below fails. On
+	// success, the rename has already moved the file to f.Path so this is
+	// a no-op (we deliberately ignore the not-exist error).
+	defer os.Remove(tempName)
 
 	log.Printf("Rewriting %v", f.Path)
 	if err := os.Rename(tempName, f.Path); err != nil {
-		log.Fatalf("Unable to atomically move temp file %v to %v: %v", tempName, f.Path, err)
+		return fmt.Errorf("atomically move temp file %v to %v: %w", tempName, f.Path, err)
 	}
+	return nil
 }

file_handling_test.go

@@ -5,11 +5,7 @@ import (
 	"testing"
 )
 
-// TestNewFile exercises NewFile's path-resolution behavior. It does NOT cover
-// the filepath.Abs error path: NewFile currently calls log.Fatalf on that
-// failure, which would kill the test binary, and that surface is not reachable
-// on most platforms in any case. The error path will become testable when
-// NewFile is refactored to return an error (see issue #6).
+// TestNewFile exercises NewFile's path-resolution behavior.
 func TestNewFile(t *testing.T) {
 	tmp := t.TempDir()
 
@@ -65,7 +61,10 @@ func TestNewFile(t *testing.T) {
 				want = abs
 			}
 
-			got := NewFile(tc.input)
+			got, err := NewFile(tc.input)
+			if err != nil {
+				t.Fatalf("NewFile(%q) returned unexpected error: %v", tc.input, err)
+			}
 			if got == nil {
 				t.Fatalf("NewFile(%q) returned nil", tc.input)
 			}