blob: 710a09d469f4a8f018b048f3b6822872ab08b11b [file] [log] [blame]
Earl Lee2e463fb2025-04-17 11:22:22 -07001package claudetool
2
3import (
4 "bytes"
Josh Bleecher Snyderffecb1e2025-04-28 18:59:14 +00005 "cmp"
Earl Lee2e463fb2025-04-17 11:22:22 -07006 "context"
7 "encoding/json"
8 "fmt"
9 "io"
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +000010 "io/fs"
Earl Lee2e463fb2025-04-17 11:22:22 -070011 "log/slog"
12 "maps"
13 "os"
14 "os/exec"
15 "path/filepath"
Josh Bleecher Snyderffecb1e2025-04-28 18:59:14 +000016 "runtime"
Earl Lee2e463fb2025-04-17 11:22:22 -070017 "slices"
18 "strings"
Josh Bleecher Snyderffecb1e2025-04-28 18:59:14 +000019 "sync"
Earl Lee2e463fb2025-04-17 11:22:22 -070020 "time"
21
22 "golang.org/x/tools/go/packages"
23 "sketch.dev/ant"
24)
25
26// This file does differential quality analysis of a commit relative to a base commit.
27
28// Tool returns a tool spec for a CodeReview tool backed by r.
29func (r *CodeReviewer) Tool() *ant.Tool {
30 spec := &ant.Tool{
31 Name: "codereview",
32 Description: `Run an automated code review.`,
33 // If you modify this, update the termui template for prettier rendering.
34 InputSchema: ant.MustSchema(`{"type": "object"}`),
35 Run: r.Run,
36 }
37 return spec
38}
39
40func (r *CodeReviewer) Run(ctx context.Context, m json.RawMessage) (string, error) {
41 if err := r.RequireNormalGitState(ctx); err != nil {
42 slog.DebugContext(ctx, "CodeReviewer.Run: failed to check for normal git state", "err", err)
43 return "", err
44 }
45 if err := r.RequireNoUncommittedChanges(ctx); err != nil {
46 slog.DebugContext(ctx, "CodeReviewer.Run: failed to check for uncommitted changes", "err", err)
47 return "", err
48 }
49
50 // Check that the current commit is not the initial commit
51 currentCommit, err := r.CurrentCommit(ctx)
52 if err != nil {
53 slog.DebugContext(ctx, "CodeReviewer.Run: failed to get current commit", "err", err)
54 return "", err
55 }
56 if r.IsInitialCommit(currentCommit) {
57 slog.DebugContext(ctx, "CodeReviewer.Run: current commit is initial commit, nothing to review")
58 return "", fmt.Errorf("no new commits have been added, nothing to review")
59 }
60
61 // No matter what failures happen from here out, we will declare this to have been reviewed.
62 // This should help avoid the model getting blocked by a broken code review tool.
63 r.reviewed = append(r.reviewed, currentCommit)
64
65 changedFiles, err := r.changedFiles(ctx, r.initialCommit, currentCommit)
66 if err != nil {
67 slog.DebugContext(ctx, "CodeReviewer.Run: failed to get changed files", "err", err)
68 return "", err
69 }
70
71 // Prepare to analyze before/after for the impacted files.
72 // We use the current commit to determine what packages exist and are impacted.
73 // The packages in the initial commit may be different.
74 // Good enough for now.
75 // TODO: do better
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +000076 allPkgs, err := r.packagesForFiles(ctx, changedFiles)
Earl Lee2e463fb2025-04-17 11:22:22 -070077 if err != nil {
78 // TODO: log and skip to stuff that doesn't require packages
79 slog.DebugContext(ctx, "CodeReviewer.Run: failed to get packages for files", "err", err)
80 return "", err
81 }
82 allPkgList := slices.Collect(maps.Keys(allPkgs))
Earl Lee2e463fb2025-04-17 11:22:22 -070083
Josh Bleecher Snyderffecb1e2025-04-28 18:59:14 +000084 var errorMessages []string // problems we want the model to address
85 var infoMessages []string // info the model should consider
86
87 // Find potentially related files that should also be considered
88 // TODO: add some caching here, since this depends only on the initial commit and the changed files, not the details of the changes
89 relatedFiles, err := r.findRelatedFiles(ctx, changedFiles)
90 if err != nil {
91 slog.DebugContext(ctx, "CodeReviewer.Run: failed to find related files", "err", err)
92 } else {
93 relatedMsg := r.formatRelatedFiles(relatedFiles)
94 if relatedMsg != "" {
95 infoMessages = append(infoMessages, relatedMsg)
96 }
97 }
Earl Lee2e463fb2025-04-17 11:22:22 -070098
99 testMsg, err := r.checkTests(ctx, allPkgList)
100 if err != nil {
101 slog.DebugContext(ctx, "CodeReviewer.Run: failed to check tests", "err", err)
102 return "", err
103 }
104 if testMsg != "" {
Josh Bleecher Snyderffecb1e2025-04-28 18:59:14 +0000105 errorMessages = append(errorMessages, testMsg)
Earl Lee2e463fb2025-04-17 11:22:22 -0700106 }
107
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000108 goplsMsg, err := r.checkGopls(ctx, changedFiles) // includes vet checks
Earl Lee2e463fb2025-04-17 11:22:22 -0700109 if err != nil {
110 slog.DebugContext(ctx, "CodeReviewer.Run: failed to check gopls", "err", err)
111 return "", err
112 }
113 if goplsMsg != "" {
Josh Bleecher Snyderffecb1e2025-04-28 18:59:14 +0000114 errorMessages = append(errorMessages, goplsMsg)
Earl Lee2e463fb2025-04-17 11:22:22 -0700115 }
116
Josh Bleecher Snyderffecb1e2025-04-28 18:59:14 +0000117 buf := new(strings.Builder)
118 if len(infoMessages) > 0 {
119 buf.WriteString("# Info\n\n")
120 buf.WriteString(strings.Join(infoMessages, "\n\n"))
121 buf.WriteString("\n\n")
Earl Lee2e463fb2025-04-17 11:22:22 -0700122 }
Josh Bleecher Snyderffecb1e2025-04-28 18:59:14 +0000123 if len(errorMessages) > 0 {
124 buf.WriteString("# Errors\n\n")
125 buf.WriteString(strings.Join(errorMessages, "\n\n"))
126 buf.WriteString("\n\nPlease fix before proceeding.\n")
127 }
128 if buf.Len() == 0 {
129 buf.WriteString("OK")
130 }
131 return buf.String(), nil
Earl Lee2e463fb2025-04-17 11:22:22 -0700132}
133
134func (r *CodeReviewer) initializeInitialCommitWorktree(ctx context.Context) error {
135 if r.initialWorktree != "" {
136 return nil
137 }
138 tmpDir, err := os.MkdirTemp("", "sketch-codereview-worktree")
139 if err != nil {
140 return err
141 }
142 worktreeCmd := exec.CommandContext(ctx, "git", "worktree", "add", "--detach", tmpDir, r.initialCommit)
143 worktreeCmd.Dir = r.repoRoot
144 out, err := worktreeCmd.CombinedOutput()
145 if err != nil {
146 return fmt.Errorf("unable to create worktree for initial commit: %w\n%s", err, out)
147 }
148 r.initialWorktree = tmpDir
149 return nil
150}
151
152func (r *CodeReviewer) checkTests(ctx context.Context, pkgList []string) (string, error) {
153 goTestArgs := []string{"test", "-json", "-v"}
154 goTestArgs = append(goTestArgs, pkgList...)
155
156 afterTestCmd := exec.CommandContext(ctx, "go", goTestArgs...)
157 afterTestCmd.Dir = r.repoRoot
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000158 afterTestOut, _ := afterTestCmd.Output()
159 // unfortunately, we can't short-circuit here even if all tests pass,
160 // because we need to check for skipped tests.
Earl Lee2e463fb2025-04-17 11:22:22 -0700161
162 err := r.initializeInitialCommitWorktree(ctx)
163 if err != nil {
164 return "", err
165 }
166
167 beforeTestCmd := exec.CommandContext(ctx, "go", goTestArgs...)
168 beforeTestCmd.Dir = r.initialWorktree
169 beforeTestOut, _ := beforeTestCmd.Output() // ignore error, interesting info is in the output
170
171 // Parse the jsonl test results
172 beforeResults, beforeParseErr := parseTestResults(beforeTestOut)
173 if beforeParseErr != nil {
174 return "", fmt.Errorf("unable to parse test results for initial commit: %w\n%s", beforeParseErr, beforeTestOut)
175 }
176 afterResults, afterParseErr := parseTestResults(afterTestOut)
177 if afterParseErr != nil {
178 return "", fmt.Errorf("unable to parse test results for current commit: %w\n%s", afterParseErr, afterTestOut)
179 }
Earl Lee2e463fb2025-04-17 11:22:22 -0700180 testRegressions, err := r.compareTestResults(beforeResults, afterResults)
181 if err != nil {
182 return "", fmt.Errorf("failed to compare test results: %w", err)
183 }
184 // TODO: better output formatting?
185 res := r.formatTestRegressions(testRegressions)
186 return res, nil
187}
188
Earl Lee2e463fb2025-04-17 11:22:22 -0700189// GoplsIssue represents a single issue reported by gopls check
190type GoplsIssue struct {
191 Position string // File position in format "file:line:col-range"
192 Message string // Description of the issue
193}
194
195// checkGopls runs gopls check on the provided files in both the current and initial state,
196// compares the results, and reports any new issues introduced in the current state.
197func (r *CodeReviewer) checkGopls(ctx context.Context, changedFiles []string) (string, error) {
198 if len(changedFiles) == 0 {
199 return "", nil // no files to check
200 }
201
202 // Filter out non-Go files as gopls only works on Go files
203 // and verify they still exist (not deleted)
204 var goFiles []string
205 for _, file := range changedFiles {
206 if !strings.HasSuffix(file, ".go") {
207 continue // not a Go file
208 }
209
210 // Check if the file still exists (not deleted)
211 if _, err := os.Stat(file); os.IsNotExist(err) {
212 continue // file doesn't exist anymore (deleted)
213 }
214
215 goFiles = append(goFiles, file)
216 }
217
218 if len(goFiles) == 0 {
219 return "", nil // no Go files to check
220 }
221
222 // Run gopls check on the current state
223 goplsArgs := append([]string{"check"}, goFiles...)
224
225 afterGoplsCmd := exec.CommandContext(ctx, "gopls", goplsArgs...)
226 afterGoplsCmd.Dir = r.repoRoot
227 afterGoplsOut, err := afterGoplsCmd.CombinedOutput() // gopls returns non-zero if it finds issues
228 if err != nil {
229 // Check if the output looks like real gopls issues or if it's just error output
230 if !looksLikeGoplsIssues(afterGoplsOut) {
231 slog.WarnContext(ctx, "CodeReviewer.checkGopls: gopls check failed to run properly", "err", err, "output", string(afterGoplsOut))
232 return "", nil // Skip rather than failing the entire code review
233 }
234 // Otherwise, proceed with parsing - it's likely just the non-zero exit code due to found issues
235 }
236
237 // Parse the output
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000238 afterIssues := parseGoplsOutput(r.repoRoot, afterGoplsOut)
Earl Lee2e463fb2025-04-17 11:22:22 -0700239
240 // If no issues were found, we're done
241 if len(afterIssues) == 0 {
242 return "", nil
243 }
244
245 // Gopls detected issues in the current state, check if they existed in the initial state
246 initErr := r.initializeInitialCommitWorktree(ctx)
247 if initErr != nil {
248 return "", err
249 }
250
251 // For each file that exists in the initial commit, run gopls check
252 var initialFilesToCheck []string
253 for _, file := range goFiles {
254 // Get relative path for git operations
255 relFile, err := filepath.Rel(r.repoRoot, file)
256 if err != nil {
257 slog.WarnContext(ctx, "CodeReviewer.checkGopls: failed to get relative path", "repo_root", r.repoRoot, "file", file, "err", err)
258 continue
259 }
260
261 // Check if the file exists in the initial commit
262 checkCmd := exec.CommandContext(ctx, "git", "cat-file", "-e", fmt.Sprintf("%s:%s", r.initialCommit, relFile))
263 checkCmd.Dir = r.repoRoot
264 if err := checkCmd.Run(); err == nil {
265 // File exists in initial commit
266 initialFilePath := filepath.Join(r.initialWorktree, relFile)
267 initialFilesToCheck = append(initialFilesToCheck, initialFilePath)
268 }
269 }
270
271 // Run gopls check on the files that existed in the initial commit
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000272 var beforeIssues []GoplsIssue
Earl Lee2e463fb2025-04-17 11:22:22 -0700273 if len(initialFilesToCheck) > 0 {
274 beforeGoplsArgs := append([]string{"check"}, initialFilesToCheck...)
275 beforeGoplsCmd := exec.CommandContext(ctx, "gopls", beforeGoplsArgs...)
276 beforeGoplsCmd.Dir = r.initialWorktree
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000277 beforeGoplsOut, beforeCmdErr := beforeGoplsCmd.CombinedOutput()
Earl Lee2e463fb2025-04-17 11:22:22 -0700278 if beforeCmdErr != nil && !looksLikeGoplsIssues(beforeGoplsOut) {
279 // If gopls fails to run properly on the initial commit, log a warning and continue
280 // with empty before issues - this will be conservative and report more issues
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000281 slog.WarnContext(ctx, "CodeReviewer.checkGopls: gopls check failed on initial commit", "err", err, "output", string(beforeGoplsOut))
Earl Lee2e463fb2025-04-17 11:22:22 -0700282 } else {
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000283 beforeIssues = parseGoplsOutput(r.initialWorktree, beforeGoplsOut)
Earl Lee2e463fb2025-04-17 11:22:22 -0700284 }
285 }
286
287 // Find new issues that weren't present in the initial state
288 goplsRegressions := findGoplsRegressions(beforeIssues, afterIssues)
289 if len(goplsRegressions) == 0 {
290 return "", nil // no new issues
291 }
292
293 // Format the results
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000294 return r.formatGoplsRegressions(goplsRegressions), nil
Earl Lee2e463fb2025-04-17 11:22:22 -0700295}
296
297// parseGoplsOutput parses the text output from gopls check
298// Each line has the format: '/path/to/file.go:448:22-26: unused parameter: path'
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000299func parseGoplsOutput(root string, output []byte) []GoplsIssue {
Earl Lee2e463fb2025-04-17 11:22:22 -0700300 var issues []GoplsIssue
301 lines := strings.Split(string(output), "\n")
302
303 for _, line := range lines {
304 line = strings.TrimSpace(line)
305 if line == "" {
306 continue
307 }
308
309 // Skip lines that look like error messages rather than gopls issues
310 if strings.HasPrefix(line, "Error:") ||
311 strings.HasPrefix(line, "Failed:") ||
312 strings.HasPrefix(line, "Warning:") ||
313 strings.HasPrefix(line, "gopls:") {
314 continue
315 }
316
317 // Find the first colon that separates the file path from the line number
318 firstColonIdx := strings.Index(line, ":")
319 if firstColonIdx < 0 {
320 continue // Invalid format
321 }
322
323 // Verify the part before the first colon looks like a file path
324 potentialPath := line[:firstColonIdx]
325 if !strings.HasSuffix(potentialPath, ".go") {
326 continue // Not a Go file path
327 }
328
329 // Find the position of the first message separator ': '
330 // This separates the position info from the message
331 messageStart := strings.Index(line, ": ")
332 if messageStart < 0 || messageStart <= firstColonIdx {
333 continue // Invalid format
334 }
335
336 // Extract position and message
337 position := line[:messageStart]
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000338 rel, err := filepath.Rel(root, position)
339 if err == nil {
340 position = rel
341 }
Earl Lee2e463fb2025-04-17 11:22:22 -0700342 message := line[messageStart+2:] // Skip the ': ' separator
343
344 // Verify position has the expected format (at least 2 colons for line:col)
345 colonCount := strings.Count(position, ":")
346 if colonCount < 2 {
347 continue // Not enough position information
348 }
349
350 issues = append(issues, GoplsIssue{
351 Position: position,
352 Message: message,
353 })
354 }
355
356 return issues
357}
358
359// looksLikeGoplsIssues checks if the output appears to be actual gopls issues
360// rather than error messages about gopls itself failing
361func looksLikeGoplsIssues(output []byte) bool {
362 // If output is empty, it's not valid issues
363 if len(output) == 0 {
364 return false
365 }
366
367 // Check if output has at least one line that looks like a gopls issue
368 // A gopls issue looks like: '/path/to/file.go:123:45-67: message'
369 lines := strings.Split(string(output), "\n")
370 for _, line := range lines {
371 line = strings.TrimSpace(line)
372 if line == "" {
373 continue
374 }
375
376 // A gopls issue has at least two colons (file path, line number, column)
377 // and contains a colon followed by a space (separating position from message)
378 colonCount := strings.Count(line, ":")
379 hasSeparator := strings.Contains(line, ": ")
380
381 if colonCount >= 2 && hasSeparator {
382 // Check if it starts with a likely file path (ending in .go)
383 parts := strings.SplitN(line, ":", 2)
384 if strings.HasSuffix(parts[0], ".go") {
385 return true
386 }
387 }
388 }
389 return false
390}
391
392// normalizeGoplsPosition extracts just the file path from a position string
393func normalizeGoplsPosition(position string) string {
394 // Extract just the file path by taking everything before the first colon
395 parts := strings.Split(position, ":")
396 if len(parts) < 1 {
397 return position
398 }
399 return parts[0]
400}
401
402// findGoplsRegressions identifies gopls issues that are new in the after state
403func findGoplsRegressions(before, after []GoplsIssue) []GoplsIssue {
404 var regressions []GoplsIssue
405
406 // Build map of before issues for easier lookup
407 beforeIssueMap := make(map[string]map[string]bool) // file -> message -> exists
408 for _, issue := range before {
409 file := normalizeGoplsPosition(issue.Position)
410 if _, exists := beforeIssueMap[file]; !exists {
411 beforeIssueMap[file] = make(map[string]bool)
412 }
413 // Store both the exact message and the general issue type for fuzzy matching
414 beforeIssueMap[file][issue.Message] = true
415
416 // Extract the general issue type (everything before the first ':' in the message)
417 generalIssue := issue.Message
418 if colonIdx := strings.Index(issue.Message, ":"); colonIdx > 0 {
419 generalIssue = issue.Message[:colonIdx]
420 }
421 beforeIssueMap[file][generalIssue] = true
422 }
423
424 // Check each after issue to see if it's new
425 for _, afterIssue := range after {
426 file := normalizeGoplsPosition(afterIssue.Position)
427 isNew := true
428
429 if fileIssues, fileExists := beforeIssueMap[file]; fileExists {
430 // Check for exact message match
431 if fileIssues[afterIssue.Message] {
432 isNew = false
433 } else {
434 // Check for general issue type match
435 generalIssue := afterIssue.Message
436 if colonIdx := strings.Index(afterIssue.Message, ":"); colonIdx > 0 {
437 generalIssue = afterIssue.Message[:colonIdx]
438 }
439 if fileIssues[generalIssue] {
440 isNew = false
441 }
442 }
443 }
444
445 if isNew {
446 regressions = append(regressions, afterIssue)
447 }
448 }
449
450 // Sort regressions for deterministic output
451 slices.SortFunc(regressions, func(a, b GoplsIssue) int {
452 return strings.Compare(a.Position, b.Position)
453 })
454
455 return regressions
456}
457
458// formatGoplsRegressions generates a human-readable summary of gopls check regressions
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000459func (r *CodeReviewer) formatGoplsRegressions(regressions []GoplsIssue) string {
Earl Lee2e463fb2025-04-17 11:22:22 -0700460 if len(regressions) == 0 {
461 return ""
462 }
463
464 var sb strings.Builder
465 sb.WriteString("Gopls check issues detected:\n\n")
466
467 // Format each issue
468 for i, issue := range regressions {
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000469 sb.WriteString(fmt.Sprintf("%d. %s: %s\n", i+1, filepath.Join(r.repoRoot, issue.Position), issue.Message))
Earl Lee2e463fb2025-04-17 11:22:22 -0700470 }
471
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000472 sb.WriteString("\nIMPORTANT: Only fix new gopls check issues in parts of the code that you have already edited.")
473 sb.WriteString(" Do not change existing code that was not part of your current edits.\n")
Earl Lee2e463fb2025-04-17 11:22:22 -0700474 return sb.String()
475}
476
477func (r *CodeReviewer) HasReviewed(commit string) bool {
478 return slices.Contains(r.reviewed, commit)
479}
480
481func (r *CodeReviewer) IsInitialCommit(commit string) bool {
482 return commit == r.initialCommit
483}
484
485// packagesForFiles returns maps of packages related to the given files:
486// 1. directPkgs: packages that directly contain the changed files
487// 2. allPkgs: all packages that might be affected, including downstream packages that depend on the direct packages
488// It may include false positives.
489// Files must be absolute paths!
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000490func (r *CodeReviewer) packagesForFiles(ctx context.Context, files []string) (allPkgs map[string]*packages.Package, err error) {
Earl Lee2e463fb2025-04-17 11:22:22 -0700491 for _, f := range files {
492 if !filepath.IsAbs(f) {
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000493 return nil, fmt.Errorf("path %q is not absolute", f)
Earl Lee2e463fb2025-04-17 11:22:22 -0700494 }
495 }
496 cfg := &packages.Config{
497 Mode: packages.LoadImports | packages.NeedEmbedFiles,
498 Context: ctx,
499 // Logf: func(msg string, args ...any) {
500 // slog.DebugContext(ctx, "loading go packages", "msg", fmt.Sprintf(msg, args...))
501 // },
502 // TODO: in theory, go.mod might not be in the repo root, and there might be multiple go.mod files.
503 // We can cross that bridge when we get there.
504 Dir: r.repoRoot,
505 Tests: true,
506 }
507 universe, err := packages.Load(cfg, "./...")
508 if err != nil {
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000509 return nil, err
Earl Lee2e463fb2025-04-17 11:22:22 -0700510 }
511 // Identify packages that directly contain the changed files
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000512 directPkgs := make(map[string]*packages.Package) // import path -> package
Earl Lee2e463fb2025-04-17 11:22:22 -0700513 for _, pkg := range universe {
Earl Lee2e463fb2025-04-17 11:22:22 -0700514 pkgFiles := allFiles(pkg)
Earl Lee2e463fb2025-04-17 11:22:22 -0700515 for _, file := range files {
516 if pkgFiles[file] {
517 // prefer test packages, as they contain strictly more files (right?)
518 prev := directPkgs[pkg.PkgPath]
519 if prev == nil || prev.ForTest == "" {
520 directPkgs[pkg.PkgPath] = pkg
521 }
522 }
523 }
524 }
525
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000526 allPkgs = maps.Clone(directPkgs)
Earl Lee2e463fb2025-04-17 11:22:22 -0700527
528 // Add packages that depend on the direct packages
529 addDependentPackages(universe, allPkgs)
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000530 return allPkgs, nil
Earl Lee2e463fb2025-04-17 11:22:22 -0700531}
532
533// allFiles returns all files that might be referenced by the package.
534// It may contain false positives.
535func allFiles(p *packages.Package) map[string]bool {
536 files := make(map[string]bool)
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000537 // Add files from package info
Earl Lee2e463fb2025-04-17 11:22:22 -0700538 add := [][]string{p.GoFiles, p.CompiledGoFiles, p.OtherFiles, p.EmbedFiles, p.IgnoredFiles}
539 for _, extra := range add {
540 for _, file := range extra {
541 files[file] = true
542 }
543 }
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000544 // Add files from testdata directory
545 testdataDir := filepath.Join(p.Dir, "testdata")
546 if _, err := os.Stat(testdataDir); err == nil {
547 fsys := os.DirFS(p.Dir)
548 fs.WalkDir(fsys, "testdata", func(path string, d fs.DirEntry, err error) error {
549 if err == nil && !d.IsDir() {
550 files[filepath.Join(p.Dir, path)] = true
551 }
552 return nil
553 })
554 }
Earl Lee2e463fb2025-04-17 11:22:22 -0700555 return files
556}
557
558// addDependentPackages adds to pkgs all packages from universe
559// that directly or indirectly depend on any package already in pkgs.
560func addDependentPackages(universe []*packages.Package, pkgs map[string]*packages.Package) {
561 for {
562 changed := false
563 for _, p := range universe {
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000564 if strings.HasSuffix(p.PkgPath, ".test") { // ick, but I don't see another way
565 // skip test packages
566 continue
567 }
Earl Lee2e463fb2025-04-17 11:22:22 -0700568 if _, ok := pkgs[p.PkgPath]; ok {
569 // already in pkgs
570 continue
571 }
572 for importPath := range p.Imports {
573 if _, ok := pkgs[importPath]; ok {
574 // imports a package dependent on pkgs, add it
575 pkgs[p.PkgPath] = p
576 changed = true
577 break
578 }
579 }
580 }
581 if !changed {
582 break
583 }
584 }
585}
586
587// testJSON is a union of BuildEvent and TestEvent
588type testJSON struct {
589 // TestEvent only:
590 // The Time field holds the time the event happened. It is conventionally omitted
591 // for cached test results.
592 Time time.Time `json:"Time"`
593 // BuildEvent only:
594 // The ImportPath field gives the package ID of the package being built.
595 // This matches the Package.ImportPath field of go list -json and the
596 // TestEvent.FailedBuild field of go test -json. Note that it does not
597 // match TestEvent.Package.
598 ImportPath string `json:"ImportPath"` // BuildEvent only
599 // TestEvent only:
600 // The Package field, if present, specifies the package being tested. When the
601 // go command runs parallel tests in -json mode, events from different tests are
602 // interlaced; the Package field allows readers to separate them.
603 Package string `json:"Package"`
604 // Action is used in both BuildEvent and TestEvent.
605 // It is the key to distinguishing between them.
606 // BuildEvent:
607 // build-output or build-fail
608 // TestEvent:
609 // start, run, pause, cont, pass, bench, fail, output, skip
610 Action string `json:"Action"`
611 // TestEvent only:
612 // The Test field, if present, specifies the test, example, or benchmark function
613 // that caused the event. Events for the overall package test do not set Test.
614 Test string `json:"Test"`
615 // TestEvent only:
616 // The Elapsed field is set for "pass" and "fail" events. It gives the time elapsed in seconds
617 // for the specific test or the overall package test that passed or failed.
618 Elapsed float64
619 // TestEvent:
620 // The Output field is set for Action == "output" and is a portion of the
621 // test's output (standard output and standard error merged together). The
622 // output is unmodified except that invalid UTF-8 output from a test is coerced
623 // into valid UTF-8 by use of replacement characters. With that one exception,
624 // the concatenation of the Output fields of all output events is the exact output
625 // of the test execution.
626 // BuildEvent:
627 // The Output field is set for Action == "build-output" and is a portion of
628 // the build's output. The concatenation of the Output fields of all output
629 // events is the exact output of the build. A single event may contain one
630 // or more lines of output and there may be more than one output event for
631 // a given ImportPath. This matches the definition of the TestEvent.Output
632 // field produced by go test -json.
633 Output string `json:"Output"`
634 // TestEvent only:
635 // The FailedBuild field is set for Action == "fail" if the test failure was caused
636 // by a build failure. It contains the package ID of the package that failed to
637 // build. This matches the ImportPath field of the "go list" output, as well as the
638 // BuildEvent.ImportPath field as emitted by "go build -json".
639 FailedBuild string `json:"FailedBuild"`
640}
641
642// parseTestResults converts test output in JSONL format into a slice of testJSON objects
643func parseTestResults(testOutput []byte) ([]testJSON, error) {
644 var results []testJSON
645 dec := json.NewDecoder(bytes.NewReader(testOutput))
646 for {
647 var event testJSON
648 if err := dec.Decode(&event); err != nil {
649 if err == io.EOF {
650 break
651 }
652 return nil, err
653 }
654 results = append(results, event)
655 }
656 return results, nil
657}
658
659// testStatus represents the status of a test in a given commit
660type testStatus int
661
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000662//go:generate go tool stringer -type=testStatus -trimprefix=testStatus
Earl Lee2e463fb2025-04-17 11:22:22 -0700663const (
664 testStatusUnknown testStatus = iota
665 testStatusPass
666 testStatusFail
667 testStatusBuildFail
668 testStatusSkip
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000669 testStatusNoTests // no tests exist for this package
Earl Lee2e463fb2025-04-17 11:22:22 -0700670)
671
Earl Lee2e463fb2025-04-17 11:22:22 -0700672// testRegression represents a test that regressed between commits
673type testRegression struct {
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000674 Package string
675 Test string // empty for package tests
Earl Lee2e463fb2025-04-17 11:22:22 -0700676 BeforeStatus testStatus
677 AfterStatus testStatus
678 Output string // failure output in the after state
679}
680
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000681func (r *testRegression) Source() string {
682 if r.Test == "" {
683 return r.Package
Earl Lee2e463fb2025-04-17 11:22:22 -0700684 }
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000685 return fmt.Sprintf("%s.%s", r.Package, r.Test)
686}
Earl Lee2e463fb2025-04-17 11:22:22 -0700687
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000688type packageResult struct {
689 Status testStatus // overall status for the package
690 TestStatus map[string]testStatus // name -> status
691 TestOutput map[string][]string // name -> output parts
692}
Earl Lee2e463fb2025-04-17 11:22:22 -0700693
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000694// collectTestStatuses processes a slice of test events and returns rich status information
695func collectTestStatuses(results []testJSON) map[string]*packageResult {
696 m := make(map[string]*packageResult)
697
698 for _, event := range results {
699 pkg := event.Package
700 p, ok := m[pkg]
701 if !ok {
702 p = new(packageResult)
703 p.TestStatus = make(map[string]testStatus)
704 p.TestOutput = make(map[string][]string)
705 m[pkg] = p
Earl Lee2e463fb2025-04-17 11:22:22 -0700706 }
707
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000708 switch event.Action {
709 case "output":
710 p.TestOutput[event.Test] = append(p.TestOutput[event.Test], event.Output)
Earl Lee2e463fb2025-04-17 11:22:22 -0700711 continue
Earl Lee2e463fb2025-04-17 11:22:22 -0700712 case "pass":
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000713 if event.Test == "" {
714 p.Status = testStatusPass
715 } else {
716 p.TestStatus[event.Test] = testStatusPass
717 }
Earl Lee2e463fb2025-04-17 11:22:22 -0700718 case "fail":
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000719 if event.Test == "" {
720 if event.FailedBuild != "" {
721 p.Status = testStatusBuildFail
722 } else {
723 p.Status = testStatusFail
724 }
725 } else {
726 p.TestStatus[event.Test] = testStatusFail
727 }
Earl Lee2e463fb2025-04-17 11:22:22 -0700728 case "skip":
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000729 if event.Test == "" {
730 p.Status = testStatusNoTests
731 } else {
732 p.TestStatus[event.Test] = testStatusSkip
733 }
Earl Lee2e463fb2025-04-17 11:22:22 -0700734 }
735 }
736
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000737 return m
Earl Lee2e463fb2025-04-17 11:22:22 -0700738}
739
740// compareTestResults identifies tests that have regressed between commits
741func (r *CodeReviewer) compareTestResults(beforeResults, afterResults []testJSON) ([]testRegression, error) {
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000742 before := collectTestStatuses(beforeResults)
743 after := collectTestStatuses(afterResults)
744 var testLevelRegressions []testRegression
745 var packageLevelRegressions []testRegression
Earl Lee2e463fb2025-04-17 11:22:22 -0700746
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000747 afterPkgs := slices.Sorted(maps.Keys(after))
748 for _, pkg := range afterPkgs {
749 afterResult := after[pkg]
750 afterStatus := afterResult.Status
751 // Can't short-circuit here when tests are passing, because we need to check for skipped tests.
752 beforeResult, ok := before[pkg]
753 beforeStatus := testStatusNoTests
754 if ok {
755 beforeStatus = beforeResult.Status
756 }
757 // If things no longer build, stop at the package level.
758 // Otherwise, proceed to the test level, so we have more precise information.
759 if afterStatus == testStatusBuildFail && beforeStatus != testStatusBuildFail {
760 packageLevelRegressions = append(packageLevelRegressions, testRegression{
761 Package: pkg,
762 BeforeStatus: beforeStatus,
763 AfterStatus: afterStatus,
764 })
765 continue
766 }
767 tests := slices.Sorted(maps.Keys(afterResult.TestStatus))
768 for _, test := range tests {
769 afterStatus := afterResult.TestStatus[test]
770 switch afterStatus {
771 case testStatusPass:
772 continue
773 case testStatusUnknown:
774 slog.WarnContext(context.Background(), "unknown test status", "package", pkg, "test", test)
775 continue
776 }
777 beforeStatus := beforeResult.TestStatus[test]
778 if isRegression(beforeStatus, afterStatus) {
779 testLevelRegressions = append(testLevelRegressions, testRegression{
780 Package: pkg,
781 Test: test,
782 BeforeStatus: beforeStatus,
783 AfterStatus: afterStatus,
784 Output: strings.Join(afterResult.TestOutput[test], ""),
785 })
786 }
Earl Lee2e463fb2025-04-17 11:22:22 -0700787 }
788 }
789
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000790 // If we have test-level regressions, report only those
791 // Otherwise, report package-level regressions
Earl Lee2e463fb2025-04-17 11:22:22 -0700792 var regressions []testRegression
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000793 if len(testLevelRegressions) > 0 {
794 regressions = testLevelRegressions
795 } else {
796 regressions = packageLevelRegressions
Earl Lee2e463fb2025-04-17 11:22:22 -0700797 }
798
799 // Sort regressions for consistent output
800 slices.SortFunc(regressions, func(a, b testRegression) int {
801 // First by package
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000802 if c := strings.Compare(a.Package, b.Package); c != 0 {
Earl Lee2e463fb2025-04-17 11:22:22 -0700803 return c
804 }
805 // Then by test name
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000806 return strings.Compare(a.Test, b.Test)
Earl Lee2e463fb2025-04-17 11:22:22 -0700807 })
808
809 return regressions, nil
810}
811
812// badnessLevels maps test status to a badness level
813// Higher values indicate worse status (more severe issues)
814var badnessLevels = map[testStatus]int{
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000815 testStatusBuildFail: 5, // Worst
816 testStatusFail: 4,
817 testStatusSkip: 3,
818 testStatusNoTests: 2,
Earl Lee2e463fb2025-04-17 11:22:22 -0700819 testStatusPass: 1,
820 testStatusUnknown: 0, // Least bad - avoids false positives
821}
822
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000823// regressionFormatter defines a mapping of before/after state pairs to descriptive messages
824type regressionKey struct {
825 before, after testStatus
826}
827
828var regressionMessages = map[regressionKey]string{
829 {testStatusUnknown, testStatusBuildFail}: "New test has build/vet errors",
830 {testStatusUnknown, testStatusFail}: "New test is failing",
831 {testStatusUnknown, testStatusSkip}: "New test is skipped",
832 {testStatusPass, testStatusBuildFail}: "Was passing, now has build/vet errors",
833 {testStatusPass, testStatusFail}: "Was passing, now failing",
834 {testStatusPass, testStatusSkip}: "Was passing, now skipped",
835 {testStatusNoTests, testStatusBuildFail}: "Previously had no tests, now has build/vet errors",
836 {testStatusNoTests, testStatusFail}: "Previously had no tests, now has failing tests",
837 {testStatusNoTests, testStatusSkip}: "Previously had no tests, now has skipped tests",
838 {testStatusSkip, testStatusBuildFail}: "Was skipped, now has build/vet errors",
839 {testStatusSkip, testStatusFail}: "Was skipped, now failing",
840 {testStatusFail, testStatusBuildFail}: "Was failing, now has build/vet errors",
841}
842
Earl Lee2e463fb2025-04-17 11:22:22 -0700843// isRegression determines if a test has regressed based on before and after status
844// A regression is defined as an increase in badness level
845func isRegression(before, after testStatus) bool {
846 // Higher badness level means worse status
847 return badnessLevels[after] > badnessLevels[before]
848}
849
850// formatTestRegressions generates a human-readable summary of test regressions
851func (r *CodeReviewer) formatTestRegressions(regressions []testRegression) string {
852 if len(regressions) == 0 {
853 return ""
854 }
855
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000856 buf := new(strings.Builder)
857 fmt.Fprintf(buf, "Test regressions detected between initial commit (%s) and HEAD:\n\n", r.initialCommit)
Earl Lee2e463fb2025-04-17 11:22:22 -0700858
859 for i, reg := range regressions {
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000860 fmt.Fprintf(buf, "%d: %v: ", i+1, reg.Source())
861 key := regressionKey{reg.BeforeStatus, reg.AfterStatus}
862 message, exists := regressionMessages[key]
863 if !exists {
864 message = "Regression detected"
Earl Lee2e463fb2025-04-17 11:22:22 -0700865 }
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000866 fmt.Fprintf(buf, "%s\n", message)
Earl Lee2e463fb2025-04-17 11:22:22 -0700867 }
868
Josh Bleecher Snyder833a0f82025-04-24 18:39:36 +0000869 return buf.String()
Earl Lee2e463fb2025-04-17 11:22:22 -0700870}
Josh Bleecher Snyderffecb1e2025-04-28 18:59:14 +0000871
872// RelatedFile represents a file historically related to the changed files
873type RelatedFile struct {
874 Path string // Path to the file
875 Correlation float64 // Correlation score (0.0-1.0)
876}
877
878// findRelatedFiles identifies files that are historically related to the changed files
879// by analyzing git commit history for co-occurrences.
880func (r *CodeReviewer) findRelatedFiles(ctx context.Context, changedFiles []string) ([]RelatedFile, error) {
881 commits, err := r.getCommitsTouchingFiles(ctx, changedFiles)
882 if err != nil {
883 return nil, fmt.Errorf("failed to get commits touching files: %w", err)
884 }
885 if len(commits) == 0 {
886 return nil, nil
887 }
888
889 relChanged := make(map[string]bool, len(changedFiles))
890 for _, file := range changedFiles {
891 rel, err := filepath.Rel(r.repoRoot, file)
892 if err != nil {
893 return nil, fmt.Errorf("failed to get relative path for %s: %w", file, err)
894 }
895 relChanged[rel] = true
896 }
897
898 historyFiles := make(map[string]int)
899 var historyMu sync.Mutex
900
901 maxWorkers := runtime.GOMAXPROCS(0)
902 semaphore := make(chan bool, maxWorkers)
903 var wg sync.WaitGroup
904
905 for _, commit := range commits {
906 wg.Add(1)
907 semaphore <- true // acquire
908
909 go func(commit string) {
910 defer wg.Done()
911 defer func() { <-semaphore }() // release
912 commitFiles, err := r.getFilesInCommit(ctx, commit)
913 if err != nil {
914 slog.WarnContext(ctx, "Failed to get files in commit", "commit", commit, "err", err)
915 return
916 }
917 incr := 0
918 for _, file := range commitFiles {
919 if relChanged[file] {
920 incr++
921 }
922 }
923 if incr == 0 {
924 return
925 }
926 historyMu.Lock()
927 defer historyMu.Unlock()
928 for _, file := range commitFiles {
929 historyFiles[file] += incr
930 }
931 }(commit)
932 }
933 wg.Wait()
934
935 // normalize
936 maxCount := 0
937 for _, count := range historyFiles {
938 maxCount = max(maxCount, count)
939 }
940 if maxCount == 0 {
941 return nil, nil
942 }
943
944 var relatedFiles []RelatedFile
945 for file, count := range historyFiles {
946 if relChanged[file] {
947 // Don't include inputs in the output.
948 continue
949 }
950 correlation := float64(count) / float64(maxCount)
951 // Require min correlation to avoid noise
952 if correlation >= 0.1 {
953 relatedFiles = append(relatedFiles, RelatedFile{Path: file, Correlation: correlation})
954 }
955 }
956
957 // Highest correlation first
958 slices.SortFunc(relatedFiles, func(a, b RelatedFile) int {
959 return -1 * cmp.Compare(a.Correlation, b.Correlation)
960 })
961
962 // Limit to 1 correlated file per input file.
963 // (Arbitrary limit, to be adjusted.)
964 maxFiles := len(changedFiles)
965 if len(relatedFiles) > maxFiles {
966 relatedFiles = relatedFiles[:maxFiles]
967 }
968
969 // TODO: add an LLM in the mix here (like the keyword search tool) to do a filtering pass,
970 // and then increase the strength of the wording in the relatedFiles message.
971
972 return relatedFiles, nil
973}
974
975// getCommitsTouchingFiles returns all commits that touch any of the specified files
976func (r *CodeReviewer) getCommitsTouchingFiles(ctx context.Context, files []string) ([]string, error) {
977 if len(files) == 0 {
978 return nil, nil
979 }
980 fileArgs := append([]string{"rev-list", "--all", "--"}, files...)
981 cmd := exec.CommandContext(ctx, "git", fileArgs...)
982 cmd.Dir = r.repoRoot
983 out, err := cmd.CombinedOutput()
984 if err != nil {
985 return nil, fmt.Errorf("failed to get commits: %w\n%s", err, out)
986 }
987 return nonEmptyTrimmedLines(out), nil
988}
989
990// getFilesInCommit returns all files changed in a specific commit
991func (r *CodeReviewer) getFilesInCommit(ctx context.Context, commit string) ([]string, error) {
992 cmd := exec.CommandContext(ctx, "git", "diff-tree", "--no-commit-id", "--name-only", "-r", commit)
993 cmd.Dir = r.repoRoot
994 out, err := cmd.CombinedOutput()
995 if err != nil {
996 return nil, fmt.Errorf("failed to get files in commit: %w\n%s", err, out)
997 }
998 return nonEmptyTrimmedLines(out), nil
999}
1000
1001func nonEmptyTrimmedLines(b []byte) []string {
1002 var lines []string
1003 for line := range strings.Lines(string(b)) {
1004 line = strings.TrimSpace(line)
1005 if line != "" {
1006 lines = append(lines, line)
1007 }
1008 }
1009 return lines
1010}
1011
1012// formatRelatedFiles formats the related files list into a human-readable message
1013func (r *CodeReviewer) formatRelatedFiles(files []RelatedFile) string {
1014 if len(files) == 0 {
1015 return ""
1016 }
1017
1018 buf := new(strings.Builder)
1019
1020 fmt.Fprintf(buf, "Potentially related files:\n\n")
1021
1022 for _, file := range files {
1023 fmt.Fprintf(buf, "- %s (%0.0f%%)\n", file.Path, 100*file.Correlation)
1024 }
1025
1026 fmt.Fprintf(buf, "\nThese files have historically changed with the files you have modified. Consider whether they require updates as well.\n")
1027 return buf.String()
1028}