Move review preferences from project settings to local preferences#1181
Merged
Conversation
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) <noreply@anthropic.com> Entire-Checkpoint: d534f23017c5
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) <noreply@anthropic.com> Entire-Checkpoint: 60d4f6868ab0
Entire-Checkpoint: 307d92a3feb5
Contributor
There was a problem hiding this comment.
Pull request overview
This PR moves review configuration out of the committed project settings (.entire/settings.json) into clone-local preferences stored under the git common dir (shared across worktrees), and adds a one-time interactive migration path. It also improves entire recap error handling by distinguishing keyring-read failures and DNS NXDOMAIN from other connectivity/auth errors.
Changes:
- Add clone-local preferences (
.git/entire/preferences.json) and merge them into loaded settings with precedence between project, clone, and local overrides. - Update review config persistence to write/read clone-local preferences and add an interactive migration that removes legacy review keys from project settings.
- Improve recap client creation and error messaging (keyring-read errors, insecure base URL, DNS-not-found host).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| cmd/entire/cli/settings/settings.go | Introduces clone-local preferences (path resolution, load/save, merge into settings) and adds wholesale review merge behavior. |
| cmd/entire/cli/settings/settings_test.go | Adds tests for clone-preferences precedence and wholesale review replacement behavior. |
| cmd/entire/cli/review/picker.go | Switches review config persistence from project settings to clone-local preferences; updates user messaging. |
| cmd/entire/cli/review/picker_test.go | Updates tests to validate writing/reading clone-local preferences and preserving fix-agent preference. |
| cmd/entire/cli/review/migration.go | New migration helper to move legacy review/review_fix_agent keys from project settings into clone-local preferences. |
| cmd/entire/cli/review/migration_test.go | Adds coverage for migration prompt flow, non-overwrite behavior, and no-op cases. |
| cmd/entire/cli/review/cmd.go | Triggers the migration prompt at the start of entire review execution. |
| cmd/entire/cli/recap.go | Adds newRecapClient with targeted error classification (keyring vs insecure URL vs other). |
| cmd/entire/cli/recap_test.go | Tests keyring error wrapping and DNS NXDOMAIN messaging behavior. |
| cmd/entire/cli/recap_errors.go | Adds DNS NXDOMAIN detection to provide a more accurate user-facing error message. |
Entire-Checkpoint: a1631c3ee912
Contributor
Author
|
@BugBot review |
peyton-alt
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit df11c92. Configure here.
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
…conflict 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) <noreply@anthropic.com>
… 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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Soph
approved these changes
May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
https://entire.io/gh/entireio/cli/trails/356
Summary
entire reviewpicker persistence from committed.entire/settings.jsonintoclone-local
<git-common-dir>/entire/preferences.json..entire/settings.local.json, including areviewmerge fix.strip the project keys after migration.
Why
entire review --editwas writing personal review preferences into project settings,making them easy to commit. The new clone-local preference layer keeps review choices
uncommitted while sharing them across linked worktrees in the same clone.
Validation
mise run checkgo test ./cmd/entire/cli/... -count=1.entire/settings.jsonto.git/ entire/preferences.jsonin a temp repo.Notes
explicitly requested
Note
Medium Risk
Medium risk because it changes settings load precedence and introduces a new clone-local preferences file plus an on-run migration that rewrites
.entire/settings.json, which could affect user configuration if bugs slip through. Also tweaksentire recapauth/network error handling, impacting CLI UX for auth and connectivity failures.Overview
entire reviewnow stores review configuration in clone-local preferences (git common direntire/preferences.json) instead of committed.entire/settings.json, and updates picker/help text accordingly.Adds a one-time interactive migration prompt that copies
review/review_fix_agentfrom project settings into clone-local preferences (without overwriting existing local prefs) and then removes those keys from.entire/settings.json.Extends
settings.Loadto apply project settings → clone-local preferences →.entire/settings.local.json, and fixes merge semantics so a presentreviewkey wholesale replaces prior review config.Improves
entire recapfailure UX by creating a token lookup path that distinguishes keyring read failures vs missing tokens, enforces HTTPS only when a token exists, and adds a clearer NXDOMAIN/DNS-not-found error message.Reviewed by Cursor Bugbot for commit df11c92. Configure here.