Move review preferences from project settings to local preferences

cmd/entire/cli/jsonutil/write.go

@@ -0,0 +1,72 @@
+package jsonutil
+
+import (
+	"fmt"
+	"io/fs"
+	"os"
+	"path/filepath"
+)
+
+// WriteFileAtomic writes data to filePath atomically by writing to a temp file
+// in the same directory, fsyncing it, renaming into place, and fsyncing the
+// parent directory. A crash or signal mid-write leaves the original file
+// intact rather than a truncated partial — important for config files like
+// .entire/settings.json that callers expect to remain parseable across
+// interrupted writes.
+//
+// The fsync between Write and Close guarantees the temp file's bytes are on
+// disk before the rename takes effect; without it, some filesystems (notably
+// ext4 with non-default mount options) can surface the rename as completed
+// while the file is still empty after a hard crash.
+//
+// The parent-directory fsync after rename guarantees the rename's directory
+// entry is durable. Without it, the file contents are on disk but the
+// directory may still point to the pre-rename state after a crash, so the
+// "leaves the original intact" promise would silently break. Windows does
+// not support directory fsync; we make this step best-effort so the call
+// does not fail on platforms where the operation is a no-op.
+//
+// perm is applied to the temp file via Chmod before rename so the final file
+// lands with the requested permission regardless of the temp file's default.
+func WriteFileAtomic(filePath string, data []byte, perm fs.FileMode) error {
+	dir := filepath.Dir(filePath)
+	base := filepath.Base(filePath)
+	tmp, err := os.CreateTemp(dir, base+".*.tmp")
+	if err != nil {
+		return fmt.Errorf("create temp for %s: %w", filePath, err)
+	}
+	tmpName := tmp.Name()
+	removeTmp := true
+	defer func() {
+		if removeTmp {
+			_ = os.Remove(tmpName)
+		}
+	}()
+	if _, err := tmp.Write(data); err != nil {
+		_ = tmp.Close()
+		return fmt.Errorf("write temp for %s: %w", filePath, err)
+	}
+	if err := tmp.Sync(); err != nil {
+		_ = tmp.Close()
+		return fmt.Errorf("sync temp for %s: %w", filePath, err)
+	}
+	if err := tmp.Close(); err != nil {
+		return fmt.Errorf("close temp for %s: %w", filePath, err)
+	}
+	if err := os.Chmod(tmpName, perm); err != nil {
+		return fmt.Errorf("chmod temp for %s: %w", filePath, err)
+	}
+	if err := os.Rename(tmpName, filePath); err != nil {
+		return fmt.Errorf("rename temp to %s: %w", filePath, err)
+	}
+	removeTmp = false
+	// Best-effort: the rename succeeded, so don't propagate failures here.
+	// Directory fsync isn't supported on Windows, and on POSIX an error
+	// after a successful rename would mislead callers who already have the
+	// file in place.
+	if d, err := os.Open(dir); err == nil { //nolint:gosec // G304: dir is filepath.Dir of caller-supplied filePath, not user input
+		_ = d.Sync() //nolint:errcheck // best-effort directory fsync; failure does not roll back the rename
+		_ = d.Close()
+	}
+	return nil
+}

cmd/entire/cli/jsonutil/write_test.go

@@ -0,0 +1,157 @@
+package jsonutil
+
+import (
+	"errors"
+	"os"
+	"path/filepath"
+	"runtime"
+	"strings"
+	"testing"
+)
+
+func TestWriteFileAtomic_CreatesNewFile(t *testing.T) {
+	t.Parallel()
+	dir := t.TempDir()
+	target := filepath.Join(dir, "out.json")
+	data := []byte(`{"hello":"world"}`)
+
+	if err := WriteFileAtomic(target, data, 0o644); err != nil {
+		t.Fatalf("WriteFileAtomic: %v", err)
+	}
+
+	got, err := os.ReadFile(target)
+	if err != nil {
+		t.Fatalf("read back: %v", err)
+	}
+	if string(got) != string(data) {
+		t.Errorf("content mismatch: got %q want %q", got, data)
+	}
+}
+
+func TestWriteFileAtomic_ReplacesExistingFile(t *testing.T) {
+	t.Parallel()
+	dir := t.TempDir()
+	target := filepath.Join(dir, "out.json")
+	if err := os.WriteFile(target, []byte("old contents"), 0o644); err != nil {
+		t.Fatalf("seed file: %v", err)
+	}
+
+	newData := []byte("new contents")
+	if err := WriteFileAtomic(target, newData, 0o644); err != nil {
+		t.Fatalf("WriteFileAtomic: %v", err)
+	}
+
+	got, err := os.ReadFile(target)
+	if err != nil {
+		t.Fatalf("read back: %v", err)
+	}
+	if string(got) != string(newData) {
+		t.Errorf("content not replaced: got %q want %q", got, newData)
+	}
+}
+
+// AppliesPermission verifies the Chmod-before-rename step actually lands the
+// requested mode on the final file. os.CreateTemp defaults to 0o600 so
+// without the Chmod a 0o644 caller would silently get a tighter mode.
+func TestWriteFileAtomic_AppliesPermission(t *testing.T) {
+	t.Parallel()
+	if runtime.GOOS == "windows" {
+		t.Skip("POSIX permission bits are not meaningful on Windows")
+	}
+	dir := t.TempDir()
+	target := filepath.Join(dir, "out.json")
+
+	if err := WriteFileAtomic(target, []byte("x"), 0o600); 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 != 0o600 {
+		t.Errorf("perm: got %#o want %#o", got, 0o600)
+	}
+}
+
+// LeavesNoTempOnSuccess guards against the removeTmp defer being skipped or
+// the temp suffix changing in a way that breaks cleanup.
+func TestWriteFileAtomic_LeavesNoTempOnSuccess(t *testing.T) {
+	t.Parallel()
+	dir := t.TempDir()
+	target := filepath.Join(dir, "out.json")
+
+	if err := WriteFileAtomic(target, []byte("x"), 0o644); err != nil {
+		t.Fatalf("WriteFileAtomic: %v", err)
+	}
+
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatalf("ReadDir: %v", err)
+	}
+	if len(entries) != 1 {
+		names := make([]string, 0, len(entries))
+		for _, e := range entries {
+			names = append(names, e.Name())
+		}
+		t.Errorf("expected exactly one entry in dir, got %d: %v", len(entries), names)
+	}
+	for _, e := range entries {
+		if strings.HasSuffix(e.Name(), ".tmp") {
+			t.Errorf("leftover temp file: %s", e.Name())
+		}
+	}
+}
+
+// CleansUpTempOnRenameFailure reaches the rename step and forces it to fail
+// (renaming a regular file onto a non-empty directory is rejected on every
+// POSIX filesystem, and on Windows). The removeTmp defer must clear the
+// orphan so /tmp doesn't accumulate junk across many failed writes.
+func TestWriteFileAtomic_CleansUpTempOnRenameFailure(t *testing.T) {
+	t.Parallel()
+	dir := t.TempDir()
+	target := filepath.Join(dir, "out.json")
+	if err := os.Mkdir(target, 0o755); err != nil {
+		t.Fatalf("mkdir target: %v", err)
+	}
+	if err := os.WriteFile(filepath.Join(target, "occupant"), []byte("x"), 0o644); err != nil {
+		t.Fatalf("seed dir: %v", err)
+	}
+
+	err := WriteFileAtomic(target, []byte("x"), 0o644)
+	if err == nil {
+		t.Fatal("expected error when target is a non-empty directory")
+	}
+
+	info, statErr := os.Stat(target)
+	if statErr != nil {
+		t.Fatalf("stat target: %v", statErr)
+	}
+	if !info.IsDir() {
+		t.Error("target should still be a directory after failed rename")
+	}
+
+	entries, err := os.ReadDir(dir)
+	if err != nil {
+		t.Fatalf("ReadDir: %v", err)
+	}
+	for _, e := range entries {
+		if strings.HasSuffix(e.Name(), ".tmp") {
+			t.Errorf("leftover temp file after failed rename: %s", e.Name())
+		}
+	}
+}
+
+func TestWriteFileAtomic_ParentMissing(t *testing.T) {
+	t.Parallel()
+	dir := t.TempDir()
+	target := filepath.Join(dir, "does-not-exist", "out.json")
+
+	err := WriteFileAtomic(target, []byte("x"), 0o644)
+	if err == nil {
+		t.Fatal("expected error when parent dir is missing")
+	}
+	if !errors.Is(err, os.ErrNotExist) {
+		t.Errorf("expected ErrNotExist; got: %v", err)
+	}
+}

cmd/entire/cli/review/cmd.go

@@ -110,8 +110,9 @@ func NewCommand(deps Deps) *cobra.Command {
 		// review --help` and the command works normally.
 		Hidden: true,
 		Short:  "Run configured review skills against the current branch",
-		Long: `Run the review skills configured in .entire/settings.json against
-the current branch. On first run, an interactive picker writes the config.
+		Long: `Run configured review skills against the current branch. Review
+preferences are loaded from Entire settings and clone-local preferences. On
+first run, an interactive picker writes clone-local preferences.
 
 Labs entry: review is experimental. We are actively refining it based on user
 feedback.
@@ -162,6 +163,22 @@ Subcommands:
 			if modes > 1 {
 				return errors.New("--edit, --findings, and --fix are mutually exclusive")
 			}
+			// The migration prompt is only relevant for flows that write or
+			// read picker config (--edit and the default review run).
+			// --findings (read-only browsing) and --fix (uses
+			// ReviewFixAgent only) don't interact with the picker, so
+			// prompting in those paths interrupts the user for no reason.
+			if !findings && !fix {
+				if err := maybePromptReviewSettingsMigration(
+					ctx,
+					cmd.OutOrStdout(),
+					cmd.ErrOrStderr(),
+					interactive.IsTerminalWriter(cmd.OutOrStdout()) && interactive.CanPromptInteractively(),
+					deps.PromptYN,
+				); err != nil {
+					return err
+				}
+			}
 			if edit {
 				_, err := RunReviewConfigPicker(ctx, cmd.OutOrStdout(), deps.GetAgentsWithHooksInstalled)
 				return err
@@ -216,7 +233,8 @@ func runReview(ctx context.Context, cmd *cobra.Command, agentOverride, baseOverr
 	if err != nil {
 		cmd.SilenceUsage = true
 		fmt.Fprintf(cmd.ErrOrStderr(), "Failed to load settings: %v\n", err)
-		fmt.Fprintln(cmd.ErrOrStderr(), "Fix `.entire/settings.json` and re-run `entire review`.")
+		fmt.Fprintln(cmd.ErrOrStderr(),
+			"Fix your Entire settings or clone-local review preferences and re-run `entire review`.")
 		return silentErr(err)
 	}
 	if s == nil || len(s.Review) == 0 {