blob: 11f897e55c124dd4d3babdaf0ca68c8d553f3b25 [file] [log] [blame]
Josh Bleecher Snyderf4047bb2025-05-05 23:02:56 +00001package codereview
Earl Lee2e463fb2025-04-17 11:22:22 -07002
3import (
4 "bytes"
5 "context"
6 "fmt"
7 "log/slog"
8 "os"
9 "os/exec"
10 "path/filepath"
Josh Bleecher Snyderc72ceb22025-05-05 23:30:15 +000011 "slices"
Earl Lee2e463fb2025-04-17 11:22:22 -070012 "strings"
Josh Bleecher Snyderf4047bb2025-05-05 23:02:56 +000013
14 "sketch.dev/claudetool"
Earl Lee2e463fb2025-04-17 11:22:22 -070015)
16
17// A CodeReviewer manages quality checks.
18type CodeReviewer struct {
19 repoRoot string
Philip Zeyliger49edc922025-05-14 09:45:45 -070020 sketchBaseRef string
Earl Lee2e463fb2025-04-17 11:22:22 -070021 initialStatus []fileStatus // git status of files at initial commit, absolute paths
22 reviewed []string // history of all commits which have been reviewed
23 initialWorktree string // git worktree at initial commit, absolute path
24}
25
Josh Bleecher Snyder9daa5182025-05-16 18:34:00 +000026func NewCodeReviewer(ctx context.Context, repoRoot, sketchBaseRef string) (*CodeReviewer, error) {
Earl Lee2e463fb2025-04-17 11:22:22 -070027 r := &CodeReviewer{
28 repoRoot: repoRoot,
Philip Zeyliger49edc922025-05-14 09:45:45 -070029 sketchBaseRef: sketchBaseRef,
Earl Lee2e463fb2025-04-17 11:22:22 -070030 }
31 if r.repoRoot == "" {
32 return nil, fmt.Errorf("NewCodeReviewer: repoRoot must be non-empty")
33 }
Philip Zeyliger49edc922025-05-14 09:45:45 -070034 if r.sketchBaseRef == "" {
35 return nil, fmt.Errorf("NewCodeReviewer: sketchBaseRef must be non-empty")
Earl Lee2e463fb2025-04-17 11:22:22 -070036 }
37 // Confirm that root is in fact the git repo root.
Josh Bleecher Snyderf4047bb2025-05-05 23:02:56 +000038 root, err := claudetool.FindRepoRoot(r.repoRoot)
Earl Lee2e463fb2025-04-17 11:22:22 -070039 if err != nil {
40 return nil, err
41 }
42 if root != r.repoRoot {
43 return nil, fmt.Errorf("NewCodeReviewer: repoRoot=%q but git repo root is %q", r.repoRoot, root)
44 }
45
46 // Get an initial list of dirty and untracked files.
47 // We'll filter them out later when deciding whether the worktree is clean.
48 status, err := r.repoStatus(ctx)
49 if err != nil {
50 return nil, err
51 }
52 r.initialStatus = status
53 return r, nil
54}
55
Josh Bleecher Snyderc72ceb22025-05-05 23:30:15 +000056// autoformat formats all files changed in HEAD.
Earl Lee2e463fb2025-04-17 11:22:22 -070057// It returns a list of all files that were formatted.
58// It is best-effort only.
Josh Bleecher Snyderc72ceb22025-05-05 23:30:15 +000059func (r *CodeReviewer) autoformat(ctx context.Context) []string {
60 // Refuse to format if initial commit is not an ancestor of HEAD
Philip Zeyliger49edc922025-05-14 09:45:45 -070061 err := r.requireHEADDescendantOfSketchBaseRef(ctx)
Josh Bleecher Snyderc72ceb22025-05-05 23:30:15 +000062 if err != nil {
63 slog.WarnContext(ctx, "CodeReviewer.Autoformat refusing to format", "err", err)
64 return nil
65 }
66
Earl Lee2e463fb2025-04-17 11:22:22 -070067 head, err := r.CurrentCommit(ctx)
68 if err != nil {
69 slog.WarnContext(ctx, "CodeReviewer.Autoformat unable to get current commit", "err", err)
70 return nil
71 }
72 parent, err := r.ResolveCommit(ctx, "HEAD^1")
73 if err != nil {
74 slog.WarnContext(ctx, "CodeReviewer.Autoformat unable to get parent commit", "err", err)
75 return nil
76 }
Earl Lee2e463fb2025-04-17 11:22:22 -070077 // Retrieve a list of all files changed
78 // TODO: instead of one git diff --name-only and then N --name-status, do one --name-status.
Philip Zeyliger49edc922025-05-14 09:45:45 -070079 changedFiles, err := r.changedFiles(ctx, r.sketchBaseRef, head)
Earl Lee2e463fb2025-04-17 11:22:22 -070080 if err != nil {
81 slog.WarnContext(ctx, "CodeReviewer.Autoformat unable to get changed files", "err", err)
82 return nil
83 }
84
85 // General strategy: For all changed files,
86 // run the strictest formatter that passes on the original version.
87 // TODO: add non-Go formatters?
88 // TODO: at a minimum, for common file types, ensure trailing newlines and maybe trim trailing whitespace per line?
89 var fmtFiles []string
90 for _, file := range changedFiles {
91 if !strings.HasSuffix(file, ".go") {
92 continue
93 }
94 fileStatus, err := r.gitFileStatus(ctx, file)
95 if err != nil {
96 slog.WarnContext(ctx, "CodeReviewer.Autoformat unable to get file status", "file", file, "err", err)
97 continue
98 }
99 if fileStatus == "D" { // deleted, nothing to format
100 continue
101 }
102 code, err := r.getFileContentAtCommit(ctx, file, head)
103 if err != nil {
104 slog.WarnContext(ctx, "CodeReviewer.Autoformat unable to get file content at head", "file", file, "err", err)
105 continue
106 }
Josh Bleecher Snyderf4047bb2025-05-05 23:02:56 +0000107 if claudetool.IsAutogeneratedGoFile(code) { // leave autogenerated files alone
Earl Lee2e463fb2025-04-17 11:22:22 -0700108 continue
109 }
110 onDisk, err := os.ReadFile(file)
111 if err != nil {
112 slog.WarnContext(ctx, "CodeReviewer.Autoformat unable to read file", "file", file, "err", err)
113 continue
114 }
115 if !bytes.Equal(code, onDisk) { // file has been modified since HEAD
116 slog.WarnContext(ctx, "CodeReviewer.Autoformat file modified since HEAD", "file", file, "err", err)
117 continue
118 }
119 var formatterToUse string
120 if fileStatus == "A" {
121 formatterToUse = "gofumpt" // newly added, so we can format how we please: use gofumpt
122 } else {
123 prev, err := r.getFileContentAtCommit(ctx, file, parent)
124 if err != nil {
125 slog.WarnContext(ctx, "CodeReviewer.Autoformat unable to get file content at parent", "file", file, "err", err)
126 continue
127 }
128 formatterToUse = r.pickFormatter(ctx, prev) // pick the strictest formatter that passes on the original version
129 }
130
131 // Apply the chosen formatter to the current file
132 newCode := r.runFormatter(ctx, formatterToUse, code)
133 if newCode == nil { // no changes made
134 continue
135 }
136 // write to disk
137 if err := os.WriteFile(file, newCode, 0o600); err != nil {
138 slog.WarnContext(ctx, "CodeReviewer.Autoformat unable to write formatted file", "file", file, "err", err)
139 continue
140 }
141 fmtFiles = append(fmtFiles, file)
142 }
143 return fmtFiles
144}
145
146// RequireNormalGitState checks that the git repo state is pretty normal.
147func (r *CodeReviewer) RequireNormalGitState(_ context.Context) error {
148 rebaseDirs := []string{"rebase-merge", "rebase-apply"}
149 for _, dir := range rebaseDirs {
150 _, err := os.Stat(filepath.Join(r.repoRoot, dir))
151 if err == nil {
152 return fmt.Errorf("git repo is not clean: rebase in progress")
153 }
154 }
155 filesReason := map[string]string{
156 "MERGE_HEAD": "merge is in progress",
157 "CHERRY_PICK_HEAD": "cherry-pick is in progress",
158 "REVERT_HEAD": "revert is in progress",
159 "BISECT_LOG": "bisect is in progress",
160 }
161 for file, reason := range filesReason {
162 _, err := os.Stat(filepath.Join(r.repoRoot, file))
163 if err == nil {
164 return fmt.Errorf("git repo is not clean: %s", reason)
165 }
166 }
167 return nil
168}
169
170func (r *CodeReviewer) RequireNoUncommittedChanges(ctx context.Context) error {
171 // Check that there are no uncommitted changes, whether staged or not.
172 // (Changes in r.initialStatus are OK, no other changes are.)
173 statuses, err := r.repoStatus(ctx)
174 if err != nil {
175 return fmt.Errorf("unable to get repo status: %w", err)
176 }
177 uncommitted := new(strings.Builder)
178 for _, status := range statuses {
179 if !r.initialStatusesContainFile(status.Path) {
180 fmt.Fprintf(uncommitted, "%s %s\n", status.Path, status.RawStatus)
181 }
182 }
183 if uncommitted.Len() > 0 {
Josh Bleecher Snyder83b2d352025-05-23 11:39:50 -0700184 return fmt.Errorf("uncommitted changes in repo, please commit relevant changes and revert/delete others:\n%s", uncommitted.String())
Earl Lee2e463fb2025-04-17 11:22:22 -0700185 }
186 return nil
187}
188
189func (r *CodeReviewer) initialStatusesContainFile(file string) bool {
190 for _, s := range r.initialStatus {
191 if s.Path == file {
192 return true
193 }
194 }
195 return false
196}
197
198type fileStatus struct {
199 Path string
200 RawStatus string // always 2 characters
201}
202
203func (r *CodeReviewer) repoStatus(ctx context.Context) ([]fileStatus, error) {
204 // Run git status --porcelain, split into lines
205 cmd := exec.CommandContext(ctx, "git", "status", "--porcelain")
206 cmd.Dir = r.repoRoot
207 out, err := cmd.CombinedOutput()
208 if err != nil {
209 return nil, fmt.Errorf("failed to run git status: %w\n%s", err, out)
210 }
211 var statuses []fileStatus
212 for line := range strings.Lines(string(out)) {
213 if len(line) == 0 {
214 continue
215 }
216 if len(line) < 3 {
217 return nil, fmt.Errorf("invalid status line: %s", line)
218 }
219 path := line[3:]
220 status := line[:2]
221 absPath := r.absPath(path)
222 statuses = append(statuses, fileStatus{Path: absPath, RawStatus: status})
223 }
224 return statuses, nil
225}
226
227// CurrentCommit retrieves the current git commit hash
228func (r *CodeReviewer) CurrentCommit(ctx context.Context) (string, error) {
229 return r.ResolveCommit(ctx, "HEAD")
230}
231
232func (r *CodeReviewer) ResolveCommit(ctx context.Context, ref string) (string, error) {
233 cmd := exec.CommandContext(ctx, "git", "rev-parse", ref)
234 cmd.Dir = r.repoRoot
235 out, err := cmd.CombinedOutput()
236 if err != nil {
237 return "", fmt.Errorf("failed to get current commit hash: %w\n%s", err, out)
238 }
239 return strings.TrimSpace(string(out)), nil
240}
241
242func (r *CodeReviewer) absPath(relPath string) string {
243 return filepath.Clean(filepath.Join(r.repoRoot, relPath))
244}
245
246// gitFileStatus returns the status of a file (A for added, M for modified, D for deleted, etc.)
247func (r *CodeReviewer) gitFileStatus(ctx context.Context, file string) (string, error) {
Philip Zeyliger49edc922025-05-14 09:45:45 -0700248 cmd := exec.CommandContext(ctx, "git", "diff", "--name-status", r.sketchBaseRef, "HEAD", "--", file)
Earl Lee2e463fb2025-04-17 11:22:22 -0700249 cmd.Dir = r.repoRoot
250 out, err := cmd.CombinedOutput()
251 if err != nil {
252 return "", fmt.Errorf("failed to get file status: %w\n%s", err, out)
253 }
254 status := strings.TrimSpace(string(out))
255 if status == "" {
256 return "", fmt.Errorf("no status found for file: %s", file)
257 }
258 return string(status[0]), nil
259}
260
261// getFileContentAtCommit retrieves file content at a specific commit
262func (r *CodeReviewer) getFileContentAtCommit(ctx context.Context, file, commit string) ([]byte, error) {
263 relFile, err := filepath.Rel(r.repoRoot, file)
264 if err != nil {
265 slog.WarnContext(ctx, "CodeReviewer.getFileContentAtCommit: failed to get relative path", "repo_root", r.repoRoot, "file", file, "err", err)
266 file = relFile
267 }
268 cmd := exec.CommandContext(ctx, "git", "show", fmt.Sprintf("%s:%s", commit, relFile))
269 cmd.Dir = r.repoRoot
270 out, err := cmd.CombinedOutput()
271 if err != nil {
272 return nil, fmt.Errorf("failed to get file content at commit %s: %w\n%s", commit, err, out)
273 }
274 return out, nil
275}
276
277// runFormatter runs the specified formatter on a file and returns the results.
278// A nil result indicates that the file is unchanged, or that an error occurred.
279func (r *CodeReviewer) runFormatter(ctx context.Context, formatter string, content []byte) []byte {
280 if formatter == "" {
281 return nil // no formatter
282 }
283 // Run the formatter and capture the output
284 cmd := exec.CommandContext(ctx, formatter)
285 cmd.Dir = r.repoRoot
286 cmd.Stdin = bytes.NewReader(content)
287 out, err := cmd.CombinedOutput()
288 if err != nil {
289 // probably a parse error, err on the side of safety
290 return nil
291 }
292 if bytes.Equal(content, out) {
293 return nil // no changes
294 }
295 return out
296}
297
298// formatterWouldChange reports whether a formatter would make changes to the content.
299// If the contents are invalid, it returns false.
300// It works by piping the content to the formatter with the -l flag.
301func (r *CodeReviewer) formatterWouldChange(ctx context.Context, formatter string, content []byte) bool {
302 cmd := exec.CommandContext(ctx, formatter, "-l")
303 cmd.Dir = r.repoRoot
304 cmd.Stdin = bytes.NewReader(content)
305 out, err := cmd.CombinedOutput()
306 if err != nil {
307 // probably a parse error, err on the side of safety
308 return false
309 }
310
311 // If the output is empty, the file passes the formatter
312 // If the output contains "<standard input>", the file would be changed
313 return len(bytes.TrimSpace(out)) > 0
314}
315
316// pickFormatter picks a formatter to use for code.
317// If something goes wrong, it recommends no formatter (empty string).
318func (r *CodeReviewer) pickFormatter(ctx context.Context, code []byte) string {
319 // Test each formatter from strictest to least strict.
320 // Keep the first one that doesn't make changes.
321 formatters := []string{"gofumpt", "goimports", "gofmt"}
322 for _, formatter := range formatters {
323 if r.formatterWouldChange(ctx, formatter, code) {
324 continue
325 }
326 return formatter
327 }
328 return "" // no safe formatter found
329}
330
331// changedFiles retrieves a list of all files changed between two commits
332func (r *CodeReviewer) changedFiles(ctx context.Context, fromCommit, toCommit string) ([]string, error) {
333 cmd := exec.CommandContext(ctx, "git", "diff", "--name-only", fromCommit, toCommit)
334 cmd.Dir = r.repoRoot
335 out, err := cmd.CombinedOutput()
336 if err != nil {
337 return nil, fmt.Errorf("failed to get changed files: %w\n%s", err, out)
338 }
339 var files []string
340 for line := range strings.Lines(string(out)) {
341 line = strings.TrimSpace(line)
342 if len(line) == 0 {
343 continue
344 }
345 path := r.absPath(line)
346 if r.initialStatusesContainFile(path) {
347 continue
348 }
349 files = append(files, path)
350 }
351 return files, nil
352}
Josh Bleecher Snyderc72ceb22025-05-05 23:30:15 +0000353
Josh Bleecher Snyder1ed1cc42025-05-07 20:21:40 +0000354// ModTidy runs go mod tidy if go module files have changed.
355// Returns a list of files changed by go mod tidy (empty if none).
356func (r *CodeReviewer) ModTidy(ctx context.Context) ([]string, error) {
Philip Zeyliger49edc922025-05-14 09:45:45 -0700357 err := r.requireHEADDescendantOfSketchBaseRef(ctx)
Josh Bleecher Snyderc72ceb22025-05-05 23:30:15 +0000358 if err != nil {
Josh Bleecher Snyder1ed1cc42025-05-07 20:21:40 +0000359 return nil, fmt.Errorf("cannot run ModTidy: %w", err)
Josh Bleecher Snyderc72ceb22025-05-05 23:30:15 +0000360 }
361
Josh Bleecher Snyder1ed1cc42025-05-07 20:21:40 +0000362 // Check if any go.mod, go.sum, etc. files have changed
363 currentCommit, err := r.CurrentCommit(ctx)
364 if err != nil {
365 return nil, fmt.Errorf("failed to get current commit: %w", err)
366 }
367
Philip Zeyliger49edc922025-05-14 09:45:45 -0700368 changedFiles, err := r.changedFiles(ctx, r.sketchBaseRef, currentCommit)
Josh Bleecher Snyder1ed1cc42025-05-07 20:21:40 +0000369 if err != nil {
370 return nil, fmt.Errorf("failed to get changed files: %w", err)
371 }
372
373 // Check if any of the changed files are go module files
374 goModsChanged := false
375 for _, file := range changedFiles {
376 if isGoModFile(file) {
377 goModsChanged = true
378 break
379 }
380 }
381
382 if !goModsChanged {
383 // No go module files changed, so don't run go mod tidy
384 return nil, nil
385 }
386
387 // Run go mod tidy
Josh Bleecher Snyderc72ceb22025-05-05 23:30:15 +0000388 cmd := exec.CommandContext(ctx, "go", "mod", "tidy")
389 cmd.Dir = r.repoRoot
390 out, err := cmd.CombinedOutput()
391 if err != nil {
Josh Bleecher Snyder1ed1cc42025-05-07 20:21:40 +0000392 return nil, fmt.Errorf("go mod tidy failed: %w\n%s", err, out)
Josh Bleecher Snyderc72ceb22025-05-05 23:30:15 +0000393 }
394
Josh Bleecher Snyder1ed1cc42025-05-07 20:21:40 +0000395 // Check which files were changed by go mod tidy
396 statusCmd := exec.CommandContext(ctx, "git", "status", "--porcelain")
397 statusCmd.Dir = r.repoRoot
398 statusOut, err := statusCmd.CombinedOutput()
399 if err != nil {
400 return nil, fmt.Errorf("unable to get git status: %w", err)
401 }
402
403 var changedByTidy []string
404
405 for line := range strings.Lines(string(statusOut)) {
406 if len(line) <= 3 {
407 // empty line, defensiveness to avoid panics
408 continue
409 }
410 file := line[3:]
411 if !isGoModFile(file) {
412 continue
413 }
414 path := filepath.Join(r.repoRoot, file)
415 changedByTidy = append(changedByTidy, path)
416 }
417
418 return changedByTidy, nil
Josh Bleecher Snyderc72ceb22025-05-05 23:30:15 +0000419}
420
421// RunMechanicalChecks runs all mechanical checks and returns a message describing any changes made.
422func (r *CodeReviewer) RunMechanicalChecks(ctx context.Context) string {
423 var actions []string
424
425 changed := r.autoformat(ctx)
426 if len(changed) > 0 {
427 actions = append(actions, "autoformatters")
428 }
429
Josh Bleecher Snyder1ed1cc42025-05-07 20:21:40 +0000430 // Run go mod tidy (only if go module files have changed)
431 tidyChanges, err := r.ModTidy(ctx)
Josh Bleecher Snyderc72ceb22025-05-05 23:30:15 +0000432 if err != nil {
433 slog.WarnContext(ctx, "CodeReviewer.RunMechanicalChecks: ModTidy failed", "err", err)
Josh Bleecher Snyder1ed1cc42025-05-07 20:21:40 +0000434 }
435 if len(tidyChanges) > 0 {
436 changed = append(changed, tidyChanges...)
437 actions = append(actions, "`go mod tidy`")
Josh Bleecher Snyderc72ceb22025-05-05 23:30:15 +0000438 }
439
440 if len(changed) == 0 {
441 return ""
442 }
443
444 slices.Sort(changed)
445
446 msg := fmt.Sprintf(`I ran %s, which updated these files:
447
448%s
449
450Please amend your latest git commit with these changes and then continue with what you were doing.`,
451 strings.Join(actions, " and "),
452 strings.Join(changed, "\n"),
453 )
454
455 return msg
456}
Josh Bleecher Snyder1ed1cc42025-05-07 20:21:40 +0000457
458// isGoModFile returns true if the file is a Go module file (go.mod, go.sum, etc.)
459func isGoModFile(path string) bool {
460 basename := filepath.Base(path)
461 return strings.HasPrefix(basename, "go.")
462}