From 98b4106c3f1d9199ed56354491f55d9991367822 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Fri, 8 May 2026 17:35:12 -0400 Subject: [PATCH 01/11] fix(recap): surface real auth/network errors instead of "sign in" hint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The auth pre-check in runRecap collapsed three distinct failure modes (missing token, keyring read error, insecure base URL) into a single "Sign in with `entire login`" message, with the real error swallowed by NewSilentError. This made flags appear broken on first run because the function exited before any line that consumed --week, --agent, etc. Replace the gating call with a newRecapClient helper that builds the client even with an empty token, so FetchMeRecap always runs and surfaces the actual cause via the existing recapLoadErrorMessage mapping (401 → "Run `entire login` to re-authenticate.", 4xx/5xx → specific guidance). The insecure-URL check stays but only when a token exists, with a friendlier message that names the offending env var and the fix. Also split DNS NXDOMAIN out of the generic network-error path so a misconfigured ENTIRE_API_BASE_URL says "Could not resolve API host..." instead of blaming the user's internet connection. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: d534f23017c5 --- cmd/entire/cli/recap.go | 23 ++++++++++++++++++++--- cmd/entire/cli/recap_errors.go | 13 +++++++++++++ cmd/entire/cli/recap_test.go | 19 +++++++++++++++++++ 3 files changed, 52 insertions(+), 3 deletions(-) diff --git a/cmd/entire/cli/recap.go b/cmd/entire/cli/recap.go index f7a6b51f1e..ed03ef6ee5 100644 --- a/cmd/entire/cli/recap.go +++ b/cmd/entire/cli/recap.go @@ -15,6 +15,7 @@ import ( "golang.org/x/term" "github.com/entireio/cli/cmd/entire/cli/api" + "github.com/entireio/cli/cmd/entire/cli/auth" "github.com/entireio/cli/cmd/entire/cli/gitremote" "github.com/entireio/cli/cmd/entire/cli/interactive" "github.com/entireio/cli/cmd/entire/cli/paths" @@ -122,10 +123,13 @@ func runRecap(ctx context.Context, w, errW io.Writer, f *recapFlags) error { if err != nil { return err } - client, err := NewAuthenticatedAPIClient(f.insecureHTTP) + client, err := newRecapClient(f.insecureHTTP) if err != nil { - fmt.Fprintln(errW, "Sign in with `entire login` to use `entire recap`.") - return NewSilentError(err) + if errors.Is(err, api.ErrInsecureHTTP) { + fmt.Fprintf(errW, "ENTIRE_API_BASE_URL is set to an insecure http:// URL (%s). Use https:// for production, or pass --insecure-http-auth for local dev.\n", api.BaseURL()) + return NewSilentError(err) + } + return err } rangeKey := f.rangeKey() repoSlug := currentRepoSlug(ctx) @@ -154,6 +158,19 @@ func runRecap(ctx context.Context, w, errW io.Writer, f *recapFlags) error { return nil } +// newRecapClient does not gate on a missing token; FetchMeRecap surfaces 401s +// via recapLoadErrorMessage so flag effects (--week, --agent, ...) and the +// real auth error are not collapsed into one "sign in" hint. +func newRecapClient(insecureHTTP bool) (*api.Client, error) { + token, _ := auth.LookupCurrentToken() //nolint:errcheck // keyring errors are non-fatal here; the API call surfaces auth failure with the right hint + if token != "" && !insecureHTTP { + if err := api.RequireSecureURL(api.BaseURL()); err != nil { + return nil, fmt.Errorf("base URL check: %w", err) + } + } + return api.NewClient(token), nil +} + func handleRecapFetchError(w io.Writer, err error) error { if shouldShowRecapLoadErrorMessage(err) { fmt.Fprintln(w, recapLoadErrorMessage(err)) diff --git a/cmd/entire/cli/recap_errors.go b/cmd/entire/cli/recap_errors.go index 9d650a5c22..7f13853502 100644 --- a/cmd/entire/cli/recap_errors.go +++ b/cmd/entire/cli/recap_errors.go @@ -37,12 +37,25 @@ func recapLoadErrorMessage(err error) string { return err.Error() } } + if host, ok := dnsNotFoundHost(err); ok { + return fmt.Sprintf("Could not resolve API host %q (DNS lookup failed). Check ENTIRE_API_BASE_URL — the host may be misspelled or the env var may be pointing at a non-existent server. Details: %v", host, err) + } if isRecapNetworkError(err) { return fmt.Sprintf("Could not reach entire.io. Check your internet connection and ENTIRE_API_BASE_URL if you use a custom API host. Details: %v", err) } return err.Error() } +// dnsNotFoundHost reports an NXDOMAIN-style failure, distinguishing "host +// doesn't exist" from generic connectivity loss. +func dnsNotFoundHost(err error) (string, bool) { + var dnsErr *net.DNSError + if errors.As(err, &dnsErr) && dnsErr.IsNotFound { + return dnsErr.Name, true + } + return "", false +} + func recapErrorDetail(err *api.HTTPError) string { if strings.TrimSpace(err.Message) != "" { return fmt.Sprintf("HTTP %d: %s", err.StatusCode, err.Message) diff --git a/cmd/entire/cli/recap_test.go b/cmd/entire/cli/recap_test.go index 60b63ea7b9..7e27adca3c 100644 --- a/cmd/entire/cli/recap_test.go +++ b/cmd/entire/cli/recap_test.go @@ -314,6 +314,25 @@ func TestRecapLoadErrorMessage_NetworkError(t *testing.T) { } } +func TestRecapLoadErrorMessage_DNSNotFound(t *testing.T) { + t.Parallel() + + nxdomain := &net.DNSError{Name: "no-token-here.example.com", Err: "no such host", IsNotFound: true} + got := recapLoadErrorMessage(fmt.Errorf("me/recap get: %w", nxdomain)) + if strings.Contains(got, "Check your internet connection") { + t.Fatalf("NXDOMAIN should not blame internet connection:\n%s", got) + } + for _, want := range []string{ + "Could not resolve API host", + "no-token-here.example.com", + "ENTIRE_API_BASE_URL", + } { + if !strings.Contains(got, want) { + t.Fatalf("message missing %q:\n%s", want, got) + } + } +} + func TestRecapLoadErrorMessage_ContextCancellation(t *testing.T) { t.Parallel() From da6a6d4d2a47de81d57538277ee7121094f155c0 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Mon, 11 May 2026 13:11:21 -0400 Subject: [PATCH 02/11] fix(recap): surface keyring read errors with targeted message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Copilot review on #1168: newRecapClient was discarding the error from auth.LookupCurrentToken, which silently turned a keyring read failure (locked, permission denied, etc.) into a "no token" state. That re-introduced the same collapsed-error UX the PR set out to fix — the user would be told to run `entire login`, which can't help when the keyring itself is the problem. Wrap the underlying error in a *keyringReadError (preserving the cause via errors.As) and route it in runRecap to a targeted message that states the keyring is the problem and that re-login is unlikely to help. Truly-missing tokens (keyring.ErrNotFound resolves to "", nil upstream) still flow through to FetchMeRecap and the existing 401 → "Run `entire login` to re-authenticate." mapping. Co-Authored-By: Claude Opus 4.7 (1M context) Entire-Checkpoint: 60d4f6868ab0 --- cmd/entire/cli/recap.go | 30 +++++++++++++++++++++++++----- cmd/entire/cli/recap_test.go | 21 +++++++++++++++++++++ 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/cmd/entire/cli/recap.go b/cmd/entire/cli/recap.go index ed03ef6ee5..5271ef543d 100644 --- a/cmd/entire/cli/recap.go +++ b/cmd/entire/cli/recap.go @@ -125,11 +125,16 @@ func runRecap(ctx context.Context, w, errW io.Writer, f *recapFlags) error { } client, err := newRecapClient(f.insecureHTTP) if err != nil { - if errors.Is(err, api.ErrInsecureHTTP) { + var keyringErr *keyringReadError + switch { + case errors.Is(err, api.ErrInsecureHTTP): fmt.Fprintf(errW, "ENTIRE_API_BASE_URL is set to an insecure http:// URL (%s). Use https:// for production, or pass --insecure-http-auth for local dev.\n", api.BaseURL()) - return NewSilentError(err) + case errors.As(err, &keyringErr): + fmt.Fprintf(errW, "Could not read your auth token from the system keyring: %v. Running `entire login` may not help — the keyring may be locked or inaccessible. Check your OS keychain settings.\n", keyringErr.Cause) + default: + return err } - return err + return NewSilentError(err) } rangeKey := f.rangeKey() repoSlug := currentRepoSlug(ctx) @@ -158,11 +163,26 @@ func runRecap(ctx context.Context, w, errW io.Writer, f *recapFlags) error { return nil } +// keyringReadError marks a failure to read the auth token from the system +// keyring (locked, permission denied, etc.) — distinct from "no token saved", +// which keyring.ErrNotFound resolves to (token=="", err==nil) upstream. +type keyringReadError struct{ Cause error } + +func (e *keyringReadError) Error() string { + return "read auth token from keyring: " + e.Cause.Error() +} +func (e *keyringReadError) Unwrap() error { return e.Cause } + // newRecapClient does not gate on a missing token; FetchMeRecap surfaces 401s // via recapLoadErrorMessage so flag effects (--week, --agent, ...) and the -// real auth error are not collapsed into one "sign in" hint. +// real auth error are not collapsed into one "sign in" hint. A keyring read +// failure is surfaced as *keyringReadError so the caller can show a targeted +// message instead of misattributing it to a missing login. func newRecapClient(insecureHTTP bool) (*api.Client, error) { - token, _ := auth.LookupCurrentToken() //nolint:errcheck // keyring errors are non-fatal here; the API call surfaces auth failure with the right hint + token, err := auth.LookupCurrentToken() + if err != nil { + return nil, &keyringReadError{Cause: err} + } if token != "" && !insecureHTTP { if err := api.RequireSecureURL(api.BaseURL()); err != nil { return nil, fmt.Errorf("base URL check: %w", err) diff --git a/cmd/entire/cli/recap_test.go b/cmd/entire/cli/recap_test.go index 7e27adca3c..69cfff5a31 100644 --- a/cmd/entire/cli/recap_test.go +++ b/cmd/entire/cli/recap_test.go @@ -143,6 +143,27 @@ func TestRecapFlags_ColorEnabled(t *testing.T) { } } +func TestKeyringReadError_PreservesCauseAndMatchesAs(t *testing.T) { + t.Parallel() + + cause := errors.New("keychain locked") + err := error(&keyringReadError{Cause: cause}) + + if !errors.Is(err, cause) { + t.Fatalf("errors.Is should match wrapped cause; got false for %v", err) + } + var keyringErr *keyringReadError + if !errors.As(err, &keyringErr) { + t.Fatalf("errors.As should extract *keyringReadError; got false for %v", err) + } + if !errors.Is(keyringErr.Cause, cause) { + t.Fatalf("Cause = %v, want %v", keyringErr.Cause, cause) + } + if !strings.Contains(err.Error(), "keychain locked") { + t.Fatalf("Error() should include cause text; got %q", err.Error()) + } +} + func TestRunRecap_PrerequisiteErrorsUseErrorWriter(t *testing.T) { tmpDir := t.TempDir() t.Chdir(tmpDir) From 5052942367acbd91b540ecf9f19b4baed2db77bd Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Mon, 11 May 2026 19:57:00 -0400 Subject: [PATCH 03/11] fix(review): store review config in local preferences Entire-Checkpoint: 307d92a3feb5 --- cmd/entire/cli/review/cmd.go | 9 ++ cmd/entire/cli/review/migration.go | 153 +++++++++++++++++++ cmd/entire/cli/review/migration_test.go | 178 +++++++++++++++++++++++ cmd/entire/cli/review/picker.go | 62 ++++---- cmd/entire/cli/review/picker_test.go | 63 ++++---- cmd/entire/cli/settings/settings.go | 112 +++++++++++++- cmd/entire/cli/settings/settings_test.go | 78 ++++++++++ 7 files changed, 592 insertions(+), 63 deletions(-) create mode 100644 cmd/entire/cli/review/migration.go create mode 100644 cmd/entire/cli/review/migration_test.go diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index 07b0f38c65..9fe30310c5 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -156,6 +156,15 @@ Subcommands: if modes > 1 { return errors.New("--edit, --findings, and --fix are mutually exclusive") } + 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 diff --git a/cmd/entire/cli/review/migration.go b/cmd/entire/cli/review/migration.go new file mode 100644 index 0000000000..57cebad8d2 --- /dev/null +++ b/cmd/entire/cli/review/migration.go @@ -0,0 +1,153 @@ +package review + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "os" + + "github.com/entireio/cli/cmd/entire/cli/jsonutil" + "github.com/entireio/cli/cmd/entire/cli/paths" + "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 + } + + if !canPrompt { + fmt.Fprintln(errOut, "Review preferences are stored in project settings (.entire/settings.json). Run `entire review --edit` interactively to move them to local preferences.") + return nil + } + + if promptYN == nil { + promptYN = realPromptYN + } + migrate, err := promptYN(ctx, "Review preferences are stored in project settings (.entire/settings.json). Move them to local preferences now?", false) + if err != nil { + return fmt.Errorf("review settings migration prompt: %w", err) + } + if !migrate { + return nil + } + + if err := migrateProjectReviewSettings(ctx, project); err != nil { + return err + } + fmt.Fprintln(out, "Moved review preferences from project settings to local preferences.") + return nil +} + +func loadProjectReviewSettings(ctx context.Context) (*projectReviewSettings, bool, error) { + path, err := paths.AbsPath(ctx, settings.EntireSettingsFile) + if err != nil { + path = settings.EntireSettingsFile + } + + data, err := os.ReadFile(path) //nolint:gosec // path is resolved from repo settings path + if err != nil { + if os.IsNotExist(err) { + return nil, false, nil + } + return nil, false, fmt.Errorf("reading project settings for review migration: %w", err) + } + + raw := map[string]json.RawMessage{} + if err := json.Unmarshal(data, &raw); err != nil { + return nil, false, fmt.Errorf("parsing project settings for review migration: %w", err) + } + + 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 +} + +func migrateProjectReviewSettings(ctx context.Context, project *projectReviewSettings) error { + if project == nil { + return nil + } + + prefs, err := settings.LoadClonePreferences(ctx) + if err != nil { + return fmt.Errorf("load review preferences for migration: %w", err) + } + if prefs == nil { + prefs = &settings.ClonePreferences{} + } + + preferencesChanged := false + if project.hasReview && len(prefs.Review) == 0 && !isJSONNull(project.review) { + var review map[string]settings.ReviewConfig + if err := json.Unmarshal(project.review, &review); err != nil { + return fmt.Errorf("parsing project review settings: %w", err) + } + if len(review) > 0 { + prefs.Review = review + preferencesChanged = true + } + } + if project.hasFixAgent && prefs.ReviewFixAgent == "" && !isJSONNull(project.fixAgent) { + var fixAgent string + if err := json.Unmarshal(project.fixAgent, &fixAgent); err != nil { + return fmt.Errorf("parsing project review_fix_agent: %w", err) + } + if fixAgent != "" { + prefs.ReviewFixAgent = fixAgent + preferencesChanged = true + } + } + + if preferencesChanged { + if err := settings.SaveClonePreferences(ctx, prefs); err != nil { + return fmt.Errorf("save review preferences for migration: %w", err) + } + } + + delete(project.raw, "review") + delete(project.raw, "review_fix_agent") + data, err := jsonutil.MarshalIndentWithNewline(project.raw, "", " ") + if err != nil { + return fmt.Errorf("marshal project settings after review migration: %w", err) + } + //nolint:gosec // G306: settings file is config, not secrets; 0o644 is appropriate + if err := os.WriteFile(project.path, data, 0o644); err != nil { + return fmt.Errorf("write project settings after review migration: %w", err) + } + return nil +} + +func isJSONNull(raw json.RawMessage) bool { + return bytes.Equal(bytes.TrimSpace(raw), []byte("null")) +} diff --git a/cmd/entire/cli/review/migration_test.go b/cmd/entire/cli/review/migration_test.go new file mode 100644 index 0000000000..2ab36d5790 --- /dev/null +++ b/cmd/entire/cli/review/migration_test.go @@ -0,0 +1,178 @@ +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", "local preferences"} { + if !strings.Contains(promptQuestion, want) { + t.Fatalf("migration prompt = %q, want it to mention %q", promptQuestion, want) + } + } + if strings.Contains(promptQuestion, "can be committed") { + t.Fatalf("migration prompt = %q, should not mention commit risk", promptQuestion) + } + + 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) + } +} + +func TestReviewSettingsMigration_DoesNotOverwriteExistingClonePreferences(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"}}, + "review_fix_agent": "project-agent" + }`) + 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"}, + }, + ReviewFixAgent: "local-agent", + }); 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 local", got) + } + if _, ok := prefs.Review["project-agent"]; ok { + t.Fatalf("project review overwrote existing preferences: %+v", prefs.Review) + } + if prefs.ReviewFixAgent != "local-agent" { + t.Fatalf("ReviewFixAgent = %q, want local-agent", prefs.ReviewFixAgent) + } + + data, err := os.ReadFile(projectPath) + if err != nil { + t.Fatalf("read project settings: %v", err) + } + if bytes.Contains(data, []byte("review")) { + t.Fatalf("project review keys were not removed: %s", data) + } +} + +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) + } +} diff --git a/cmd/entire/cli/review/picker.go b/cmd/entire/cli/review/picker.go index 723e243e22..a6476727f3 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. // @@ -116,7 +116,7 @@ func RunReviewConfigPicker(ctx context.Context, out io.Writer, getInstalled func if len(configurable) == 0 { return nil, errors.New( "no installed agents have curated review skills; " + - "edit .entire/settings.json directly under review.", + "edit local review preferences directly under review.", ) } @@ -224,7 +224,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 +249,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 a4c6a4acd2..2cc4d3affc 100644 --- a/cmd/entire/cli/settings/settings.go +++ b/cmd/entire/cli/settings/settings.go @@ -17,6 +17,7 @@ import ( "github.com/entireio/cli/cmd/entire/cli/jsonutil" "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/session" ) const ( @@ -24,6 +25,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 +114,13 @@ 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. +type ClonePreferences struct { + Review map[string]ReviewConfig `json:"review,omitempty"` + ReviewFixAgent string `json:"review_fix_agent,omitempty"` +} + // SummaryGenerationSettings configures provider selection for on-demand // checkpoint summaries generated by explain --generate. type SummaryGenerationSettings struct { @@ -240,21 +250,33 @@ 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 + } 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 { @@ -286,6 +308,33 @@ func LoadFromFile(filePath string) (*EntireSettings, error) { return loadFromFile(filePath) } +// 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) { @@ -331,6 +380,58 @@ 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) + } + + dec := json.NewDecoder(bytes.NewReader(data)) + dec.DisallowUnknownFields() + if err := dec.Decode(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) + } + + //nolint:gosec // G306: preferences file is config, not secrets; 0o644 is appropriate + if err := os.WriteFile(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. func mergeJSON(settings *EntireSettings, data []byte) error { @@ -360,6 +461,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 { diff --git a/cmd/entire/cli/settings/settings_test.go b/cmd/entire/cli/settings/settings_test.go index 2fbaefda4a..a8ea7ca9a4 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 ( @@ -1012,6 +1015,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{ From df11c92764bca856a479796d77237c4e99bcd5e6 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Mon, 11 May 2026 22:11:10 -0400 Subject: [PATCH 04/11] fix(review): clarify local preference messaging Entire-Checkpoint: a1631c3ee912 --- cmd/entire/cli/review/cmd.go | 8 +++++--- cmd/entire/cli/review/picker.go | 18 ++++++++++++++---- cmd/entire/cli/settings/settings.go | 11 +++++++---- 3 files changed, 26 insertions(+), 11 deletions(-) diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index 9fe30310c5..295fad3491 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -108,8 +108,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. @@ -218,7 +219,8 @@ func runReview(ctx context.Context, cmd *cobra.Command, agentOverride string, de 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/picker.go b/cmd/entire/cli/review/picker.go index a6476727f3..d6f39e5d0d 100644 --- a/cmd/entire/cli/review/picker.go +++ b/cmd/entire/cli/review/picker.go @@ -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 local review preferences 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, ) } diff --git a/cmd/entire/cli/settings/settings.go b/cmd/entire/cli/settings/settings.go index 2cc4d3affc..72ae88fa27 100644 --- a/cmd/entire/cli/settings/settings.go +++ b/cmd/entire/cli/settings/settings.go @@ -240,9 +240,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 @@ -433,7 +434,9 @@ func applyClonePreferences(settings *EntireSettings, prefs *ClonePreferences) { } // 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)) From 5a9b0630d0b4f3dabf33edf28843c65efaa80a6a Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 12 May 2026 00:41:45 -0400 Subject: [PATCH 05/11] review: harden migration against data loss and partial writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The review-settings migration rewrites two files (clone-local prefs + the committed project settings). Three safety issues addressed: 1. Atomic writes (jsonutil.WriteFileAtomic). Both writers used plain os.WriteFile, so SIGINT or disk-full mid-write truncated the project .entire/settings.json to partial JSON — every subsequent CLI invocation then failed to parse settings. Replaces os.WriteFile in saveClonePreferencesToFile, saveToFile, and the migration's project rewrite with a temp-then-rename helper. 2. Conflict detection. The old migration silently dropped project review config whenever clone prefs were already populated, then unconditionally stripped the project keys — destroying user data without notice. Now detects same-key-different-value conflicts and aborts with a clear "reconcile manually" message; non-overlapping entries are merged. 3. Honest success message. The "Moved ..." line printed even when nothing actually moved (e.g. project had `review: null`). Now branches on whether prefs changed, distinguishing migration from cleanup. Test changes: - TestReviewSettingsMigration_DoesNotOverwriteExistingClonePreferences (which enshrined the old silent-drop) is replaced by _MergesNonOverlappingPrefs covering the safe merge case. - New _RefusesConflictingPrefs asserts the abort message names the conflicting agent and the project file is untouched on the abort path. - New _NoMoveCleansUpKeys covers the cleanup-only path (`review: null` → prefs unchanged, project keys stripped). - Existing happy-path test's prompt assertion is updated for the new copy ("typically committed", "clone-local preferences"), which surfaces the why behind the migration. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/entire/cli/jsonutil/write.go | 47 ++++++++ cmd/entire/cli/review/migration.go | 128 ++++++++++++++++++---- cmd/entire/cli/review/migration_test.go | 139 +++++++++++++++++++++--- cmd/entire/cli/settings/settings.go | 6 +- 4 files changed, 277 insertions(+), 43 deletions(-) create mode 100644 cmd/entire/cli/jsonutil/write.go diff --git a/cmd/entire/cli/jsonutil/write.go b/cmd/entire/cli/jsonutil/write.go new file mode 100644 index 0000000000..ddea5b5933 --- /dev/null +++ b/cmd/entire/cli/jsonutil/write.go @@ -0,0 +1,47 @@ +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 and renaming into place. 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. +// +// 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.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 + return nil +} diff --git a/cmd/entire/cli/review/migration.go b/cmd/entire/cli/review/migration.go index 57cebad8d2..4735d0cb52 100644 --- a/cmd/entire/cli/review/migration.go +++ b/cmd/entire/cli/review/migration.go @@ -38,14 +38,16 @@ func maybePromptReviewSettingsMigration( } if !canPrompt { - fmt.Fprintln(errOut, "Review preferences are stored in project settings (.entire/settings.json). Run `entire review --edit` interactively to move them to local preferences.") + 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). Move them to local preferences now?", false) + 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) } @@ -53,10 +55,15 @@ func maybePromptReviewSettingsMigration( return nil } - if err := migrateProjectReviewSettings(ctx, project); err != nil { + moved, err := migrateProjectReviewSettings(ctx, project) + if err != nil { return err } - fmt.Fprintln(out, "Moved review preferences from project settings to local preferences.") + 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 } @@ -94,44 +101,76 @@ func loadProjectReviewSettings(ctx context.Context) (*projectReviewSettings, boo }, true, nil } -func migrateProjectReviewSettings(ctx context.Context, project *projectReviewSettings) error { +// 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 nil + return false, nil } prefs, err := settings.LoadClonePreferences(ctx) if err != nil { - return fmt.Errorf("load review preferences for migration: %w", err) + return false, fmt.Errorf("load review preferences for migration: %w", err) } if prefs == nil { prefs = &settings.ClonePreferences{} } preferencesChanged := false - if project.hasReview && len(prefs.Review) == 0 && !isJSONNull(project.review) { - var review map[string]settings.ReviewConfig - if err := json.Unmarshal(project.review, &review); err != nil { - return fmt.Errorf("parsing project review settings: %w", err) + 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(review) > 0 { - prefs.Review = review - preferencesChanged = true + 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 && prefs.ReviewFixAgent == "" && !isJSONNull(project.fixAgent) { + if project.hasFixAgent && !isJSONNull(project.fixAgent) { var fixAgent string if err := json.Unmarshal(project.fixAgent, &fixAgent); err != nil { - return fmt.Errorf("parsing project review_fix_agent: %w", err) + return false, fmt.Errorf("parsing project review_fix_agent: %w", err) } if fixAgent != "" { - prefs.ReviewFixAgent = fixAgent - preferencesChanged = true + 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 fmt.Errorf("save review preferences for migration: %w", err) + return false, fmt.Errorf("save review preferences for migration: %w", err) } } @@ -139,13 +178,54 @@ func migrateProjectReviewSettings(ctx context.Context, project *projectReviewSet delete(project.raw, "review_fix_agent") data, err := jsonutil.MarshalIndentWithNewline(project.raw, "", " ") if err != nil { - return fmt.Errorf("marshal project settings after review migration: %w", err) + return false, fmt.Errorf("marshal project settings after review migration: %w", err) } - //nolint:gosec // G306: settings file is config, not secrets; 0o644 is appropriate - if err := os.WriteFile(project.path, data, 0o644); err != nil { - return fmt.Errorf("write project settings after review migration: %w", err) + if err := jsonutil.WriteFileAtomic(project.path, data, 0o644); err != nil { + return false, fmt.Errorf("write project settings after review migration: %w", err) } - return nil + 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 { diff --git a/cmd/entire/cli/review/migration_test.go b/cmd/entire/cli/review/migration_test.go index 2ab36d5790..fb98945c35 100644 --- a/cmd/entire/cli/review/migration_test.go +++ b/cmd/entire/cli/review/migration_test.go @@ -48,14 +48,11 @@ func TestReviewSettingsMigration_MovesProjectReviewToClonePreferences(t *testing if !prompted { t.Fatal("expected migration prompt") } - for _, want := range []string{"project settings", "local preferences"} { + 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) } } - if strings.Contains(promptQuestion, "can be committed") { - t.Fatalf("migration prompt = %q, should not mention commit risk", promptQuestion) - } prefs, err := settings.LoadClonePreferences(context.Background()) if err != nil { @@ -87,7 +84,11 @@ func TestReviewSettingsMigration_MovesProjectReviewToClonePreferences(t *testing } } -func TestReviewSettingsMigration_DoesNotOverwriteExistingClonePreferences(t *testing.T) { +// 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) @@ -100,8 +101,7 @@ func TestReviewSettingsMigration_DoesNotOverwriteExistingClonePreferences(t *tes projectPath := filepath.Join(entireDir, "settings.json") projectSettings := []byte(`{ "enabled": true, - "review": {"project-agent": {"prompt": "project"}}, - "review_fix_agent": "project-agent" + "review": {"project-agent": {"prompt": "project"}} }`) if err := os.WriteFile(projectPath, projectSettings, 0o600); err != nil { t.Fatalf("write project settings: %v", err) @@ -110,7 +110,6 @@ func TestReviewSettingsMigration_DoesNotOverwriteExistingClonePreferences(t *tes Review: map[string]settings.ReviewConfig{ "local-agent": {Prompt: "local"}, }, - ReviewFixAgent: "local-agent", }); err != nil { t.Fatalf("seed preferences: %v", err) } @@ -127,21 +126,131 @@ func TestReviewSettingsMigration_DoesNotOverwriteExistingClonePreferences(t *tes t.Fatalf("load preferences: %v", err) } if got := prefs.Review["local-agent"].Prompt; got != "local" { - t.Fatalf("local prompt = %q, want local", got) + 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 _, ok := prefs.Review["project-agent"]; ok { - t.Fatalf("project review overwrote existing preferences: %+v", prefs.Review) + if !strings.Contains(err.Error(), "claude-code") { + t.Errorf("error = %q, want it to name the conflicting agent (claude-code)", err.Error()) } - if prefs.ReviewFixAgent != "local-agent" { - t.Fatalf("ReviewFixAgent = %q, want local-agent", prefs.ReviewFixAgent) + 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("review")) { - t.Fatalf("project review keys were not removed: %s", data) + 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) } } diff --git a/cmd/entire/cli/settings/settings.go b/cmd/entire/cli/settings/settings.go index 72ae88fa27..14fac851b0 100644 --- a/cmd/entire/cli/settings/settings.go +++ b/cmd/entire/cli/settings/settings.go @@ -414,8 +414,7 @@ func saveClonePreferencesToFile(prefs *ClonePreferences, filePath string) error return fmt.Errorf("marshaling preferences: %w", err) } - //nolint:gosec // G306: preferences file is config, not secrets; 0o644 is appropriate - if err := os.WriteFile(filePath, data, 0o644); err != nil { + if err := jsonutil.WriteFileAtomic(filePath, data, 0o644); err != nil { return fmt.Errorf("writing preferences file: %w", err) } return nil @@ -1062,8 +1061,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 From 484acb0ce5f3a25af0a242c611c0ceff52fa8108 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 12 May 2026 00:47:42 -0400 Subject: [PATCH 06/11] review: stop nagging on every invocation and skip irrelevant flows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two UX fixes for the migration prompt: 1. Persist user dismissal. Declining the prompt previously persisted no state, so `entire review` re-prompted on every run — a real annoyance for teams who intentionally commit review prefs. Adds a ReviewMigrationDismissed bool to ClonePreferences; once set, the prompt is skipped silently. Users who change their mind can clear the key from preferences.json. 2. Only prompt for paths that interact with the picker. The prompt previously fired in `RunE` before the mode branches, so `entire review --findings` (read-only browsing) and `entire review --fix` (uses ReviewFixAgent only) interrupted users with a prompt irrelevant to what they were doing. Now gated on `!findings && !fix`. Also documents ClonePreferences's purpose at the type level. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/entire/cli/review/cmd.go | 23 ++++++--- cmd/entire/cli/review/migration.go | 18 +++++++ cmd/entire/cli/review/migration_test.go | 65 +++++++++++++++++++++++++ cmd/entire/cli/settings/settings.go | 10 ++++ 4 files changed, 108 insertions(+), 8 deletions(-) diff --git a/cmd/entire/cli/review/cmd.go b/cmd/entire/cli/review/cmd.go index 295fad3491..d0b6e3dbef 100644 --- a/cmd/entire/cli/review/cmd.go +++ b/cmd/entire/cli/review/cmd.go @@ -157,14 +157,21 @@ Subcommands: if modes > 1 { return errors.New("--edit, --findings, and --fix are mutually exclusive") } - if err := maybePromptReviewSettingsMigration( - ctx, - cmd.OutOrStdout(), - cmd.ErrOrStderr(), - interactive.IsTerminalWriter(cmd.OutOrStdout()) && interactive.CanPromptInteractively(), - deps.PromptYN, - ); err != nil { - return err + // 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) diff --git a/cmd/entire/cli/review/migration.go b/cmd/entire/cli/review/migration.go index 4735d0cb52..0c1d9459d3 100644 --- a/cmd/entire/cli/review/migration.go +++ b/cmd/entire/cli/review/migration.go @@ -37,6 +37,17 @@ func maybePromptReviewSettingsMigration( 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 + } + if !canPrompt { 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.") @@ -52,6 +63,13 @@ func maybePromptReviewSettingsMigration( 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 } diff --git a/cmd/entire/cli/review/migration_test.go b/cmd/entire/cli/review/migration_test.go index fb98945c35..e55c15ba48 100644 --- a/cmd/entire/cli/review/migration_test.go +++ b/cmd/entire/cli/review/migration_test.go @@ -254,6 +254,71 @@ func TestReviewSettingsMigration_NoMoveCleansUpKeys(t *testing.T) { } } +// 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) diff --git a/cmd/entire/cli/settings/settings.go b/cmd/entire/cli/settings/settings.go index 14fac851b0..0048662df3 100644 --- a/cmd/entire/cli/settings/settings.go +++ b/cmd/entire/cli/settings/settings.go @@ -116,9 +116,19 @@ type EntireSettings struct { // 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 From 15be35423b79c4d18df79c5d94a77972c86abd26 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 12 May 2026 00:54:52 -0400 Subject: [PATCH 07/11] review: tighten settings boundary and surface load-path failures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four correctness/forward-compat fixes around the settings package boundary that the new migration introduced: 1. Route project-file IO through settings. migration.go previously used os.ReadFile and bare jsonutil writes against .entire/settings.json, duplicating settings-parsing logic and violating CLAUDE.md's "Settings access must go through the settings package" rule. Adds LoadProjectRaw / SaveProjectRaw helpers; the migration now calls those and never touches paths/os directly. 2. Lenient decoding for clone preferences. The file lives in .git/ and persists across CLI versions running against the same clone, so DisallowUnknownFields meant a newer binary's added field bricked an older binary's settings.Load. Dropped strict decoding here; accepted trade-off is that hand-edit typos in this file are silently ignored (documented in-place). 3. Surface ClonePreferencesPath failures. settings.Load() previously dropped any error from ClonePreferencesPath silently, so a git PATH issue or .git/ permission failure would silently disable the prefs layer and the user's picker choices would seem to vanish. Now logs at Debug — findable via ENTIRE_LOG_LEVEL=debug when triaging "my preferences aren't loading" without spamming the non-repo commands that legitimately fail this resolution. 4. Non-interactive migration logs at Warn. Scripted/CI invocations previously emitted a one-shot stderr hint that scrolled past unnoticed. Adds a structured logging.Warn alongside the stderr message so operators tailing .entire/logs/ catch the pending migration even when the user never sees the prompt. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/entire/cli/review/migration.go | 38 +++++++---------- cmd/entire/cli/settings/settings.go | 66 +++++++++++++++++++++++++++-- 2 files changed, 78 insertions(+), 26 deletions(-) diff --git a/cmd/entire/cli/review/migration.go b/cmd/entire/cli/review/migration.go index 0c1d9459d3..0cde1a67ec 100644 --- a/cmd/entire/cli/review/migration.go +++ b/cmd/entire/cli/review/migration.go @@ -6,10 +6,9 @@ import ( "encoding/json" "fmt" "io" - "os" + "log/slog" - "github.com/entireio/cli/cmd/entire/cli/jsonutil" - "github.com/entireio/cli/cmd/entire/cli/paths" + "github.com/entireio/cli/cmd/entire/cli/logging" "github.com/entireio/cli/cmd/entire/cli/settings" ) @@ -49,6 +48,13 @@ func maybePromptReviewSettingsMigration( } 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.") @@ -86,22 +92,12 @@ func maybePromptReviewSettingsMigration( } func loadProjectReviewSettings(ctx context.Context) (*projectReviewSettings, bool, error) { - path, err := paths.AbsPath(ctx, settings.EntireSettingsFile) + path, raw, exists, err := settings.LoadProjectRaw(ctx) if err != nil { - path = settings.EntireSettingsFile + return nil, false, fmt.Errorf("review migration: %w", err) } - - data, err := os.ReadFile(path) //nolint:gosec // path is resolved from repo settings path - if err != nil { - if os.IsNotExist(err) { - return nil, false, nil - } - return nil, false, fmt.Errorf("reading project settings for review migration: %w", err) - } - - raw := map[string]json.RawMessage{} - if err := json.Unmarshal(data, &raw); err != nil { - return nil, false, fmt.Errorf("parsing project settings for review migration: %w", err) + if !exists { + return nil, false, nil } reviewRaw, hasReview := raw["review"] @@ -194,12 +190,8 @@ func migrateProjectReviewSettings(ctx context.Context, project *projectReviewSet delete(project.raw, "review") delete(project.raw, "review_fix_agent") - data, err := jsonutil.MarshalIndentWithNewline(project.raw, "", " ") - if err != nil { - return false, fmt.Errorf("marshal project settings after review migration: %w", err) - } - if err := jsonutil.WriteFileAtomic(project.path, data, 0o644); err != nil { - return false, fmt.Errorf("write project settings after review migration: %w", err) + 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 } diff --git a/cmd/entire/cli/settings/settings.go b/cmd/entire/cli/settings/settings.go index 0048662df3..f34a299aab 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,6 +17,7 @@ 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" ) @@ -264,6 +266,14 @@ func Load(ctx context.Context) (*EntireSettings, error) { 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 { @@ -319,6 +329,53 @@ 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 +} + +// 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) @@ -402,9 +459,12 @@ func loadClonePreferencesFromFile(filePath string) (*ClonePreferences, error) { return nil, fmt.Errorf("%w", err) } - dec := json.NewDecoder(bytes.NewReader(data)) - dec.DisallowUnknownFields() - if err := dec.Decode(prefs); err != nil { + // Lenient decoding: clone preferences live in .git/ and persist across CLI + // versions running against the same clone. A newer binary may write a + // field an older binary doesn't know about — strict decoding would brick + // the older binary's settings.Load. The trade-off is that typos in + // hand-edited fields are silently ignored; document & accept. + if err := json.Unmarshal(data, prefs); err != nil { return nil, fmt.Errorf("parsing preferences file: %w", err) } return prefs, nil From 48684bf62ad02f6ab9bda5787414036825370cbf Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 12 May 2026 22:05:43 -0400 Subject: [PATCH 08/11] review: harden atomic write fsync + bail migration on local-override conflict MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three small follow-ups from review of this PR: 1. WriteFileAtomic now fsyncs the temp file between Write and Close. The prior implementation relied on Close to flush, which doesn't sync data to disk on its own — 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. tmp.Sync before Close closes that gap and matches the comment's "leaves the original file intact rather than a truncated partial" promise. 2. Bail the migration up front when .entire/settings.local.json already has review keys. settings.local.json overrides clone-local preferences via mergeJSON's wholesale-replace path, so migrating without cleaning the local file first silently nullifies the migration on the next settings.Load — the user clicks "yes", their config moves to clone prefs, then the local override hides it. Now detects the precondition, prints a clear "remove these keys from settings.local.json" message, and returns without prompting or setting ReviewMigrationDismissed (this is a fixable precondition, not a user-rejected migration). New settings helper LoadLocalRaw mirrors LoadProjectRaw so the IO stays inside the settings package per CLAUDE.md's boundary rule. 3. Expand the strict/lenient decoding comment to explain why EntireSettings stays strict while ClonePreferences is lenient. The prior comment justified the lenient half but a reader of loadFromFile saw DisallowUnknownFields without a paired explanation. The new comment makes the asymmetry deliberate and self-documenting. New test TestReviewSettingsMigration_BailsOnLocalSettingsReviewKeys pins the bail contract: prompt not called, project file untouched, ReviewMigrationDismissed not set, and the warning message names both settings.local.json and the conflicting keys. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/entire/cli/jsonutil/write.go | 17 ++++-- cmd/entire/cli/review/migration.go | 42 +++++++++++++++ cmd/entire/cli/review/migration_test.go | 69 +++++++++++++++++++++++++ cmd/entire/cli/settings/settings.go | 43 +++++++++++++-- 4 files changed, 162 insertions(+), 9 deletions(-) diff --git a/cmd/entire/cli/jsonutil/write.go b/cmd/entire/cli/jsonutil/write.go index ddea5b5933..bdf83d3942 100644 --- a/cmd/entire/cli/jsonutil/write.go +++ b/cmd/entire/cli/jsonutil/write.go @@ -8,10 +8,15 @@ import ( ) // WriteFileAtomic writes data to filePath atomically by writing to a temp file -// in the same directory and renaming into place. 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. +// in the same directory, fsyncing it, and renaming into place. 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. // // 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. @@ -33,6 +38,10 @@ func WriteFileAtomic(filePath string, data []byte, perm fs.FileMode) error { _ = 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) } diff --git a/cmd/entire/cli/review/migration.go b/cmd/entire/cli/review/migration.go index 0cde1a67ec..ad2fe8a3c2 100644 --- a/cmd/entire/cli/review/migration.go +++ b/cmd/entire/cli/review/migration.go @@ -47,6 +47,27 @@ func maybePromptReviewSettingsMigration( 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 @@ -241,3 +262,24 @@ func reviewConfigEqual(a, b settings.ReviewConfig) bool { 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 index e55c15ba48..97d30eec00 100644 --- a/cmd/entire/cli/review/migration_test.go +++ b/cmd/entire/cli/review/migration_test.go @@ -350,3 +350,72 @@ func TestReviewSettingsMigration_SkipsWhenProjectHasNoReviewKeys(t *testing.T) { 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/settings/settings.go b/cmd/entire/cli/settings/settings.go index f34a299aab..d5a707b814 100644 --- a/cmd/entire/cli/settings/settings.go +++ b/cmd/entire/cli/settings/settings.go @@ -362,6 +362,33 @@ func LoadProjectRaw(ctx context.Context) (path string, raw map[string]json.RawMe 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. @@ -459,11 +486,17 @@ func loadClonePreferencesFromFile(filePath string) (*ClonePreferences, error) { return nil, fmt.Errorf("%w", err) } - // Lenient decoding: clone preferences live in .git/ and persist across CLI - // versions running against the same clone. A newer binary may write a - // field an older binary doesn't know about — strict decoding would brick - // the older binary's settings.Load. The trade-off is that typos in - // hand-edited fields are silently ignored; document & accept. + // 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) } From 6c7c233f71edb5f1872d9d28922f28dd7cc37984 Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Tue, 12 May 2026 22:14:13 -0400 Subject: [PATCH 09/11] settings: update test for loadMergedSettings new signature after main merge The redaction custom-rules test merged in from main calls loadMergedSettings(base, local), but PR 1181's merge adds a preferencesFileAbs argument between them. Passing "" for that arg skips the clone-preferences layer, which matches the test's intent (it only exercises project + local merge). Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/entire/cli/settings/settings_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/cmd/entire/cli/settings/settings_test.go b/cmd/entire/cli/settings/settings_test.go index 9414ff941b..27e7b22b90 100644 --- a/cmd/entire/cli/settings/settings_test.go +++ b/cmd/entire/cli/settings/settings_test.go @@ -1005,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) } From 7b578c45fd7cac08fffc5dd84fb94bdda23449df Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 13 May 2026 16:19:48 -0400 Subject: [PATCH 10/11] jsonutil: fsync parent dir after rename + add WriteFileAtomic tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-ups from review of this PR: 1. WriteFileAtomic now best-effort fsyncs the parent directory after the rename. The prior code fsynced the temp file (so contents are durable) and renamed it into place, but never synced the directory entry. On a hard crash POSIX can replay the file-contents fsync without the rename, leaving the original file in place even though os.Rename returned nil — which silently breaks the docstring's "leaves the original intact rather than a truncated partial" promise. The fsync is best-effort because Windows doesn't support directory sync and the rename has already succeeded by the time we reach it; failing here would mislead callers whose write is done. 2. Add direct test coverage for the package. WriteFileAtomic is now the entry point for every settings writer in the codebase (settings.go:373, 487, 1134) but had no tests of its own. Covers happy path, replace-existing, perm-bit application, no-leftover on success, temp cleanup on rename failure, and missing-parent error propagation. The rename-failure test reaches the failing step by renaming onto a non-empty directory, which fails on every POSIX filesystem and on Windows without needing fault injection. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/entire/cli/jsonutil/write.go | 24 +++- cmd/entire/cli/jsonutil/write_test.go | 157 ++++++++++++++++++++++++++ 2 files changed, 177 insertions(+), 4 deletions(-) create mode 100644 cmd/entire/cli/jsonutil/write_test.go diff --git a/cmd/entire/cli/jsonutil/write.go b/cmd/entire/cli/jsonutil/write.go index bdf83d3942..e59709fa98 100644 --- a/cmd/entire/cli/jsonutil/write.go +++ b/cmd/entire/cli/jsonutil/write.go @@ -8,16 +8,24 @@ import ( ) // WriteFileAtomic writes data to filePath atomically by writing to a temp file -// in the same directory, fsyncing it, and renaming into place. 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. +// 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 { @@ -52,5 +60,13 @@ func WriteFileAtomic(filePath string, data []byte, perm fs.FileMode) error { 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 { + _ = d.Sync() + _ = 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) + } +} From e4267cd966822831f6f42c33fd7e676bc870599f Mon Sep 17 00:00:00 2001 From: Peyton Montei Date: Wed, 13 May 2026 16:25:01 -0400 Subject: [PATCH 11/11] jsonutil: silence gosec/errcheck on best-effort parent-dir fsync MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit golangci-lint config has errcheck.check-blank=true so `_ = d.Sync()` gets flagged even with blank assignment, and gosec G304 flags os.Open(dir) on a variable path. Both are intentional here — dir is filepath.Dir of the caller-supplied destination, and the directory fsync is best-effort because the rename has already succeeded. Add nolint comments matching the codebase pattern in transcript.go and trace.go. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/entire/cli/jsonutil/write.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/entire/cli/jsonutil/write.go b/cmd/entire/cli/jsonutil/write.go index e59709fa98..38f4f02ae0 100644 --- a/cmd/entire/cli/jsonutil/write.go +++ b/cmd/entire/cli/jsonutil/write.go @@ -64,8 +64,8 @@ func WriteFileAtomic(filePath string, data []byte, perm fs.FileMode) error { // 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 { - _ = d.Sync() + 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