diff --git a/cmd/entire/cli/jsonutil/write.go b/cmd/entire/cli/jsonutil/write.go new file mode 100644 index 0000000000..38f4f02ae0 --- /dev/null +++ b/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 +} diff --git a/cmd/entire/cli/jsonutil/write_test.go b/cmd/entire/cli/jsonutil/write_test.go new file mode 100644 index 0000000000..192f819005 --- /dev/null +++ b/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) + } +} diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index 230c52b6a6..586b6a0cde 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/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 { diff --git a/cmd/entire/cli/review/migration.go b/cmd/entire/cli/review/migration.go new file mode 100644 index 0000000000..ad2fe8a3c2 --- /dev/null +++ b/cmd/entire/cli/review/migration.go @@ -0,0 +1,285 @@ +package review + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "log/slog" + + "github.com/entireio/cli/cmd/entire/cli/logging" + "github.com/entireio/cli/cmd/entire/cli/settings" +) + +type projectReviewSettings struct { + path string + raw map[string]json.RawMessage + review json.RawMessage + fixAgent json.RawMessage + hasReview bool + hasFixAgent bool +} + +func maybePromptReviewSettingsMigration( + ctx context.Context, + out io.Writer, + errOut io.Writer, + canPrompt bool, + promptYN func(context.Context, string, bool) (bool, error), +) error { + project, ok, err := loadProjectReviewSettings(ctx) + if err != nil { + return err + } + if !ok { + return nil + } + + // Skip the prompt entirely if the user has already declined. Without this, + // teams who intentionally commit review prefs would be re-prompted on + // every invocation of `entire review`. + prefs, prefsErr := settings.LoadClonePreferences(ctx) + if prefsErr != nil { + return fmt.Errorf("load review preferences for migration: %w", prefsErr) + } + if prefs != nil && prefs.ReviewMigrationDismissed { + return nil + } + + // Bail before prompting if .entire/settings.local.json already has review + // keys. settings.local.json overrides clone-local preferences (mergeJSON + // wholesale-replaces the review map), so migrating without cleaning the + // local file first would silently nullify the migration on the very next + // settings.Load — the user clicks "yes", their config moves to clone + // prefs, then the local override hides it. Better to surface the + // precondition up front than to leave the user wondering why their + // migrated config disappeared. + // + // Intentionally does NOT set ReviewMigrationDismissed: this is a fixable + // precondition, not a user-rejected migration; the prompt should fire + // again on the next run after the user cleans settings.local.json. + if localHas, localPath, localErr := localSettingsHasReviewKeys(ctx); localErr != nil { + return fmt.Errorf("inspect local settings for migration: %w", localErr) + } else if localHas { + fmt.Fprintln(errOut, "Cannot migrate review preferences: .entire/settings.local.json also has review keys.") + fmt.Fprintf(errOut, "Those override clone-local preferences and would mask the migration. Remove the\n") + fmt.Fprintf(errOut, "`review` / `review_fix_agent` keys from %s, then re-run `entire review`.\n", localPath) + return nil + } + + if !canPrompt { + // Log at Warn so operators tailing .entire/logs/ catch the pending + // migration on scripted/CI invocations where the stderr hint may + // scroll past unnoticed. + logging.Warn(ctx, "review migration pending: project settings has review keys that may be committed", + slog.String("project_settings_path", project.path), + slog.Bool("has_review", project.hasReview), + slog.Bool("has_fix_agent", project.hasFixAgent)) + fmt.Fprintln(errOut, "Review preferences are stored in project settings (.entire/settings.json).") + fmt.Fprintln(errOut, "These are typically committed and may be visible to teammates.") + fmt.Fprintln(errOut, "Run `entire review --edit` interactively to move them to clone-local preferences.") + return nil + } + + if promptYN == nil { + promptYN = realPromptYN + } + migrate, err := promptYN(ctx, "Review preferences are stored in project settings (.entire/settings.json), which is typically committed. Move them to clone-local preferences so they stay private?", false) + if err != nil { + return fmt.Errorf("review settings migration prompt: %w", err) + } + if !migrate { + if prefs == nil { + prefs = &settings.ClonePreferences{} + } + prefs.ReviewMigrationDismissed = true + if err := settings.SaveClonePreferences(ctx, prefs); err != nil { + return fmt.Errorf("save migration dismissal: %w", err) + } + return nil + } + + moved, err := migrateProjectReviewSettings(ctx, project) + if err != nil { + return err + } + if moved { + fmt.Fprintln(out, "Moved review preferences from project settings to clone-local preferences.") + } else { + fmt.Fprintln(out, "Removed unused review keys from project settings; nothing to move.") + } + return nil +} + +func loadProjectReviewSettings(ctx context.Context) (*projectReviewSettings, bool, error) { + path, raw, exists, err := settings.LoadProjectRaw(ctx) + if err != nil { + return nil, false, fmt.Errorf("review migration: %w", err) + } + if !exists { + return nil, false, nil + } + + reviewRaw, hasReview := raw["review"] + fixAgentRaw, hasFixAgent := raw["review_fix_agent"] + if !hasReview && !hasFixAgent { + return nil, false, nil + } + return &projectReviewSettings{ + path: path, + raw: raw, + review: reviewRaw, + fixAgent: fixAgentRaw, + hasReview: hasReview, + hasFixAgent: hasFixAgent, + }, true, nil +} + +// migrateProjectReviewSettings copies review keys from the project settings +// file into clone-local preferences and strips them from the project file. +// +// Returns moved=true when any review data was copied into prefs. When the +// project file's review keys are empty/null (or fully conflict with existing +// prefs, which is rejected upstream), moved=false but the project keys are +// still stripped as cleanup. +// +// Write ordering: prefs are saved first (atomic), then the project file is +// rewritten (atomic). Both writes use temp-then-rename so a crash mid-write +// leaves the original file intact rather than truncated. If the project +// rewrite fails after the prefs write succeeded, prefs precedence covers +// the gap until the next run. +func migrateProjectReviewSettings(ctx context.Context, project *projectReviewSettings) (moved bool, err error) { + if project == nil { + return false, nil + } + + prefs, err := settings.LoadClonePreferences(ctx) + if err != nil { + return false, fmt.Errorf("load review preferences for migration: %w", err) + } + if prefs == nil { + prefs = &settings.ClonePreferences{} + } + + preferencesChanged := false + if project.hasReview && !isJSONNull(project.review) { + var projectReview map[string]settings.ReviewConfig + if err := json.Unmarshal(project.review, &projectReview); err != nil { + return false, fmt.Errorf("parsing project review settings: %w", err) + } + if len(projectReview) > 0 { + merged, mergedOK, conflicts := mergeProjectReviewIntoPrefs(prefs.Review, projectReview) + if len(conflicts) > 0 { + return false, fmt.Errorf( + "review settings exist in both %s and clone-local preferences for agent(s) %v; "+ + "reconcile manually by removing the redundant keys from %s, then re-run `entire review`", + project.path, conflicts, project.path, + ) + } + if mergedOK { + prefs.Review = merged + preferencesChanged = true + } + } + } + if project.hasFixAgent && !isJSONNull(project.fixAgent) { + var fixAgent string + if err := json.Unmarshal(project.fixAgent, &fixAgent); err != nil { + return false, fmt.Errorf("parsing project review_fix_agent: %w", err) + } + if fixAgent != "" { + if prefs.ReviewFixAgent != "" && prefs.ReviewFixAgent != fixAgent { + return false, fmt.Errorf( + "review_fix_agent differs between %s (%q) and clone-local preferences (%q); "+ + "reconcile manually by removing review_fix_agent from %s, then re-run `entire review`", + project.path, fixAgent, prefs.ReviewFixAgent, project.path, + ) + } + if prefs.ReviewFixAgent == "" { + prefs.ReviewFixAgent = fixAgent + preferencesChanged = true + } + } + } + + if preferencesChanged { + if err := settings.SaveClonePreferences(ctx, prefs); err != nil { + return false, fmt.Errorf("save review preferences for migration: %w", err) + } + } + + delete(project.raw, "review") + delete(project.raw, "review_fix_agent") + if err := settings.SaveProjectRaw(project.path, project.raw); err != nil { + return false, fmt.Errorf("save project settings after review migration: %w", err) + } + return preferencesChanged, nil +} + +// mergeProjectReviewIntoPrefs merges projectReview into the current prefs map. +// Per-agent conflicts (same key, different value) are surfaced rather than +// silently resolved — the caller can then refuse the migration with a clear +// message. Non-overlapping entries are merged. Returns ok=false when nothing +// would change (prefs already had every project entry verbatim). +func mergeProjectReviewIntoPrefs(prefs, projectReview map[string]settings.ReviewConfig) (merged map[string]settings.ReviewConfig, ok bool, conflicts []string) { + merged = make(map[string]settings.ReviewConfig, len(prefs)+len(projectReview)) + for k, v := range prefs { + merged[k] = v + } + changed := false + for k, projectV := range projectReview { + if existing, present := merged[k]; present { + if !reviewConfigEqual(existing, projectV) { + conflicts = append(conflicts, k) + } + continue + } + merged[k] = projectV + changed = true + } + if len(conflicts) > 0 { + return nil, false, conflicts + } + return merged, changed, nil +} + +func reviewConfigEqual(a, b settings.ReviewConfig) bool { + if a.Prompt != b.Prompt { + return false + } + if len(a.Skills) != len(b.Skills) { + return false + } + for i := range a.Skills { + if a.Skills[i] != b.Skills[i] { + return false + } + } + return true +} + +func isJSONNull(raw json.RawMessage) bool { + return bytes.Equal(bytes.TrimSpace(raw), []byte("null")) +} + +// localSettingsHasReviewKeys reports whether .entire/settings.local.json +// exists and contains either a "review" or "review_fix_agent" key. Both keys +// override clone-local preferences via mergeJSON's wholesale-replace path, +// so the migration must surface their presence rather than silently produce +// a state where the migrated config never takes effect. +// +// Returns the absolute path of the local settings file too, so callers can +// quote the exact location in the warning they show the user. +func localSettingsHasReviewKeys(ctx context.Context) (has bool, path string, err error) { + path, raw, exists, loadErr := settings.LoadLocalRaw(ctx) + if loadErr != nil { + return false, path, fmt.Errorf("local settings review-keys check: %w", loadErr) + } + if !exists { + return false, path, nil + } + _, hasReview := raw["review"] + _, hasFixAgent := raw["review_fix_agent"] + return hasReview || hasFixAgent, path, nil +} diff --git a/cmd/entire/cli/review/migration_test.go b/cmd/entire/cli/review/migration_test.go new file mode 100644 index 0000000000..97d30eec00 --- /dev/null +++ b/cmd/entire/cli/review/migration_test.go @@ -0,0 +1,421 @@ +package review + +import ( + "bytes" + "context" + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/entireio/cli/cmd/entire/cli/session" + "github.com/entireio/cli/cmd/entire/cli/settings" + "github.com/entireio/cli/cmd/entire/cli/testutil" +) + +func TestReviewSettingsMigration_MovesProjectReviewToClonePreferences(t *testing.T) { + tmp := t.TempDir() + testutil.InitRepo(t, tmp) + t.Chdir(tmp) + session.ClearGitCommonDirCache() + + entireDir := filepath.Join(tmp, ".entire") + if err := os.MkdirAll(entireDir, 0o750); err != nil { + t.Fatalf("mkdir .entire: %v", err) + } + projectSettings := []byte(`{ + "enabled": true, + "log_level": "debug", + "review": {"claude-code": {"skills": ["/review"], "prompt": "project"}}, + "review_fix_agent": "claude-code" + }`) + projectPath := filepath.Join(entireDir, "settings.json") + if err := os.WriteFile(projectPath, projectSettings, 0o600); err != nil { + t.Fatalf("write project settings: %v", err) + } + + prompted := false + promptQuestion := "" + var out bytes.Buffer + if err := maybePromptReviewSettingsMigration(context.Background(), &out, &out, true, func(_ context.Context, question string, _ bool) (bool, error) { + prompted = true + promptQuestion = question + return true, nil + }); err != nil { + t.Fatalf("migration: %v", err) + } + if !prompted { + t.Fatal("expected migration prompt") + } + for _, want := range []string{"project settings", "clone-local preferences", "typically committed"} { + if !strings.Contains(promptQuestion, want) { + t.Fatalf("migration prompt = %q, want it to mention %q", promptQuestion, want) + } + } + + prefs, err := settings.LoadClonePreferences(context.Background()) + if err != nil { + t.Fatalf("load preferences: %v", err) + } + if got := prefs.Review["claude-code"].Prompt; got != "project" { + t.Fatalf("migrated prompt = %q, want project", got) + } + if prefs.ReviewFixAgent != "claude-code" { + t.Fatalf("ReviewFixAgent = %q, want claude-code", prefs.ReviewFixAgent) + } + + raw := map[string]json.RawMessage{} + data, err := os.ReadFile(projectPath) + if err != nil { + t.Fatalf("read project settings: %v", err) + } + if err := json.Unmarshal(data, &raw); err != nil { + t.Fatalf("unmarshal project settings: %v", err) + } + if _, ok := raw["review"]; ok { + t.Fatalf("project review key was not removed: %s", data) + } + if _, ok := raw["review_fix_agent"]; ok { + t.Fatalf("project review_fix_agent key was not removed: %s", data) + } + if _, ok := raw["log_level"]; !ok { + t.Fatalf("unrelated project settings were not preserved: %s", data) + } +} + +// TestReviewSettingsMigration_MergesNonOverlappingPrefs verifies that when the +// project file has review keys for an agent NOT present in clone-local prefs, +// the migration merges them in. Previously the migration silently dropped any +// project config when prefs already had any review entry — that was data loss. +func TestReviewSettingsMigration_MergesNonOverlappingPrefs(t *testing.T) { + tmp := t.TempDir() + testutil.InitRepo(t, tmp) + t.Chdir(tmp) + session.ClearGitCommonDirCache() + + entireDir := filepath.Join(tmp, ".entire") + if err := os.MkdirAll(entireDir, 0o750); err != nil { + t.Fatalf("mkdir .entire: %v", err) + } + projectPath := filepath.Join(entireDir, "settings.json") + projectSettings := []byte(`{ + "enabled": true, + "review": {"project-agent": {"prompt": "project"}} + }`) + if err := os.WriteFile(projectPath, projectSettings, 0o600); err != nil { + t.Fatalf("write project settings: %v", err) + } + if err := settings.SaveClonePreferences(context.Background(), &settings.ClonePreferences{ + Review: map[string]settings.ReviewConfig{ + "local-agent": {Prompt: "local"}, + }, + }); err != nil { + t.Fatalf("seed preferences: %v", err) + } + + var out bytes.Buffer + if err := maybePromptReviewSettingsMigration(context.Background(), &out, &out, true, func(context.Context, string, bool) (bool, error) { + return true, nil + }); err != nil { + t.Fatalf("migration: %v", err) + } + + prefs, err := settings.LoadClonePreferences(context.Background()) + if err != nil { + t.Fatalf("load preferences: %v", err) + } + if got := prefs.Review["local-agent"].Prompt; got != "local" { + t.Fatalf("local prompt = %q, want preserved as %q", got, "local") + } + if got := prefs.Review["project-agent"].Prompt; got != "project" { + t.Fatalf("project prompt = %q, want merged in as %q", got, "project") + } + + data, err := os.ReadFile(projectPath) + if err != nil { + t.Fatalf("read project settings: %v", err) + } + raw := map[string]json.RawMessage{} + if err := json.Unmarshal(data, &raw); err != nil { + t.Fatalf("unmarshal project settings: %v", err) + } + if _, ok := raw["review"]; ok { + t.Fatalf("project review key was not removed: %s", data) + } +} + +// TestReviewSettingsMigration_RefusesConflictingPrefs verifies that when both +// the project file and clone-local prefs have review config for the SAME agent +// with DIFFERENT values, the migration aborts with a clear error rather than +// silently dropping one side. The user must reconcile manually. +func TestReviewSettingsMigration_RefusesConflictingPrefs(t *testing.T) { + tmp := t.TempDir() + testutil.InitRepo(t, tmp) + t.Chdir(tmp) + session.ClearGitCommonDirCache() + + entireDir := filepath.Join(tmp, ".entire") + if err := os.MkdirAll(entireDir, 0o750); err != nil { + t.Fatalf("mkdir .entire: %v", err) + } + projectPath := filepath.Join(entireDir, "settings.json") + projectSettings := []byte(`{ + "enabled": true, + "review": {"claude-code": {"prompt": "project"}} + }`) + if err := os.WriteFile(projectPath, projectSettings, 0o600); err != nil { + t.Fatalf("write project settings: %v", err) + } + if err := settings.SaveClonePreferences(context.Background(), &settings.ClonePreferences{ + Review: map[string]settings.ReviewConfig{ + "claude-code": {Prompt: "local"}, + }, + }); err != nil { + t.Fatalf("seed preferences: %v", err) + } + + var out bytes.Buffer + err := maybePromptReviewSettingsMigration(context.Background(), &out, &out, true, func(context.Context, string, bool) (bool, error) { + return true, nil + }) + if err == nil { + t.Fatal("expected migration to refuse conflicting prefs") + } + if !strings.Contains(err.Error(), "claude-code") { + t.Errorf("error = %q, want it to name the conflicting agent (claude-code)", err.Error()) + } + if !strings.Contains(err.Error(), "reconcile manually") { + t.Errorf("error = %q, want it to guide manual reconciliation", err.Error()) + } + + // Project file must NOT have been rewritten on the conflict path. + data, err := os.ReadFile(projectPath) + if err != nil { + t.Fatalf("read project settings: %v", err) + } + if !bytes.Contains(data, []byte("claude-code")) { + t.Fatalf("project file was modified despite conflict abort: %s", data) + } + + // Clone prefs must be unchanged. + prefs, err := settings.LoadClonePreferences(context.Background()) + if err != nil { + t.Fatalf("load preferences: %v", err) + } + if got := prefs.Review["claude-code"].Prompt; got != "local" { + t.Errorf("local prompt = %q, want unchanged as %q", got, "local") + } +} + +// TestReviewSettingsMigration_NoMoveCleansUpKeys verifies the cleanup-only +// path: project has only `null` values for review keys, so nothing actually +// moves, but the project keys are still stripped and the success message +// reflects that distinction. +func TestReviewSettingsMigration_NoMoveCleansUpKeys(t *testing.T) { + tmp := t.TempDir() + testutil.InitRepo(t, tmp) + t.Chdir(tmp) + session.ClearGitCommonDirCache() + + entireDir := filepath.Join(tmp, ".entire") + if err := os.MkdirAll(entireDir, 0o750); err != nil { + t.Fatalf("mkdir .entire: %v", err) + } + projectPath := filepath.Join(entireDir, "settings.json") + if err := os.WriteFile(projectPath, []byte(`{ + "enabled": true, + "review": null, + "review_fix_agent": null + }`), 0o600); err != nil { + t.Fatalf("write project settings: %v", err) + } + + var out bytes.Buffer + if err := maybePromptReviewSettingsMigration(context.Background(), &out, &out, true, func(context.Context, string, bool) (bool, error) { + return true, nil + }); err != nil { + t.Fatalf("migration: %v", err) + } + if !strings.Contains(out.String(), "Removed unused review keys") { + t.Errorf("output = %q, want the cleanup-only message", out.String()) + } + + data, err := os.ReadFile(projectPath) + if err != nil { + t.Fatalf("read project settings: %v", err) + } + raw := map[string]json.RawMessage{} + if err := json.Unmarshal(data, &raw); err != nil { + t.Fatalf("unmarshal project settings: %v", err) + } + if _, ok := raw["review"]; ok { + t.Fatalf("project review key was not removed: %s", data) + } +} + +// TestReviewSettingsMigration_DeclinePersistsDismissal verifies that declining +// the prompt records ReviewMigrationDismissed in clone-local prefs, and that a +// subsequent invocation does NOT re-prompt. Without this, teams who +// intentionally commit review prefs would be re-prompted on every command. +func TestReviewSettingsMigration_DeclinePersistsDismissal(t *testing.T) { + tmp := t.TempDir() + testutil.InitRepo(t, tmp) + t.Chdir(tmp) + session.ClearGitCommonDirCache() + + entireDir := filepath.Join(tmp, ".entire") + if err := os.MkdirAll(entireDir, 0o750); err != nil { + t.Fatalf("mkdir .entire: %v", err) + } + projectPath := filepath.Join(entireDir, "settings.json") + projectSettings := []byte(`{ + "enabled": true, + "review": {"claude-code": {"prompt": "project"}} + }`) + if err := os.WriteFile(projectPath, projectSettings, 0o600); err != nil { + t.Fatalf("write project settings: %v", err) + } + + // First invocation: user declines. + var out bytes.Buffer + promptCount := 0 + declineThenFail := func(context.Context, string, bool) (bool, error) { + promptCount++ + return false, nil + } + if err := maybePromptReviewSettingsMigration(context.Background(), &out, &out, true, declineThenFail); err != nil { + t.Fatalf("first invocation: %v", err) + } + if promptCount != 1 { + t.Errorf("first invocation prompted %d times, want 1", promptCount) + } + + // Dismissal must be persisted. + prefs, err := settings.LoadClonePreferences(context.Background()) + if err != nil { + t.Fatalf("load preferences: %v", err) + } + if prefs == nil || !prefs.ReviewMigrationDismissed { + t.Fatalf("ReviewMigrationDismissed = false, want true after decline (prefs = %+v)", prefs) + } + + // Project file must be untouched on decline. + data, err := os.ReadFile(projectPath) + if err != nil { + t.Fatalf("read project settings: %v", err) + } + if !bytes.Contains(data, []byte("claude-code")) { + t.Errorf("project file was modified on decline: %s", data) + } + + // Second invocation: must NOT re-prompt. + failIfPrompted := func(context.Context, string, bool) (bool, error) { + t.Fatal("prompt should not be called when dismissal is persisted") + return false, nil + } + if err := maybePromptReviewSettingsMigration(context.Background(), &out, &out, true, failIfPrompted); err != nil { + t.Fatalf("second invocation: %v", err) + } +} + +func TestReviewSettingsMigration_SkipsWhenProjectHasNoReviewKeys(t *testing.T) { + tmp := t.TempDir() + testutil.InitRepo(t, tmp) + t.Chdir(tmp) + session.ClearGitCommonDirCache() + + entireDir := filepath.Join(tmp, ".entire") + if err := os.MkdirAll(entireDir, 0o750); err != nil { + t.Fatalf("mkdir .entire: %v", err) + } + projectPath := filepath.Join(entireDir, "settings.json") + if err := os.WriteFile(projectPath, []byte(`{"enabled":true,"log_level":"debug"}`), 0o600); err != nil { + t.Fatalf("write project settings: %v", err) + } + + var out bytes.Buffer + if err := maybePromptReviewSettingsMigration(context.Background(), &out, &out, true, func(context.Context, string, bool) (bool, error) { + t.Fatal("prompt should not be called") + return false, nil + }); err != nil { + t.Fatalf("migration: %v", err) + } + + preferencesPath, err := settings.ClonePreferencesPath(context.Background()) + if err != nil { + t.Fatalf("preferences path: %v", err) + } + if _, err := os.Stat(preferencesPath); !os.IsNotExist(err) { + t.Fatalf("preferences file exists after no-op migration: %v", err) + } +} + +// TestReviewSettingsMigration_BailsOnLocalSettingsReviewKeys pins the +// precondition: when .entire/settings.local.json has review keys, those +// override clone-local preferences via mergeJSON's wholesale-replace path, +// so the migration must surface the conflict up front rather than silently +// produce a migrated-but-masked state. Bailing also intentionally does NOT +// set ReviewMigrationDismissed — this is a fixable precondition, not a +// rejected migration, and the user should be re-prompted after cleaning +// settings.local.json. +func TestReviewSettingsMigration_BailsOnLocalSettingsReviewKeys(t *testing.T) { + tmp := t.TempDir() + testutil.InitRepo(t, tmp) + t.Chdir(tmp) + session.ClearGitCommonDirCache() + + entireDir := filepath.Join(tmp, ".entire") + if err := os.MkdirAll(entireDir, 0o750); err != nil { + t.Fatalf("mkdir .entire: %v", err) + } + projectPath := filepath.Join(entireDir, "settings.json") + projectSettings := []byte(`{ + "enabled": true, + "review": {"claude-code": {"prompt": "project"}} + }`) + if err := os.WriteFile(projectPath, projectSettings, 0o600); err != nil { + t.Fatalf("write project settings: %v", err) + } + localPath := filepath.Join(entireDir, "settings.local.json") + localSettings := []byte(`{"review": {"local-agent": {"prompt": "local"}}}`) + if err := os.WriteFile(localPath, localSettings, 0o600); err != nil { + t.Fatalf("write local settings: %v", err) + } + + var out, errOut bytes.Buffer + if err := maybePromptReviewSettingsMigration(context.Background(), &out, &errOut, true, func(context.Context, string, bool) (bool, error) { + t.Fatal("prompt should not be called when settings.local.json has review keys") + return false, nil + }); err != nil { + t.Fatalf("migration: %v", err) + } + + stderr := errOut.String() + for _, want := range []string{"settings.local.json", "review", "Remove"} { + if !strings.Contains(stderr, want) { + t.Errorf("stderr = %q, want it to mention %q", stderr, want) + } + } + + // Project file must NOT have been rewritten — the bail path leaves + // everything in place so the user can clean settings.local.json and + // re-run. + got, err := os.ReadFile(projectPath) + if err != nil { + t.Fatalf("read project settings: %v", err) + } + if !bytes.Contains(got, []byte(`"claude-code"`)) { + t.Fatalf("project file was modified despite bail; got: %s", got) + } + + // Dismissal must NOT be persisted — the user didn't choose to dismiss, + // they hit a fixable precondition. Next run should re-prompt. + prefs, err := settings.LoadClonePreferences(context.Background()) + if err != nil { + t.Fatalf("load preferences: %v", err) + } + if prefs != nil && prefs.ReviewMigrationDismissed { + t.Fatalf("ReviewMigrationDismissed = true after bail; should not persist a fixable precondition as dismissal") + } +} diff --git a/cmd/entire/cli/review/picker.go b/cmd/entire/cli/review/picker.go index 723e243e22..d6f39e5d0d 100644 --- a/cmd/entire/cli/review/picker.go +++ b/cmd/entire/cli/review/picker.go @@ -2,7 +2,7 @@ // // picker.go implements the interactive review skills picker and agent selection // helpers. pickConfig presents a huh multi-select per installed agent and saves -// the selection to .entire/settings.json. +// the selection to clone-local review preferences. package review import ( @@ -57,7 +57,7 @@ func ConfirmFirstRunSetup(ctx context.Context, out io.Writer) bool { fmt.Fprintln(out, "No review config found — let's set one up first.") fmt.Fprintln(out) fmt.Fprintln(out, "You'll pick skills for each installed agent. They're saved to") - fmt.Fprintln(out, ".entire/settings.json; edit later with `entire review --edit`.") + fmt.Fprintln(out, "local review preferences; edit later with `entire review --edit`.") fmt.Fprintln(out, "After setup, the review will run with your selection.") fmt.Fprintln(out) @@ -81,7 +81,7 @@ func ConfirmFirstRunSetup(ctx context.Context, out io.Writer) bool { // RunReviewConfigPicker presents a huh multi-select for each installed agent // that has curated review skills, and saves the selection to -// .entire/settings.json. Previously-saved skills are pre-checked via +// clone-local review preferences. Previously-saved skills are pre-checked via // huh.Option.Selected(true), mirroring how `entire enable` preserves prior // selections in its own agent picker. // @@ -97,7 +97,7 @@ func RunReviewConfigPicker(ctx context.Context, out io.Writer, getInstalled func } // Narrow to agents that have a curated skills list; others need manual - // editing of settings.json under review.. + // editing of clone-local preferences under review.. type configurableAgent struct { name types.AgentName ag agent.Agent @@ -114,9 +114,19 @@ func RunReviewConfigPicker(ctx context.Context, out io.Writer, getInstalled func configurable = append(configurable, configurableAgent{name: name, ag: ag}) } if len(configurable) == 0 { - return nil, errors.New( - "no installed agents have curated review skills; " + - "edit .entire/settings.json directly under review.", + prefsPath, pathErr := settings.ClonePreferencesPath(ctx) + if pathErr != nil { + return nil, errors.New( + "no installed agents have curated review skills; " + + "install an eligible agent and run `entire review --edit`, " + + "or edit clone-local review preferences under review.", + ) + } + return nil, fmt.Errorf( + "no installed agents have curated review skills; "+ + "install an eligible agent and run `entire review --edit`, "+ + "or edit clone-local review preferences (%s) under review.", + prefsPath, ) } @@ -224,7 +234,7 @@ func RunReviewConfigPicker(ctx context.Context, out io.Writer, getInstalled func if err := saveReviewConfigAndFixAgent(ctx, merged, fixAgent); err != nil { return nil, err } - fmt.Fprintln(out, "Saved review config to .entire/settings.json. Edit directly or run `entire review --edit`.") + fmt.Fprintln(out, "Saved review config to local review preferences. Edit later with `entire review --edit`.") return merged, nil } @@ -249,53 +259,53 @@ func MergePickerResults(existing map[string]settings.ReviewConfig, offered map[s return merged } -// SaveReviewConfig persists the review map into .entire/settings.json while -// preserving all other settings. A Load error means the file exists but is -// malformed — we must NOT silently overwrite it with an empty struct, or -// every unrelated setting the user had configured would be wiped. Return the +// SaveReviewConfig persists the review map into clone-local preferences while +// preserving other review preferences. A load error means the preferences file +// exists but is malformed — we must NOT silently overwrite it with an empty +// struct, or every unrelated review preference would be wiped. Return the // error so the caller can surface it instead. func SaveReviewConfig(ctx context.Context, review map[string]settings.ReviewConfig) error { - s, err := settings.Load(ctx) + prefs, err := settings.LoadClonePreferences(ctx) if err != nil { - return fmt.Errorf("load settings before save: %w", err) + return fmt.Errorf("load review preferences before save: %w", err) } - if s == nil { - s = &settings.EntireSettings{} + if prefs == nil { + prefs = &settings.ClonePreferences{} } - s.Review = review - if err := settings.Save(ctx, s); err != nil { - return fmt.Errorf("save settings: %w", err) + prefs.Review = review + if err := settings.SaveClonePreferences(ctx, prefs); err != nil { + return fmt.Errorf("save review preferences: %w", err) } return nil } func SaveReviewFixAgent(ctx context.Context, agentName string) error { - s, err := settings.Load(ctx) + prefs, err := settings.LoadClonePreferences(ctx) if err != nil { - return fmt.Errorf("load settings before save: %w", err) + return fmt.Errorf("load review preferences before save: %w", err) } - if s == nil { - s = &settings.EntireSettings{} + if prefs == nil { + prefs = &settings.ClonePreferences{} } - s.ReviewFixAgent = agentName - if err := settings.Save(ctx, s); err != nil { - return fmt.Errorf("save settings: %w", err) + prefs.ReviewFixAgent = agentName + if err := settings.SaveClonePreferences(ctx, prefs); err != nil { + return fmt.Errorf("save review preferences: %w", err) } return nil } func saveReviewConfigAndFixAgent(ctx context.Context, review map[string]settings.ReviewConfig, fixAgent string) error { - s, err := settings.Load(ctx) + prefs, err := settings.LoadClonePreferences(ctx) if err != nil { - return fmt.Errorf("load settings before save: %w", err) + return fmt.Errorf("load review preferences before save: %w", err) } - if s == nil { - s = &settings.EntireSettings{} + if prefs == nil { + prefs = &settings.ClonePreferences{} } - s.Review = review - s.ReviewFixAgent = fixAgent - if err := settings.Save(ctx, s); err != nil { - return fmt.Errorf("save settings: %w", err) + prefs.Review = review + prefs.ReviewFixAgent = fixAgent + if err := settings.SaveClonePreferences(ctx, prefs); err != nil { + return fmt.Errorf("save review preferences: %w", err) } return nil } diff --git a/cmd/entire/cli/review/picker_test.go b/cmd/entire/cli/review/picker_test.go index b42c435b7f..20f1a5a851 100644 --- a/cmd/entire/cli/review/picker_test.go +++ b/cmd/entire/cli/review/picker_test.go @@ -289,8 +289,8 @@ func TestBuildReviewPickerFields_SingleBuiltinDefaultsSelectedAndRenders(t *test } } -// TestSaveReviewConfig_PersistsSettings verifies SaveReviewConfig writes and -// the settings can be read back. +// TestSaveReviewConfig_PersistsSettings verifies SaveReviewConfig writes +// clone-local preferences and the settings can be read back. func TestSaveReviewConfig_PersistsSettings(t *testing.T) { tmp := t.TempDir() testutil.InitRepo(t, tmp) @@ -303,13 +303,13 @@ func TestSaveReviewConfig_PersistsSettings(t *testing.T) { t.Fatal(err) } - s, err := settings.Load(context.Background()) + prefs, err := settings.LoadClonePreferences(context.Background()) if err != nil { - t.Fatalf("load settings: %v", err) + t.Fatalf("load preferences: %v", err) } - cfg := s.Review[testAgentName] + cfg := prefs.Review[testAgentName] if len(cfg.Skills) != 2 { - t.Errorf("expected 2 skills saved, got %v", cfg.Skills) + t.Fatalf("expected 2 skills saved, got %v", cfg.Skills) } if cfg.Skills[0] != testReviewSkill { t.Errorf("first skill = %q", cfg.Skills[0]) @@ -321,13 +321,10 @@ func TestSaveReviewConfig_PreservesReviewFixAgent(t *testing.T) { testutil.InitRepo(t, tmp) t.Chdir(tmp) - entireDir := filepath.Join(tmp, ".entire") - if err := os.MkdirAll(entireDir, 0o750); err != nil { - t.Fatal(err) - } - before := []byte(`{"enabled":true,"review_fix_agent":"` + testCodexAgent + `"}`) - if err := os.WriteFile(filepath.Join(entireDir, "settings.json"), before, 0o600); err != nil { - t.Fatal(err) + if err := settings.SaveClonePreferences(context.Background(), &settings.ClonePreferences{ + ReviewFixAgent: testCodexAgent, + }); err != nil { + t.Fatalf("seed preferences: %v", err) } err := review.SaveReviewConfig(context.Background(), map[string]settings.ReviewConfig{ @@ -337,12 +334,15 @@ func TestSaveReviewConfig_PreservesReviewFixAgent(t *testing.T) { t.Fatal(err) } - s, err := settings.Load(context.Background()) + prefs, err := settings.LoadClonePreferences(context.Background()) if err != nil { - t.Fatalf("load settings: %v", err) + t.Fatalf("load preferences: %v", err) } - if s.ReviewFixAgent != testCodexAgent { - t.Fatalf("ReviewFixAgent = %q, want %s", s.ReviewFixAgent, testCodexAgent) + if prefs.ReviewFixAgent != testCodexAgent { + t.Fatalf("ReviewFixAgent = %q, want %s", prefs.ReviewFixAgent, testCodexAgent) + } + if _, ok := prefs.Review[testAgentName]; !ok { + t.Fatalf("review preferences missing %s: %+v", testAgentName, prefs.Review) } } @@ -355,31 +355,34 @@ func TestSaveReviewFixAgent_PersistsSettings(t *testing.T) { t.Fatal(err) } - s, err := settings.Load(context.Background()) + prefs, err := settings.LoadClonePreferences(context.Background()) if err != nil { - t.Fatalf("load settings: %v", err) + t.Fatalf("load preferences: %v", err) } - if s.ReviewFixAgent != testCodexAgent { - t.Fatalf("ReviewFixAgent = %q, want %s", s.ReviewFixAgent, testCodexAgent) + if prefs.ReviewFixAgent != testCodexAgent { + t.Fatalf("ReviewFixAgent = %q, want %s", prefs.ReviewFixAgent, testCodexAgent) } } // TestSaveReviewConfig_ReturnsErrorOnMalformedSettings ensures SaveReviewConfig -// does not overwrite existing settings when settings.json is malformed. +// does not overwrite existing preferences when preferences.json is malformed. func TestSaveReviewConfig_ReturnsErrorOnMalformedSettings(t *testing.T) { tmp := t.TempDir() testutil.InitRepo(t, tmp) t.Chdir(tmp) - entireDir := filepath.Join(tmp, ".entire") - if err := os.MkdirAll(entireDir, 0o750); err != nil { + preferencesPath, err := settings.ClonePreferencesPath(context.Background()) + if err != nil { + t.Fatalf("preferences path: %v", err) + } + if err := os.MkdirAll(filepath.Dir(preferencesPath), 0o750); err != nil { t.Fatal(err) } - malformed := []byte(`{"enabled": true, "strategy": "manual-commit", "review": {`) - if err := os.WriteFile(filepath.Join(entireDir, "settings.json"), malformed, 0o600); err != nil { + malformed := []byte(`{"review": {`) + if err := os.WriteFile(preferencesPath, malformed, 0o600); err != nil { t.Fatal(err) } - before, err := os.ReadFile(filepath.Join(entireDir, "settings.json")) + before, err := os.ReadFile(preferencesPath) if err != nil { t.Fatal(err) } @@ -388,14 +391,14 @@ func TestSaveReviewConfig_ReturnsErrorOnMalformedSettings(t *testing.T) { testAgentName: {Skills: []string{testReviewSkill}}, }) if err == nil { - t.Fatal("expected SaveReviewConfig to error on malformed settings") + t.Fatal("expected SaveReviewConfig to error on malformed preferences") } - after, err := os.ReadFile(filepath.Join(entireDir, "settings.json")) + after, err := os.ReadFile(preferencesPath) if err != nil { t.Fatal(err) } if string(before) != string(after) { - t.Errorf("settings.json was overwritten on load error:\nbefore=%q\nafter=%q", before, after) + t.Errorf("preferences.json was overwritten on load error:\nbefore=%q\nafter=%q", before, after) } } diff --git a/cmd/entire/cli/settings/settings.go b/cmd/entire/cli/settings/settings.go index f30062f00b..d5637ae2d9 100644 --- a/cmd/entire/cli/settings/settings.go +++ b/cmd/entire/cli/settings/settings.go @@ -8,6 +8,7 @@ import ( "context" "encoding/json" "fmt" + "log/slog" "os" "path/filepath" "strconv" @@ -16,7 +17,9 @@ import ( "time" "github.com/entireio/cli/cmd/entire/cli/jsonutil" + "github.com/entireio/cli/cmd/entire/cli/logging" "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/session" ) const ( @@ -24,6 +27,8 @@ const ( EntireSettingsFile = ".entire/settings.json" // EntireSettingsLocalFile is the path to the local settings override file (not committed) EntireSettingsLocalFile = ".entire/settings.local.json" + // ClonePreferencesFile is the path inside the git common dir for clone-local preferences. + ClonePreferencesFile = "entire/preferences.json" // defaultGenerationRetentionDays is the default retention window for archived // checkpoints v2 raw-transcript generations when no override is configured. defaultGenerationRetentionDays = 14 @@ -111,6 +116,23 @@ type EntireSettings struct { Strategy string `json:"strategy,omitempty"` } +// ClonePreferences stores clone-local, uncommitted preferences that should be +// shared by linked worktrees in the same git clone. +// +// Stored in the git common dir (not the worktree) so multiple worktrees of the +// same clone see the same preferences. Not committed because the file lives +// inside .git/. +type ClonePreferences struct { + Review map[string]ReviewConfig `json:"review,omitempty"` + ReviewFixAgent string `json:"review_fix_agent,omitempty"` + + // ReviewMigrationDismissed records that the user declined the one-shot + // migration of review keys from project settings to clone-local prefs. + // Once true, `entire review` stops prompting on every invocation; the + // user can re-enable by editing this file or deleting the key. + ReviewMigrationDismissed bool `json:"review_migration_dismissed,omitempty"` +} + // SummaryGenerationSettings configures provider selection for on-demand // checkpoint summaries generated by explain --generate. type SummaryGenerationSettings struct { @@ -239,9 +261,10 @@ func (s *EntireSettings) ReviewConfigFor(agentName string) ReviewConfig { return s.Review[agentName] } -// Load loads the Entire settings from .entire/settings.json, -// then applies any overrides from .entire/settings.local.json if it exists. -// Returns default settings if neither file exists. +// Load loads the Entire settings from .entire/settings.json, then applies +// clone-local preferences from the git common dir, then applies any overrides +// from .entire/settings.local.json if it exists. +// Returns default settings if no settings or preferences file exists. // Works correctly from any subdirectory within the repository. func Load(ctx context.Context) (*EntireSettings, error) { // Get absolute paths for settings files @@ -249,21 +272,41 @@ func Load(ctx context.Context) (*EntireSettings, error) { if err != nil { settingsFileAbs = EntireSettingsFile // Fallback to relative } + preferencesFileAbs := "" + if path, prefErr := ClonePreferencesPath(ctx); prefErr == nil { + preferencesFileAbs = path + } else { + // Log at Debug rather than silently dropping the preferences layer. + // "Not in a git repo" is a legitimate case (some commands run outside + // a repo), but a git PATH issue or .git/ permission failure is worth + // finding via `ENTIRE_LOG_LEVEL=debug` when users report "my picker + // choices vanished". + logging.Debug(ctx, "clone preferences path unresolved; skipping preferences layer", + slog.String("error", prefErr.Error())) + } localSettingsFileAbs, err := paths.AbsPath(ctx, EntireSettingsLocalFile) if err != nil { localSettingsFileAbs = EntireSettingsLocalFile // Fallback to relative } - return loadMergedSettings(settingsFileAbs, localSettingsFileAbs) + return loadMergedSettings(settingsFileAbs, preferencesFileAbs, localSettingsFileAbs) } -func loadMergedSettings(settingsFileAbs, localSettingsFileAbs string) (*EntireSettings, error) { +func loadMergedSettings(settingsFileAbs, preferencesFileAbs, localSettingsFileAbs string) (*EntireSettings, error) { // Load base settings settings, err := loadFromFile(settingsFileAbs) if err != nil { return nil, fmt.Errorf("reading settings file: %w", err) } + if preferencesFileAbs != "" { + preferences, err := loadClonePreferencesFromFile(preferencesFileAbs) + if err != nil { + return nil, fmt.Errorf("reading clone preferences file: %w", err) + } + applyClonePreferences(settings, preferences) + } + // Apply local overrides if they exist localData, err := os.ReadFile(localSettingsFileAbs) //nolint:gosec // path is from AbsPath or constant if err != nil { @@ -295,6 +338,107 @@ func LoadFromFile(filePath string) (*EntireSettings, error) { return loadFromFile(filePath) } +// LoadProjectRaw reads .entire/settings.json as a generic JSON object so +// callers can inspect or mutate individual keys without losing unrelated +// fields to round-trip decoding. +// +// Returns: +// - path: absolute path of the project settings file. +// - raw: parsed JSON object, or an empty map when the file is missing. +// - exists: false when the file does not exist (raw is empty); true otherwise. +// - err: parse error or read error other than ENOENT. +// +// Pair with SaveProjectRaw for read-modify-write flows like the review-key +// migration. Owning the path resolution and raw IO here keeps callers from +// duplicating settings parsing in violation of the "Settings access must go +// through the settings package" rule in CLAUDE.md. +func LoadProjectRaw(ctx context.Context) (path string, raw map[string]json.RawMessage, exists bool, err error) { + path, err = paths.AbsPath(ctx, EntireSettingsFile) + if err != nil { + path = EntireSettingsFile + } + data, readErr := os.ReadFile(path) //nolint:gosec // path is from AbsPath or a project-relative constant + if readErr != nil { + if os.IsNotExist(readErr) { + return path, map[string]json.RawMessage{}, false, nil + } + return path, nil, false, fmt.Errorf("reading project settings: %w", readErr) + } + raw = map[string]json.RawMessage{} + if err := json.Unmarshal(data, &raw); err != nil { + return path, nil, true, fmt.Errorf("parsing project settings: %w", err) + } + return path, raw, true, nil +} + +// LoadLocalRaw reads .entire/settings.local.json as a generic JSON object, +// mirroring LoadProjectRaw for the per-developer overrides file. Returns +// exists=false (and an empty raw map) when the file does not exist — the +// common case for users who haven't created the local override file. +// +// Pair with the migration flow: callers can use this to detect when local +// overrides would mask a freshly-migrated setting, then warn the user +// before performing the migration. +func LoadLocalRaw(ctx context.Context) (path string, raw map[string]json.RawMessage, exists bool, err error) { + path, err = paths.AbsPath(ctx, EntireSettingsLocalFile) + if err != nil { + path = EntireSettingsLocalFile + } + data, readErr := os.ReadFile(path) //nolint:gosec // path is from AbsPath or a project-relative constant + if readErr != nil { + if os.IsNotExist(readErr) { + return path, map[string]json.RawMessage{}, false, nil + } + return path, nil, false, fmt.Errorf("reading local settings: %w", readErr) + } + raw = map[string]json.RawMessage{} + if err := json.Unmarshal(data, &raw); err != nil { + return path, nil, true, fmt.Errorf("parsing local settings: %w", err) + } + return path, raw, true, nil +} + +// SaveProjectRaw writes a generic JSON object back to .entire/settings.json +// atomically (temp file + rename). Callers should mutate the map returned by +// LoadProjectRaw and pass it back here so unrelated fields are preserved. +func SaveProjectRaw(path string, raw map[string]json.RawMessage) error { + data, err := jsonutil.MarshalIndentWithNewline(raw, "", " ") + if err != nil { + return fmt.Errorf("marshal project settings: %w", err) + } + if err := jsonutil.WriteFileAtomic(path, data, 0o644); err != nil { + return fmt.Errorf("writing project settings: %w", err) + } + return nil +} + +// ClonePreferencesPath returns the clone-local preferences path in the git common dir. +func ClonePreferencesPath(ctx context.Context) (string, error) { + commonDir, err := session.GetGitCommonDir(ctx) + if err != nil { + return "", fmt.Errorf("resolve git common dir: %w", err) + } + return filepath.Join(commonDir, ClonePreferencesFile), nil +} + +// LoadClonePreferences loads clone-local preferences from the git common dir. +func LoadClonePreferences(ctx context.Context) (*ClonePreferences, error) { + path, err := ClonePreferencesPath(ctx) + if err != nil { + return nil, err + } + return loadClonePreferencesFromFile(path) +} + +// SaveClonePreferences saves clone-local preferences to the git common dir. +func SaveClonePreferences(ctx context.Context, prefs *ClonePreferences) error { + path, err := ClonePreferencesPath(ctx) + if err != nil { + return err + } + return saveClonePreferencesToFile(prefs, path) +} + // LoadFromBytes parses settings from raw JSON bytes without merging local overrides. // Use this when you have settings content from a non-file source (e.g., git show). func LoadFromBytes(data []byte) (*EntireSettings, error) { @@ -340,8 +484,70 @@ func loadFromFile(filePath string) (*EntireSettings, error) { return settings, nil } +func loadClonePreferencesFromFile(filePath string) (*ClonePreferences, error) { + prefs := &ClonePreferences{} + + data, err := os.ReadFile(filePath) //nolint:gosec // path is from caller + if err != nil { + if os.IsNotExist(err) { + return prefs, nil + } + return nil, fmt.Errorf("%w", err) + } + + // Lenient decoding here (vs. strict via DisallowUnknownFields in + // loadFromFile for EntireSettings). Two reasons clone preferences need + // the looser contract: + // 1. They are rewritten on every picker save — a newer binary can + // introduce a field the older binary then sees as unknown, which + // under strict decoding would brick settings.Load for that older + // binary across the whole clone. + // 2. The file lives in .git/, so users rarely hand-edit it; the + // typo-silently-ignored downside is theoretical here. + // EntireSettings stays strict because it's committed and team-edited, + // where unknown keys usually mean typos worth surfacing immediately. + if err := json.Unmarshal(data, prefs); err != nil { + return nil, fmt.Errorf("parsing preferences file: %w", err) + } + return prefs, nil +} + +func saveClonePreferencesToFile(prefs *ClonePreferences, filePath string) error { + if prefs == nil { + prefs = &ClonePreferences{} + } + dir := filepath.Dir(filePath) + if err := os.MkdirAll(dir, 0o750); err != nil { + return fmt.Errorf("creating preferences directory: %w", err) + } + + data, err := jsonutil.MarshalIndentWithNewline(prefs, "", " ") + if err != nil { + return fmt.Errorf("marshaling preferences: %w", err) + } + + if err := jsonutil.WriteFileAtomic(filePath, data, 0o644); err != nil { + return fmt.Errorf("writing preferences file: %w", err) + } + return nil +} + +func applyClonePreferences(settings *EntireSettings, prefs *ClonePreferences) { + if prefs == nil { + return + } + if prefs.Review != nil { + settings.Review = prefs.Review + } + if prefs.ReviewFixAgent != "" { + settings.ReviewFixAgent = prefs.ReviewFixAgent + } +} + // mergeJSON merges JSON data into existing settings. -// Only non-zero values from the JSON override existing settings. +// Most fields only apply non-zero values from JSON. The review map is replaced +// whenever the key is present, so override files can clear or fully replace +// project-level review configuration. func mergeJSON(settings *EntireSettings, data []byte) error { // Validate that there are no unknown keys using strict decoding. dec := json.NewDecoder(bytes.NewReader(data)) @@ -369,6 +575,13 @@ func mergeJSON(settings *EntireSettings, data []byte) error { if err := mergeCommitLinking(settings, raw); err != nil { return err } + if reviewRaw, ok := raw["review"]; ok { + var review map[string]ReviewConfig + if err := json.Unmarshal(reviewRaw, &review); err != nil { + return fmt.Errorf("parsing review field: %w", err) + } + settings.Review = review + } // Merge redaction sub-fields if present (field-level, not wholesale replace). if redactionRaw, ok := raw["redaction"]; ok { @@ -973,8 +1186,7 @@ func saveToFile(ctx context.Context, settings *EntireSettings, filePath string) return fmt.Errorf("marshaling settings: %w", err) } - //nolint:gosec // G306: settings file is config, not secrets; 0o644 is appropriate - if err := os.WriteFile(filePathAbs, data, 0o644); err != nil { + if err := jsonutil.WriteFileAtomic(filePathAbs, data, 0o644); err != nil { return fmt.Errorf("writing settings file: %w", err) } return nil diff --git a/cmd/entire/cli/settings/settings_test.go b/cmd/entire/cli/settings/settings_test.go index e1663421d5..27e7b22b90 100644 --- a/cmd/entire/cli/settings/settings_test.go +++ b/cmd/entire/cli/settings/settings_test.go @@ -8,6 +8,9 @@ import ( "strings" "testing" "time" + + "github.com/entireio/cli/cmd/entire/cli/session" + "github.com/entireio/cli/cmd/entire/cli/testutil" ) const ( @@ -1002,7 +1005,9 @@ func TestLoadMerged_CustomRedactionsPerKeyOverride(t *testing.T) { t.Fatal(err) } - merged, err := loadMergedSettings(base, local) + // preferencesFileAbs="" skips the clone-preferences layer; this test only + // exercises the project + local merge. + merged, err := loadMergedSettings(base, "", local) if err != nil { t.Fatalf("loadMergedSettings: %v", err) } @@ -1084,6 +1089,81 @@ func TestEntireSettings_ReviewRoundTrip(t *testing.T) { } } +func TestMergeJSON_ReviewWholesaleReplacesBase(t *testing.T) { + t.Parallel() + s := &EntireSettings{Review: map[string]ReviewConfig{ + "claude-code": {Skills: []string{"/old"}}, + }} + raw := []byte(`{"review":{"codex":{"prompt":"new"}}}`) + + if err := mergeJSON(s, raw); err != nil { + t.Fatalf("mergeJSON: %v", err) + } + if _, ok := s.Review["claude-code"]; ok { + t.Fatalf("base review entry survived wholesale replace: %+v", s.Review) + } + if got := s.Review["codex"].Prompt; got != "new" { + t.Fatalf("codex prompt = %q, want new", got) + } +} + +func TestLoad_AppliesClonePreferencesBeforeLocalSettings(t *testing.T) { + tmp := t.TempDir() + testutil.InitRepo(t, tmp) + t.Chdir(tmp) + session.ClearGitCommonDirCache() + + entireDir := filepath.Join(tmp, ".entire") + if err := os.MkdirAll(entireDir, 0o750); err != nil { + t.Fatalf("mkdir .entire: %v", err) + } + projectSettings := []byte(`{ + "enabled": true, + "review": {"project-agent": {"prompt": "project"}}, + "review_fix_agent": "project-agent" + }`) + if err := os.WriteFile(filepath.Join(entireDir, "settings.json"), projectSettings, 0o600); err != nil { + t.Fatalf("write project settings: %v", err) + } + + preferencesDir := filepath.Join(tmp, ".git", "entire") + if err := os.MkdirAll(preferencesDir, 0o750); err != nil { + t.Fatalf("mkdir preferences dir: %v", err) + } + preferences := []byte(`{ + "review": {"clone-agent": {"prompt": "clone"}}, + "review_fix_agent": "clone-agent" + }`) + if err := os.WriteFile(filepath.Join(preferencesDir, "preferences.json"), preferences, 0o600); err != nil { + t.Fatalf("write preferences: %v", err) + } + + localSettings := []byte(`{ + "review": {"local-agent": {"prompt": "local"}}, + "review_fix_agent": "local-agent" + }`) + if err := os.WriteFile(filepath.Join(entireDir, "settings.local.json"), localSettings, 0o600); err != nil { + t.Fatalf("write local settings: %v", err) + } + + s, err := Load(context.Background()) + if err != nil { + t.Fatalf("Load: %v", err) + } + if _, ok := s.Review["project-agent"]; ok { + t.Fatalf("project review survived overrides: %+v", s.Review) + } + if _, ok := s.Review["clone-agent"]; ok { + t.Fatalf("clone review survived local override: %+v", s.Review) + } + if got := s.Review["local-agent"].Prompt; got != "local" { + t.Fatalf("local-agent prompt = %q, want local", got) + } + if s.ReviewFixAgent != "local-agent" { + t.Fatalf("ReviewFixAgent = %q, want local-agent", s.ReviewFixAgent) + } +} + func TestEntireSettings_ReviewConfigFor(t *testing.T) { t.Parallel() s := &EntireSettings{Review: map[string]ReviewConfig{