|
| 1 | +package review |
| 2 | + |
| 3 | +import ( |
| 4 | + "bytes" |
| 5 | + "context" |
| 6 | + "encoding/json" |
| 7 | + "fmt" |
| 8 | + "io" |
| 9 | + "log/slog" |
| 10 | + |
| 11 | + "github.com/GrayCodeAI/trace/cmd/trace/cli/logging" |
| 12 | + "github.com/GrayCodeAI/trace/cmd/trace/cli/settings" |
| 13 | +) |
| 14 | + |
| 15 | +type projectReviewSettings struct { |
| 16 | + path string |
| 17 | + raw map[string]json.RawMessage |
| 18 | + review json.RawMessage |
| 19 | + fixAgent json.RawMessage |
| 20 | + hasReview bool |
| 21 | + hasFixAgent bool |
| 22 | +} |
| 23 | + |
| 24 | +func maybePromptReviewSettingsMigration( |
| 25 | + ctx context.Context, |
| 26 | + out io.Writer, |
| 27 | + errOut io.Writer, |
| 28 | + canPrompt bool, |
| 29 | + promptYN func(context.Context, string, bool) (bool, error), |
| 30 | +) error { |
| 31 | + project, ok, err := loadProjectReviewSettings(ctx) |
| 32 | + if err != nil { |
| 33 | + return err |
| 34 | + } |
| 35 | + if !ok { |
| 36 | + return nil |
| 37 | + } |
| 38 | + |
| 39 | + // Skip the prompt entirely if the user has already declined. Without this, |
| 40 | + // teams who intentionally commit review prefs would be re-prompted on |
| 41 | + // every invocation of `entire review`. |
| 42 | + prefs, prefsErr := settings.LoadClonePreferences(ctx) |
| 43 | + if prefsErr != nil { |
| 44 | + return fmt.Errorf("load review preferences for migration: %w", prefsErr) |
| 45 | + } |
| 46 | + if prefs != nil && prefs.ReviewMigrationDismissed { |
| 47 | + return nil |
| 48 | + } |
| 49 | + |
| 50 | + // Bail before prompting if .entire/settings.local.json already has review |
| 51 | + // keys. settings.local.json overrides clone-local preferences (mergeJSON |
| 52 | + // wholesale-replaces the review map), so migrating without cleaning the |
| 53 | + // local file first would silently nullify the migration on the very next |
| 54 | + // settings.Load — the user clicks "yes", their config moves to clone |
| 55 | + // prefs, then the local override hides it. Better to surface the |
| 56 | + // precondition up front than to leave the user wondering why their |
| 57 | + // migrated config disappeared. |
| 58 | + // |
| 59 | + // Intentionally does NOT set ReviewMigrationDismissed: this is a fixable |
| 60 | + // precondition, not a user-rejected migration; the prompt should fire |
| 61 | + // again on the next run after the user cleans settings.local.json. |
| 62 | + if localHas, localPath, localErr := localSettingsHasReviewKeys(ctx); localErr != nil { |
| 63 | + return fmt.Errorf("inspect local settings for migration: %w", localErr) |
| 64 | + } else if localHas { |
| 65 | + fmt.Fprintln(errOut, "Cannot migrate review preferences: .entire/settings.local.json also has review keys.") |
| 66 | + fmt.Fprintf(errOut, "Those override clone-local preferences and would mask the migration. Remove the\n") |
| 67 | + fmt.Fprintf(errOut, "`review` / `review_fix_agent` keys from %s, then re-run `entire review`.\n", localPath) |
| 68 | + return nil |
| 69 | + } |
| 70 | + |
| 71 | + if !canPrompt { |
| 72 | + // Log at Warn so operators tailing .entire/logs/ catch the pending |
| 73 | + // migration on scripted/CI invocations where the stderr hint may |
| 74 | + // scroll past unnoticed. |
| 75 | + logging.Warn(ctx, "review migration pending: project settings has review keys that may be committed", |
| 76 | + slog.String("project_settings_path", project.path), |
| 77 | + slog.Bool("has_review", project.hasReview), |
| 78 | + slog.Bool("has_fix_agent", project.hasFixAgent)) |
| 79 | + fmt.Fprintln(errOut, "Review preferences are stored in project settings (.entire/settings.json).") |
| 80 | + fmt.Fprintln(errOut, "These are typically committed and may be visible to teammates.") |
| 81 | + fmt.Fprintln(errOut, "Run `entire review --edit` interactively to move them to clone-local preferences.") |
| 82 | + return nil |
| 83 | + } |
| 84 | + |
| 85 | + if promptYN == nil { |
| 86 | + promptYN = realPromptYN |
| 87 | + } |
| 88 | + 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) |
| 89 | + if err != nil { |
| 90 | + return fmt.Errorf("review settings migration prompt: %w", err) |
| 91 | + } |
| 92 | + if !migrate { |
| 93 | + if prefs == nil { |
| 94 | + prefs = &settings.ClonePreferences{} |
| 95 | + } |
| 96 | + prefs.ReviewMigrationDismissed = true |
| 97 | + if err := settings.SaveClonePreferences(ctx, prefs); err != nil { |
| 98 | + return fmt.Errorf("save migration dismissal: %w", err) |
| 99 | + } |
| 100 | + return nil |
| 101 | + } |
| 102 | + |
| 103 | + moved, err := migrateProjectReviewSettings(ctx, project) |
| 104 | + if err != nil { |
| 105 | + return err |
| 106 | + } |
| 107 | + if moved { |
| 108 | + fmt.Fprintln(out, "Moved review preferences from project settings to clone-local preferences.") |
| 109 | + } else { |
| 110 | + fmt.Fprintln(out, "Removed unused review keys from project settings; nothing to move.") |
| 111 | + } |
| 112 | + return nil |
| 113 | +} |
| 114 | + |
| 115 | +func loadProjectReviewSettings(ctx context.Context) (*projectReviewSettings, bool, error) { |
| 116 | + path, raw, exists, err := settings.LoadProjectRaw(ctx) |
| 117 | + if err != nil { |
| 118 | + return nil, false, fmt.Errorf("review migration: %w", err) |
| 119 | + } |
| 120 | + if !exists { |
| 121 | + return nil, false, nil |
| 122 | + } |
| 123 | + |
| 124 | + reviewRaw, hasReview := raw["review"] |
| 125 | + fixAgentRaw, hasFixAgent := raw["review_fix_agent"] |
| 126 | + if !hasReview && !hasFixAgent { |
| 127 | + return nil, false, nil |
| 128 | + } |
| 129 | + return &projectReviewSettings{ |
| 130 | + path: path, |
| 131 | + raw: raw, |
| 132 | + review: reviewRaw, |
| 133 | + fixAgent: fixAgentRaw, |
| 134 | + hasReview: hasReview, |
| 135 | + hasFixAgent: hasFixAgent, |
| 136 | + }, true, nil |
| 137 | +} |
| 138 | + |
| 139 | +// migrateProjectReviewSettings copies review keys from the project settings |
| 140 | +// file into clone-local preferences and strips them from the project file. |
| 141 | +// |
| 142 | +// Returns moved=true when any review data was copied into prefs. When the |
| 143 | +// project file's review keys are empty/null (or fully conflict with existing |
| 144 | +// prefs, which is rejected upstream), moved=false but the project keys are |
| 145 | +// still stripped as cleanup. |
| 146 | +// |
| 147 | +// Write ordering: prefs are saved first (atomic), then the project file is |
| 148 | +// rewritten (atomic). Both writes use temp-then-rename so a crash mid-write |
| 149 | +// leaves the original file intact rather than truncated. If the project |
| 150 | +// rewrite fails after the prefs write succeeded, prefs precedence covers |
| 151 | +// the gap until the next run. |
| 152 | +func migrateProjectReviewSettings(ctx context.Context, project *projectReviewSettings) (moved bool, err error) { |
| 153 | + if project == nil { |
| 154 | + return false, nil |
| 155 | + } |
| 156 | + |
| 157 | + prefs, err := settings.LoadClonePreferences(ctx) |
| 158 | + if err != nil { |
| 159 | + return false, fmt.Errorf("load review preferences for migration: %w", err) |
| 160 | + } |
| 161 | + if prefs == nil { |
| 162 | + prefs = &settings.ClonePreferences{} |
| 163 | + } |
| 164 | + |
| 165 | + preferencesChanged := false |
| 166 | + if project.hasReview && !isJSONNull(project.review) { |
| 167 | + var projectReview map[string]settings.ReviewConfig |
| 168 | + if err := json.Unmarshal(project.review, &projectReview); err != nil { |
| 169 | + return false, fmt.Errorf("parsing project review settings: %w", err) |
| 170 | + } |
| 171 | + if len(projectReview) > 0 { |
| 172 | + merged, mergedOK, conflicts := mergeProjectReviewIntoPrefs(prefs.Review, projectReview) |
| 173 | + if len(conflicts) > 0 { |
| 174 | + return false, fmt.Errorf( |
| 175 | + "review settings exist in both %s and clone-local preferences for agent(s) %v; "+ |
| 176 | + "reconcile manually by removing the redundant keys from %s, then re-run `entire review`", |
| 177 | + project.path, conflicts, project.path, |
| 178 | + ) |
| 179 | + } |
| 180 | + if mergedOK { |
| 181 | + prefs.Review = merged |
| 182 | + preferencesChanged = true |
| 183 | + } |
| 184 | + } |
| 185 | + } |
| 186 | + if project.hasFixAgent && !isJSONNull(project.fixAgent) { |
| 187 | + var fixAgent string |
| 188 | + if err := json.Unmarshal(project.fixAgent, &fixAgent); err != nil { |
| 189 | + return false, fmt.Errorf("parsing project review_fix_agent: %w", err) |
| 190 | + } |
| 191 | + if fixAgent != "" { |
| 192 | + if prefs.ReviewFixAgent != "" && prefs.ReviewFixAgent != fixAgent { |
| 193 | + return false, fmt.Errorf( |
| 194 | + "review_fix_agent differs between %s (%q) and clone-local preferences (%q); "+ |
| 195 | + "reconcile manually by removing review_fix_agent from %s, then re-run `entire review`", |
| 196 | + project.path, fixAgent, prefs.ReviewFixAgent, project.path, |
| 197 | + ) |
| 198 | + } |
| 199 | + if prefs.ReviewFixAgent == "" { |
| 200 | + prefs.ReviewFixAgent = fixAgent |
| 201 | + preferencesChanged = true |
| 202 | + } |
| 203 | + } |
| 204 | + } |
| 205 | + |
| 206 | + if preferencesChanged { |
| 207 | + if err := settings.SaveClonePreferences(ctx, prefs); err != nil { |
| 208 | + return false, fmt.Errorf("save review preferences for migration: %w", err) |
| 209 | + } |
| 210 | + } |
| 211 | + |
| 212 | + delete(project.raw, "review") |
| 213 | + delete(project.raw, "review_fix_agent") |
| 214 | + if err := settings.SaveProjectRaw(project.path, project.raw); err != nil { |
| 215 | + return false, fmt.Errorf("save project settings after review migration: %w", err) |
| 216 | + } |
| 217 | + return preferencesChanged, nil |
| 218 | +} |
| 219 | + |
| 220 | +// mergeProjectReviewIntoPrefs merges projectReview into the current prefs map. |
| 221 | +// Per-agent conflicts (same key, different value) are surfaced rather than |
| 222 | +// silently resolved — the caller can then refuse the migration with a clear |
| 223 | +// message. Non-overlapping entries are merged. Returns ok=false when nothing |
| 224 | +// would change (prefs already had every project entry verbatim). |
| 225 | +func mergeProjectReviewIntoPrefs(prefs, projectReview map[string]settings.ReviewConfig) (merged map[string]settings.ReviewConfig, ok bool, conflicts []string) { |
| 226 | + merged = make(map[string]settings.ReviewConfig, len(prefs)+len(projectReview)) |
| 227 | + for k, v := range prefs { |
| 228 | + merged[k] = v |
| 229 | + } |
| 230 | + changed := false |
| 231 | + for k, projectV := range projectReview { |
| 232 | + if existing, present := merged[k]; present { |
| 233 | + if !reviewConfigEqual(existing, projectV) { |
| 234 | + conflicts = append(conflicts, k) |
| 235 | + } |
| 236 | + continue |
| 237 | + } |
| 238 | + merged[k] = projectV |
| 239 | + changed = true |
| 240 | + } |
| 241 | + if len(conflicts) > 0 { |
| 242 | + return nil, false, conflicts |
| 243 | + } |
| 244 | + return merged, changed, nil |
| 245 | +} |
| 246 | + |
| 247 | +func reviewConfigEqual(a, b settings.ReviewConfig) bool { |
| 248 | + if a.Prompt != b.Prompt { |
| 249 | + return false |
| 250 | + } |
| 251 | + if len(a.Skills) != len(b.Skills) { |
| 252 | + return false |
| 253 | + } |
| 254 | + for i := range a.Skills { |
| 255 | + if a.Skills[i] != b.Skills[i] { |
| 256 | + return false |
| 257 | + } |
| 258 | + } |
| 259 | + return true |
| 260 | +} |
| 261 | + |
| 262 | +func isJSONNull(raw json.RawMessage) bool { |
| 263 | + return bytes.Equal(bytes.TrimSpace(raw), []byte("null")) |
| 264 | +} |
| 265 | + |
| 266 | +// localSettingsHasReviewKeys reports whether .entire/settings.local.json |
| 267 | +// exists and contains either a "review" or "review_fix_agent" key. Both keys |
| 268 | +// override clone-local preferences via mergeJSON's wholesale-replace path, |
| 269 | +// so the migration must surface their presence rather than silently produce |
| 270 | +// a state where the migrated config never takes effect. |
| 271 | +// |
| 272 | +// Returns the absolute path of the local settings file too, so callers can |
| 273 | +// quote the exact location in the warning they show the user. |
| 274 | +func localSettingsHasReviewKeys(ctx context.Context) (has bool, path string, err error) { |
| 275 | + path, raw, exists, loadErr := settings.LoadLocalRaw(ctx) |
| 276 | + if loadErr != nil { |
| 277 | + return false, path, fmt.Errorf("local settings review-keys check: %w", loadErr) |
| 278 | + } |
| 279 | + if !exists { |
| 280 | + return false, path, nil |
| 281 | + } |
| 282 | + _, hasReview := raw["review"] |
| 283 | + _, hasFixAgent := raw["review_fix_agent"] |
| 284 | + return hasReview || hasFixAgent, path, nil |
| 285 | +} |
0 commit comments