blob: 14dce0442c6ea715e8c67ca73f59f3b80c210ffa [file] [log] [blame]
Earl Lee2e463fb2025-04-17 11:22:22 -07001package claudetool
2
3import (
4 "bytes"
5 "context"
6 "encoding/json"
7 "fmt"
8 "io"
9 "log/slog"
10 "maps"
11 "os"
12 "os/exec"
13 "path/filepath"
14 "slices"
15 "strings"
16 "time"
17
18 "golang.org/x/tools/go/packages"
19 "sketch.dev/ant"
20)
21
22// This file does differential quality analysis of a commit relative to a base commit.
23
24// Tool returns a tool spec for a CodeReview tool backed by r.
25func (r *CodeReviewer) Tool() *ant.Tool {
26 spec := &ant.Tool{
27 Name: "codereview",
28 Description: `Run an automated code review.`,
29 // If you modify this, update the termui template for prettier rendering.
30 InputSchema: ant.MustSchema(`{"type": "object"}`),
31 Run: r.Run,
32 }
33 return spec
34}
35
36func (r *CodeReviewer) Run(ctx context.Context, m json.RawMessage) (string, error) {
37 if err := r.RequireNormalGitState(ctx); err != nil {
38 slog.DebugContext(ctx, "CodeReviewer.Run: failed to check for normal git state", "err", err)
39 return "", err
40 }
41 if err := r.RequireNoUncommittedChanges(ctx); err != nil {
42 slog.DebugContext(ctx, "CodeReviewer.Run: failed to check for uncommitted changes", "err", err)
43 return "", err
44 }
45
46 // Check that the current commit is not the initial commit
47 currentCommit, err := r.CurrentCommit(ctx)
48 if err != nil {
49 slog.DebugContext(ctx, "CodeReviewer.Run: failed to get current commit", "err", err)
50 return "", err
51 }
52 if r.IsInitialCommit(currentCommit) {
53 slog.DebugContext(ctx, "CodeReviewer.Run: current commit is initial commit, nothing to review")
54 return "", fmt.Errorf("no new commits have been added, nothing to review")
55 }
56
57 // No matter what failures happen from here out, we will declare this to have been reviewed.
58 // This should help avoid the model getting blocked by a broken code review tool.
59 r.reviewed = append(r.reviewed, currentCommit)
60
61 changedFiles, err := r.changedFiles(ctx, r.initialCommit, currentCommit)
62 if err != nil {
63 slog.DebugContext(ctx, "CodeReviewer.Run: failed to get changed files", "err", err)
64 return "", err
65 }
66
67 // Prepare to analyze before/after for the impacted files.
68 // We use the current commit to determine what packages exist and are impacted.
69 // The packages in the initial commit may be different.
70 // Good enough for now.
71 // TODO: do better
72 directPkgs, allPkgs, err := r.packagesForFiles(ctx, changedFiles)
73 if err != nil {
74 // TODO: log and skip to stuff that doesn't require packages
75 slog.DebugContext(ctx, "CodeReviewer.Run: failed to get packages for files", "err", err)
76 return "", err
77 }
78 allPkgList := slices.Collect(maps.Keys(allPkgs))
79 directPkgList := slices.Collect(maps.Keys(directPkgs))
80
81 var msgs []string
82
83 testMsg, err := r.checkTests(ctx, allPkgList)
84 if err != nil {
85 slog.DebugContext(ctx, "CodeReviewer.Run: failed to check tests", "err", err)
86 return "", err
87 }
88 if testMsg != "" {
89 msgs = append(msgs, testMsg)
90 }
91
92 vetMsg, err := r.checkVet(ctx, directPkgList)
93 if err != nil {
94 slog.DebugContext(ctx, "CodeReviewer.Run: failed to check vet", "err", err)
95 return "", err
96 }
97 if vetMsg != "" {
98 msgs = append(msgs, vetMsg)
99 }
100
101 goplsMsg, err := r.checkGopls(ctx, changedFiles)
102 if err != nil {
103 slog.DebugContext(ctx, "CodeReviewer.Run: failed to check gopls", "err", err)
104 return "", err
105 }
106 if goplsMsg != "" {
107 msgs = append(msgs, goplsMsg)
108 }
109
110 if len(msgs) == 0 {
111 slog.DebugContext(ctx, "CodeReviewer.Run: no issues found")
112 return "OK", nil
113 }
114 slog.DebugContext(ctx, "CodeReviewer.Run: found issues", "issues", msgs)
115 return strings.Join(msgs, "\n\n"), nil
116}
117
118func (r *CodeReviewer) initializeInitialCommitWorktree(ctx context.Context) error {
119 if r.initialWorktree != "" {
120 return nil
121 }
122 tmpDir, err := os.MkdirTemp("", "sketch-codereview-worktree")
123 if err != nil {
124 return err
125 }
126 worktreeCmd := exec.CommandContext(ctx, "git", "worktree", "add", "--detach", tmpDir, r.initialCommit)
127 worktreeCmd.Dir = r.repoRoot
128 out, err := worktreeCmd.CombinedOutput()
129 if err != nil {
130 return fmt.Errorf("unable to create worktree for initial commit: %w\n%s", err, out)
131 }
132 r.initialWorktree = tmpDir
133 return nil
134}
135
136func (r *CodeReviewer) checkTests(ctx context.Context, pkgList []string) (string, error) {
137 goTestArgs := []string{"test", "-json", "-v"}
138 goTestArgs = append(goTestArgs, pkgList...)
139
140 afterTestCmd := exec.CommandContext(ctx, "go", goTestArgs...)
141 afterTestCmd.Dir = r.repoRoot
142 afterTestOut, afterTestErr := afterTestCmd.Output()
143 if afterTestErr == nil {
144 return "", nil // all tests pass, we're good!
145 }
146
147 err := r.initializeInitialCommitWorktree(ctx)
148 if err != nil {
149 return "", err
150 }
151
152 beforeTestCmd := exec.CommandContext(ctx, "go", goTestArgs...)
153 beforeTestCmd.Dir = r.initialWorktree
154 beforeTestOut, _ := beforeTestCmd.Output() // ignore error, interesting info is in the output
155
156 // Parse the jsonl test results
157 beforeResults, beforeParseErr := parseTestResults(beforeTestOut)
158 if beforeParseErr != nil {
159 return "", fmt.Errorf("unable to parse test results for initial commit: %w\n%s", beforeParseErr, beforeTestOut)
160 }
161 afterResults, afterParseErr := parseTestResults(afterTestOut)
162 if afterParseErr != nil {
163 return "", fmt.Errorf("unable to parse test results for current commit: %w\n%s", afterParseErr, afterTestOut)
164 }
165
166 testRegressions, err := r.compareTestResults(beforeResults, afterResults)
167 if err != nil {
168 return "", fmt.Errorf("failed to compare test results: %w", err)
169 }
170 // TODO: better output formatting?
171 res := r.formatTestRegressions(testRegressions)
172 return res, nil
173}
174
175// VetIssue represents a single issue found by go vet
176type VetIssue struct {
177 Position string `json:"posn"`
178 Message string `json:"message"`
179 // Ignoring suggested_fixes for now as we don't need them for comparison
180}
181
182// VetResult represents the JSON output of go vet -json for a single package
183type VetResult map[string][]VetIssue // category -> issues
184
185// VetResults represents the full JSON output of go vet -json
186type VetResults map[string]VetResult // package path -> result
187
188// checkVet runs go vet on the provided packages in both the current and initial state,
189// compares the results, and reports any new vet issues introduced in the current state.
190func (r *CodeReviewer) checkVet(ctx context.Context, pkgList []string) (string, error) {
191 if len(pkgList) == 0 {
192 return "", nil // no packages to check
193 }
194
195 // Run vet on the current state with JSON output
196 goVetArgs := []string{"vet", "-json"}
197 goVetArgs = append(goVetArgs, pkgList...)
198
199 afterVetCmd := exec.CommandContext(ctx, "go", goVetArgs...)
200 afterVetCmd.Dir = r.repoRoot
201 afterVetOut, afterVetErr := afterVetCmd.CombinedOutput() // ignore error, we'll parse the output regar
202 if afterVetErr != nil {
203 slog.WarnContext(ctx, "CodeReviewer.checkVet: (after) go vet failed", "err", afterVetErr, "output", string(afterVetOut))
204 return "", nil // nothing more we can do here
205 }
206
207 // Parse the JSON output (even if vet returned an error, as it does when issues are found)
208 afterVetResults, err := parseVetJSON(afterVetOut)
209 if err != nil {
210 return "", fmt.Errorf("failed to parse vet output for current state: %w", err)
211 }
212
213 // If no issues were found, we're done
214 if len(afterVetResults) == 0 || !vetResultsHaveIssues(afterVetResults) {
215 return "", nil
216 }
217
218 // Vet detected issues in the current state, check if they existed in the initial state
219 err = r.initializeInitialCommitWorktree(ctx)
220 if err != nil {
221 return "", err
222 }
223
224 beforeVetCmd := exec.CommandContext(ctx, "go", goVetArgs...)
225 beforeVetCmd.Dir = r.initialWorktree
226 beforeVetOut, _ := beforeVetCmd.CombinedOutput() // ignore error, we'll parse the output anyway
227
228 // Parse the JSON output for the initial state
229 beforeVetResults, err := parseVetJSON(beforeVetOut)
230 if err != nil {
231 return "", fmt.Errorf("failed to parse vet output for initial state: %w", err)
232 }
233
234 // Find new issues that weren't present in the initial state
235 vetRegressions := findVetRegressions(beforeVetResults, afterVetResults)
236 if !vetResultsHaveIssues(vetRegressions) {
237 return "", nil // no new issues
238 }
239
240 // Format the results
241 return formatVetRegressions(vetRegressions), nil
242}
243
244// parseVetJSON parses the JSON output from go vet -json
245func parseVetJSON(output []byte) (VetResults, error) {
246 // The output contains multiple JSON objects, one per package
247 // We need to parse them separately
248 results := make(VetResults)
249
250 // Process the output by collecting JSON chunks between # comment lines
251 lines := strings.Split(string(output), "\n")
252 currentChunk := strings.Builder{}
253
254 // Helper function to process accumulated JSON chunks
255 processChunk := func() {
256 chunk := strings.TrimSpace(currentChunk.String())
257 if chunk == "" || !strings.HasPrefix(chunk, "{") {
258 return // Skip empty chunks or non-JSON chunks
259 }
260
261 // Try to parse the chunk as JSON
262 var result VetResults
263 if err := json.Unmarshal([]byte(chunk), &result); err != nil {
264 return // Skip invalid JSON
265 }
266
267 // Merge with our results
268 for pkg, issues := range result {
269 results[pkg] = issues
270 }
271
272 // Reset the chunk builder
273 currentChunk.Reset()
274 }
275
276 // Process lines
277 for _, line := range lines {
278 // If we hit a comment line, process the previous chunk and start a new one
279 if strings.HasPrefix(strings.TrimSpace(line), "#") {
280 processChunk()
281 continue
282 }
283
284 // Add the line to the current chunk
285 currentChunk.WriteString(line)
286 currentChunk.WriteString("\n")
287 }
288
289 // Process the final chunk
290 processChunk()
291
292 return results, nil
293}
294
295// vetResultsHaveIssues checks if there are any actual issues in the vet results
296func vetResultsHaveIssues(results VetResults) bool {
297 for _, pkgResult := range results {
298 for _, issues := range pkgResult {
299 if len(issues) > 0 {
300 return true
301 }
302 }
303 }
304 return false
305}
306
307// findVetRegressions identifies vet issues that are new in the after state
308func findVetRegressions(before, after VetResults) VetResults {
309 regressions := make(VetResults)
310
311 // Go through all packages in the after state
312 for pkgPath, afterPkgResults := range after {
313 beforePkgResults, pkgExistedBefore := before[pkgPath]
314
315 // Initialize package in regressions if it has issues
316 if !pkgExistedBefore {
317 // If the package didn't exist before, all issues are new
318 regressions[pkgPath] = afterPkgResults
319 continue
320 }
321
322 // Compare issues by category
323 for category, afterIssues := range afterPkgResults {
324 beforeIssues, categoryExistedBefore := beforePkgResults[category]
325
326 if !categoryExistedBefore {
327 // If this category didn't exist before, all issues are new
328 if regressions[pkgPath] == nil {
329 regressions[pkgPath] = make(VetResult)
330 }
331 regressions[pkgPath][category] = afterIssues
332 continue
333 }
334
335 // Compare individual issues
336 var newIssues []VetIssue
337 for _, afterIssue := range afterIssues {
338 if !issueExistsIn(afterIssue, beforeIssues) {
339 newIssues = append(newIssues, afterIssue)
340 }
341 }
342
343 // Add new issues to regressions
344 if len(newIssues) > 0 {
345 if regressions[pkgPath] == nil {
346 regressions[pkgPath] = make(VetResult)
347 }
348 regressions[pkgPath][category] = newIssues
349 }
350 }
351 }
352
353 return regressions
354}
355
356// issueExistsIn checks if an issue already exists in a list of issues
357// using a looser comparison that's resilient to position changes
358func issueExistsIn(issue VetIssue, issues []VetIssue) bool {
359 issueFile := extractFilePath(issue.Position)
360
361 for _, existing := range issues {
362 // Main comparison is by message content, which is likely stable
363 if issue.Message == existing.Message {
364 // If messages match exactly, consider it the same issue even if position changed
365 return true
366 }
367
368 // As a secondary check, if the issue is in the same file and has similar message,
369 // it's likely the same issue that might have been slightly reworded or relocated
370 existingFile := extractFilePath(existing.Position)
371 if issueFile == existingFile && messagesSimilar(issue.Message, existing.Message) {
372 return true
373 }
374 }
375 return false
376}
377
378// extractFilePath gets just the file path from a position string like "/path/to/file.go:10:15"
379func extractFilePath(position string) string {
380 parts := strings.Split(position, ":")
381 if len(parts) >= 1 {
382 return parts[0]
383 }
384 return position // fallback to the full position if we can't parse it
385}
386
387// messagesSimilar checks if two messages are similar enough to be considered the same issue
388// This is a simple implementation that could be enhanced with more sophisticated text comparison
389func messagesSimilar(msg1, msg2 string) bool {
390 // For now, simple similarity check: if one is a substring of the other
391 return strings.Contains(msg1, msg2) || strings.Contains(msg2, msg1)
392}
393
394// formatVetRegressions generates a human-readable summary of vet regressions
395func formatVetRegressions(regressions VetResults) string {
396 if !vetResultsHaveIssues(regressions) {
397 return ""
398 }
399
400 var sb strings.Builder
401 sb.WriteString("Go vet issues detected:\n\n")
402
403 // Get sorted list of packages for deterministic output
404 pkgPaths := make([]string, 0, len(regressions))
405 for pkgPath := range regressions {
406 pkgPaths = append(pkgPaths, pkgPath)
407 }
408 slices.Sort(pkgPaths)
409
410 issueCount := 1
411 for _, pkgPath := range pkgPaths {
412 pkgResult := regressions[pkgPath]
413
414 // Get sorted list of categories
415 categories := make([]string, 0, len(pkgResult))
416 for category := range pkgResult {
417 categories = append(categories, category)
418 }
419 slices.Sort(categories)
420
421 for _, category := range categories {
422 issues := pkgResult[category]
423
424 // Skip empty issue lists (shouldn't happen, but just in case)
425 if len(issues) == 0 {
426 continue
427 }
428
429 // Sort issues by position for deterministic output
430 slices.SortFunc(issues, func(a, b VetIssue) int {
431 return strings.Compare(a.Position, b.Position)
432 })
433
434 // Format each issue
435 for _, issue := range issues {
436 sb.WriteString(fmt.Sprintf("%d. [%s] %s: %s\n",
437 issueCount,
438 category,
439 issue.Position,
440 issue.Message))
441 issueCount++
442 }
443 }
444 }
445
446 sb.WriteString("\nPlease fix these issues before proceeding.")
447 return sb.String()
448}
449
450// GoplsIssue represents a single issue reported by gopls check
451type GoplsIssue struct {
452 Position string // File position in format "file:line:col-range"
453 Message string // Description of the issue
454}
455
456// checkGopls runs gopls check on the provided files in both the current and initial state,
457// compares the results, and reports any new issues introduced in the current state.
458func (r *CodeReviewer) checkGopls(ctx context.Context, changedFiles []string) (string, error) {
459 if len(changedFiles) == 0 {
460 return "", nil // no files to check
461 }
462
463 // Filter out non-Go files as gopls only works on Go files
464 // and verify they still exist (not deleted)
465 var goFiles []string
466 for _, file := range changedFiles {
467 if !strings.HasSuffix(file, ".go") {
468 continue // not a Go file
469 }
470
471 // Check if the file still exists (not deleted)
472 if _, err := os.Stat(file); os.IsNotExist(err) {
473 continue // file doesn't exist anymore (deleted)
474 }
475
476 goFiles = append(goFiles, file)
477 }
478
479 if len(goFiles) == 0 {
480 return "", nil // no Go files to check
481 }
482
483 // Run gopls check on the current state
484 goplsArgs := append([]string{"check"}, goFiles...)
485
486 afterGoplsCmd := exec.CommandContext(ctx, "gopls", goplsArgs...)
487 afterGoplsCmd.Dir = r.repoRoot
488 afterGoplsOut, err := afterGoplsCmd.CombinedOutput() // gopls returns non-zero if it finds issues
489 if err != nil {
490 // Check if the output looks like real gopls issues or if it's just error output
491 if !looksLikeGoplsIssues(afterGoplsOut) {
492 slog.WarnContext(ctx, "CodeReviewer.checkGopls: gopls check failed to run properly", "err", err, "output", string(afterGoplsOut))
493 return "", nil // Skip rather than failing the entire code review
494 }
495 // Otherwise, proceed with parsing - it's likely just the non-zero exit code due to found issues
496 }
497
498 // Parse the output
499 afterIssues := parseGoplsOutput(afterGoplsOut)
500
501 // If no issues were found, we're done
502 if len(afterIssues) == 0 {
503 return "", nil
504 }
505
506 // Gopls detected issues in the current state, check if they existed in the initial state
507 initErr := r.initializeInitialCommitWorktree(ctx)
508 if initErr != nil {
509 return "", err
510 }
511
512 // For each file that exists in the initial commit, run gopls check
513 var initialFilesToCheck []string
514 for _, file := range goFiles {
515 // Get relative path for git operations
516 relFile, err := filepath.Rel(r.repoRoot, file)
517 if err != nil {
518 slog.WarnContext(ctx, "CodeReviewer.checkGopls: failed to get relative path", "repo_root", r.repoRoot, "file", file, "err", err)
519 continue
520 }
521
522 // Check if the file exists in the initial commit
523 checkCmd := exec.CommandContext(ctx, "git", "cat-file", "-e", fmt.Sprintf("%s:%s", r.initialCommit, relFile))
524 checkCmd.Dir = r.repoRoot
525 if err := checkCmd.Run(); err == nil {
526 // File exists in initial commit
527 initialFilePath := filepath.Join(r.initialWorktree, relFile)
528 initialFilesToCheck = append(initialFilesToCheck, initialFilePath)
529 }
530 }
531
532 // Run gopls check on the files that existed in the initial commit
533 beforeIssues := []GoplsIssue{}
534 if len(initialFilesToCheck) > 0 {
535 beforeGoplsArgs := append([]string{"check"}, initialFilesToCheck...)
536 beforeGoplsCmd := exec.CommandContext(ctx, "gopls", beforeGoplsArgs...)
537 beforeGoplsCmd.Dir = r.initialWorktree
538 var beforeGoplsOut []byte
539 var beforeCmdErr error
540 beforeGoplsOut, beforeCmdErr = beforeGoplsCmd.CombinedOutput()
541 if beforeCmdErr != nil && !looksLikeGoplsIssues(beforeGoplsOut) {
542 // If gopls fails to run properly on the initial commit, log a warning and continue
543 // with empty before issues - this will be conservative and report more issues
544 slog.WarnContext(ctx, "CodeReviewer.checkGopls: gopls check failed on initial commit",
545 "err", err, "output", string(beforeGoplsOut))
546 // Continue with empty beforeIssues
547 } else {
548 beforeIssues = parseGoplsOutput(beforeGoplsOut)
549 }
550 }
551
552 // Find new issues that weren't present in the initial state
553 goplsRegressions := findGoplsRegressions(beforeIssues, afterIssues)
554 if len(goplsRegressions) == 0 {
555 return "", nil // no new issues
556 }
557
558 // Format the results
559 return formatGoplsRegressions(goplsRegressions), nil
560}
561
562// parseGoplsOutput parses the text output from gopls check
563// Each line has the format: '/path/to/file.go:448:22-26: unused parameter: path'
564func parseGoplsOutput(output []byte) []GoplsIssue {
565 var issues []GoplsIssue
566 lines := strings.Split(string(output), "\n")
567
568 for _, line := range lines {
569 line = strings.TrimSpace(line)
570 if line == "" {
571 continue
572 }
573
574 // Skip lines that look like error messages rather than gopls issues
575 if strings.HasPrefix(line, "Error:") ||
576 strings.HasPrefix(line, "Failed:") ||
577 strings.HasPrefix(line, "Warning:") ||
578 strings.HasPrefix(line, "gopls:") {
579 continue
580 }
581
582 // Find the first colon that separates the file path from the line number
583 firstColonIdx := strings.Index(line, ":")
584 if firstColonIdx < 0 {
585 continue // Invalid format
586 }
587
588 // Verify the part before the first colon looks like a file path
589 potentialPath := line[:firstColonIdx]
590 if !strings.HasSuffix(potentialPath, ".go") {
591 continue // Not a Go file path
592 }
593
594 // Find the position of the first message separator ': '
595 // This separates the position info from the message
596 messageStart := strings.Index(line, ": ")
597 if messageStart < 0 || messageStart <= firstColonIdx {
598 continue // Invalid format
599 }
600
601 // Extract position and message
602 position := line[:messageStart]
603 message := line[messageStart+2:] // Skip the ': ' separator
604
605 // Verify position has the expected format (at least 2 colons for line:col)
606 colonCount := strings.Count(position, ":")
607 if colonCount < 2 {
608 continue // Not enough position information
609 }
610
611 issues = append(issues, GoplsIssue{
612 Position: position,
613 Message: message,
614 })
615 }
616
617 return issues
618}
619
620// looksLikeGoplsIssues checks if the output appears to be actual gopls issues
621// rather than error messages about gopls itself failing
622func looksLikeGoplsIssues(output []byte) bool {
623 // If output is empty, it's not valid issues
624 if len(output) == 0 {
625 return false
626 }
627
628 // Check if output has at least one line that looks like a gopls issue
629 // A gopls issue looks like: '/path/to/file.go:123:45-67: message'
630 lines := strings.Split(string(output), "\n")
631 for _, line := range lines {
632 line = strings.TrimSpace(line)
633 if line == "" {
634 continue
635 }
636
637 // A gopls issue has at least two colons (file path, line number, column)
638 // and contains a colon followed by a space (separating position from message)
639 colonCount := strings.Count(line, ":")
640 hasSeparator := strings.Contains(line, ": ")
641
642 if colonCount >= 2 && hasSeparator {
643 // Check if it starts with a likely file path (ending in .go)
644 parts := strings.SplitN(line, ":", 2)
645 if strings.HasSuffix(parts[0], ".go") {
646 return true
647 }
648 }
649 }
650 return false
651}
652
653// normalizeGoplsPosition extracts just the file path from a position string
654func normalizeGoplsPosition(position string) string {
655 // Extract just the file path by taking everything before the first colon
656 parts := strings.Split(position, ":")
657 if len(parts) < 1 {
658 return position
659 }
660 return parts[0]
661}
662
663// findGoplsRegressions identifies gopls issues that are new in the after state
664func findGoplsRegressions(before, after []GoplsIssue) []GoplsIssue {
665 var regressions []GoplsIssue
666
667 // Build map of before issues for easier lookup
668 beforeIssueMap := make(map[string]map[string]bool) // file -> message -> exists
669 for _, issue := range before {
670 file := normalizeGoplsPosition(issue.Position)
671 if _, exists := beforeIssueMap[file]; !exists {
672 beforeIssueMap[file] = make(map[string]bool)
673 }
674 // Store both the exact message and the general issue type for fuzzy matching
675 beforeIssueMap[file][issue.Message] = true
676
677 // Extract the general issue type (everything before the first ':' in the message)
678 generalIssue := issue.Message
679 if colonIdx := strings.Index(issue.Message, ":"); colonIdx > 0 {
680 generalIssue = issue.Message[:colonIdx]
681 }
682 beforeIssueMap[file][generalIssue] = true
683 }
684
685 // Check each after issue to see if it's new
686 for _, afterIssue := range after {
687 file := normalizeGoplsPosition(afterIssue.Position)
688 isNew := true
689
690 if fileIssues, fileExists := beforeIssueMap[file]; fileExists {
691 // Check for exact message match
692 if fileIssues[afterIssue.Message] {
693 isNew = false
694 } else {
695 // Check for general issue type match
696 generalIssue := afterIssue.Message
697 if colonIdx := strings.Index(afterIssue.Message, ":"); colonIdx > 0 {
698 generalIssue = afterIssue.Message[:colonIdx]
699 }
700 if fileIssues[generalIssue] {
701 isNew = false
702 }
703 }
704 }
705
706 if isNew {
707 regressions = append(regressions, afterIssue)
708 }
709 }
710
711 // Sort regressions for deterministic output
712 slices.SortFunc(regressions, func(a, b GoplsIssue) int {
713 return strings.Compare(a.Position, b.Position)
714 })
715
716 return regressions
717}
718
719// formatGoplsRegressions generates a human-readable summary of gopls check regressions
720func formatGoplsRegressions(regressions []GoplsIssue) string {
721 if len(regressions) == 0 {
722 return ""
723 }
724
725 var sb strings.Builder
726 sb.WriteString("Gopls check issues detected:\n\n")
727
728 // Format each issue
729 for i, issue := range regressions {
730 sb.WriteString(fmt.Sprintf("%d. %s: %s\n", i+1, issue.Position, issue.Message))
731 }
732
733 sb.WriteString("\nIMPORTANT: Only fix new gopls check issues in parts of the code that you have already edited. ")
734 sb.WriteString("Do not change existing code that was not part of your current edits.")
735 return sb.String()
736}
737
738func (r *CodeReviewer) HasReviewed(commit string) bool {
739 return slices.Contains(r.reviewed, commit)
740}
741
742func (r *CodeReviewer) IsInitialCommit(commit string) bool {
743 return commit == r.initialCommit
744}
745
746// packagesForFiles returns maps of packages related to the given files:
747// 1. directPkgs: packages that directly contain the changed files
748// 2. allPkgs: all packages that might be affected, including downstream packages that depend on the direct packages
749// It may include false positives.
750// Files must be absolute paths!
751func (r *CodeReviewer) packagesForFiles(ctx context.Context, files []string) (directPkgs, allPkgs map[string]*packages.Package, err error) {
752 for _, f := range files {
753 if !filepath.IsAbs(f) {
754 return nil, nil, fmt.Errorf("path %q is not absolute", f)
755 }
756 }
757 cfg := &packages.Config{
758 Mode: packages.LoadImports | packages.NeedEmbedFiles,
759 Context: ctx,
760 // Logf: func(msg string, args ...any) {
761 // slog.DebugContext(ctx, "loading go packages", "msg", fmt.Sprintf(msg, args...))
762 // },
763 // TODO: in theory, go.mod might not be in the repo root, and there might be multiple go.mod files.
764 // We can cross that bridge when we get there.
765 Dir: r.repoRoot,
766 Tests: true,
767 }
768 universe, err := packages.Load(cfg, "./...")
769 if err != nil {
770 return nil, nil, err
771 }
772 // Identify packages that directly contain the changed files
773 directPkgs = make(map[string]*packages.Package) // import path -> package
774 for _, pkg := range universe {
775 // fmt.Println("pkg:", pkg.PkgPath)
776 pkgFiles := allFiles(pkg)
777 // fmt.Println("pkgFiles:", pkgFiles)
778 for _, file := range files {
779 if pkgFiles[file] {
780 // prefer test packages, as they contain strictly more files (right?)
781 prev := directPkgs[pkg.PkgPath]
782 if prev == nil || prev.ForTest == "" {
783 directPkgs[pkg.PkgPath] = pkg
784 }
785 }
786 }
787 }
788
789 // Create a copy of directPkgs to expand with dependencies
790 allPkgs = make(map[string]*packages.Package)
791 for k, v := range directPkgs {
792 allPkgs[k] = v
793 }
794
795 // Add packages that depend on the direct packages
796 addDependentPackages(universe, allPkgs)
797 return directPkgs, allPkgs, nil
798}
799
800// allFiles returns all files that might be referenced by the package.
801// It may contain false positives.
802func allFiles(p *packages.Package) map[string]bool {
803 files := make(map[string]bool)
804 add := [][]string{p.GoFiles, p.CompiledGoFiles, p.OtherFiles, p.EmbedFiles, p.IgnoredFiles}
805 for _, extra := range add {
806 for _, file := range extra {
807 files[file] = true
808 }
809 }
810 return files
811}
812
813// addDependentPackages adds to pkgs all packages from universe
814// that directly or indirectly depend on any package already in pkgs.
815func addDependentPackages(universe []*packages.Package, pkgs map[string]*packages.Package) {
816 for {
817 changed := false
818 for _, p := range universe {
819 if _, ok := pkgs[p.PkgPath]; ok {
820 // already in pkgs
821 continue
822 }
823 for importPath := range p.Imports {
824 if _, ok := pkgs[importPath]; ok {
825 // imports a package dependent on pkgs, add it
826 pkgs[p.PkgPath] = p
827 changed = true
828 break
829 }
830 }
831 }
832 if !changed {
833 break
834 }
835 }
836}
837
838// testJSON is a union of BuildEvent and TestEvent
839type testJSON struct {
840 // TestEvent only:
841 // The Time field holds the time the event happened. It is conventionally omitted
842 // for cached test results.
843 Time time.Time `json:"Time"`
844 // BuildEvent only:
845 // The ImportPath field gives the package ID of the package being built.
846 // This matches the Package.ImportPath field of go list -json and the
847 // TestEvent.FailedBuild field of go test -json. Note that it does not
848 // match TestEvent.Package.
849 ImportPath string `json:"ImportPath"` // BuildEvent only
850 // TestEvent only:
851 // The Package field, if present, specifies the package being tested. When the
852 // go command runs parallel tests in -json mode, events from different tests are
853 // interlaced; the Package field allows readers to separate them.
854 Package string `json:"Package"`
855 // Action is used in both BuildEvent and TestEvent.
856 // It is the key to distinguishing between them.
857 // BuildEvent:
858 // build-output or build-fail
859 // TestEvent:
860 // start, run, pause, cont, pass, bench, fail, output, skip
861 Action string `json:"Action"`
862 // TestEvent only:
863 // The Test field, if present, specifies the test, example, or benchmark function
864 // that caused the event. Events for the overall package test do not set Test.
865 Test string `json:"Test"`
866 // TestEvent only:
867 // The Elapsed field is set for "pass" and "fail" events. It gives the time elapsed in seconds
868 // for the specific test or the overall package test that passed or failed.
869 Elapsed float64
870 // TestEvent:
871 // The Output field is set for Action == "output" and is a portion of the
872 // test's output (standard output and standard error merged together). The
873 // output is unmodified except that invalid UTF-8 output from a test is coerced
874 // into valid UTF-8 by use of replacement characters. With that one exception,
875 // the concatenation of the Output fields of all output events is the exact output
876 // of the test execution.
877 // BuildEvent:
878 // The Output field is set for Action == "build-output" and is a portion of
879 // the build's output. The concatenation of the Output fields of all output
880 // events is the exact output of the build. A single event may contain one
881 // or more lines of output and there may be more than one output event for
882 // a given ImportPath. This matches the definition of the TestEvent.Output
883 // field produced by go test -json.
884 Output string `json:"Output"`
885 // TestEvent only:
886 // The FailedBuild field is set for Action == "fail" if the test failure was caused
887 // by a build failure. It contains the package ID of the package that failed to
888 // build. This matches the ImportPath field of the "go list" output, as well as the
889 // BuildEvent.ImportPath field as emitted by "go build -json".
890 FailedBuild string `json:"FailedBuild"`
891}
892
893// parseTestResults converts test output in JSONL format into a slice of testJSON objects
894func parseTestResults(testOutput []byte) ([]testJSON, error) {
895 var results []testJSON
896 dec := json.NewDecoder(bytes.NewReader(testOutput))
897 for {
898 var event testJSON
899 if err := dec.Decode(&event); err != nil {
900 if err == io.EOF {
901 break
902 }
903 return nil, err
904 }
905 results = append(results, event)
906 }
907 return results, nil
908}
909
910// testStatus represents the status of a test in a given commit
911type testStatus int
912
913const (
914 testStatusUnknown testStatus = iota
915 testStatusPass
916 testStatusFail
917 testStatusBuildFail
918 testStatusSkip
919)
920
921// testInfo represents information about a specific test
922type testInfo struct {
923 Package string
924 Test string // empty for package tests
925}
926
927// String returns a human-readable string representation of the test
928func (t testInfo) String() string {
929 if t.Test == "" {
930 return t.Package
931 }
932 return fmt.Sprintf("%s.%s", t.Package, t.Test)
933}
934
935// testRegression represents a test that regressed between commits
936type testRegression struct {
937 Info testInfo
938 BeforeStatus testStatus
939 AfterStatus testStatus
940 Output string // failure output in the after state
941}
942
943// collectTestStatuses processes a slice of test events and returns a map of test statuses
944func collectTestStatuses(results []testJSON) map[testInfo]testStatus {
945 statuses := make(map[testInfo]testStatus)
946 failedBuilds := make(map[string]bool) // track packages with build failures
947 testOutputs := make(map[testInfo][]string) // collect output for failing tests
948
949 // First pass: identify build failures
950 for _, result := range results {
951 if result.Action == "fail" && result.FailedBuild != "" {
952 failedBuilds[result.FailedBuild] = true
953 }
954 }
955
956 // Second pass: collect test statuses
957 for _, result := range results {
958 info := testInfo{Package: result.Package, Test: result.Test}
959
960 // Skip output events for now, we'll process them in a separate pass
961 if result.Action == "output" {
962 if result.Test != "" { // only collect output for actual tests, not package messages
963 testOutputs[info] = append(testOutputs[info], result.Output)
964 }
965 continue
966 }
967
968 // Handle BuildEvent output
969 if result.Action == "build-fail" {
970 // Mark all tests in this package as build failures
971 for ti := range statuses {
972 if ti.Package == result.ImportPath {
973 statuses[ti] = testStatusBuildFail
974 }
975 }
976 continue
977 }
978
979 // Check if the package has a build failure
980 if _, hasBuildFailure := failedBuilds[result.Package]; hasBuildFailure {
981 statuses[info] = testStatusBuildFail
982 continue
983 }
984
985 // Handle test events
986 switch result.Action {
987 case "pass":
988 statuses[info] = testStatusPass
989 case "fail":
990 statuses[info] = testStatusFail
991 case "skip":
992 statuses[info] = testStatusSkip
993 }
994 }
995
996 return statuses
997}
998
999// compareTestResults identifies tests that have regressed between commits
1000func (r *CodeReviewer) compareTestResults(beforeResults, afterResults []testJSON) ([]testRegression, error) {
1001 beforeStatuses := collectTestStatuses(beforeResults)
1002 afterStatuses := collectTestStatuses(afterResults)
1003
1004 // Collect output for failing tests
1005 testOutputMap := make(map[testInfo]string)
1006 for _, result := range afterResults {
1007 if result.Action == "output" {
1008 info := testInfo{Package: result.Package, Test: result.Test}
1009 testOutputMap[info] += result.Output
1010 }
1011 }
1012
1013 var regressions []testRegression
1014
1015 // Look for tests that regressed
1016 for info, afterStatus := range afterStatuses {
1017 // Skip tests that are passing or skipped in the after state
1018 if afterStatus == testStatusPass || afterStatus == testStatusSkip {
1019 continue
1020 }
1021
1022 // Get the before status (default to unknown if not present)
1023 beforeStatus, exists := beforeStatuses[info]
1024 if !exists {
1025 beforeStatus = testStatusUnknown
1026 }
1027
1028 // Log warning if we encounter unexpected unknown status in the 'after' state
1029 if afterStatus == testStatusUnknown {
1030 slog.WarnContext(context.Background(), "Unexpected unknown test status encountered",
1031 "package", info.Package, "test", info.Test)
1032 }
1033
1034 // Check for regressions
1035 if isRegression(beforeStatus, afterStatus) {
1036 regressions = append(regressions, testRegression{
1037 Info: info,
1038 BeforeStatus: beforeStatus,
1039 AfterStatus: afterStatus,
1040 Output: testOutputMap[info],
1041 })
1042 }
1043 }
1044
1045 // Sort regressions for consistent output
1046 slices.SortFunc(regressions, func(a, b testRegression) int {
1047 // First by package
1048 if c := strings.Compare(a.Info.Package, b.Info.Package); c != 0 {
1049 return c
1050 }
1051 // Then by test name
1052 return strings.Compare(a.Info.Test, b.Info.Test)
1053 })
1054
1055 return regressions, nil
1056}
1057
1058// badnessLevels maps test status to a badness level
1059// Higher values indicate worse status (more severe issues)
1060var badnessLevels = map[testStatus]int{
1061 testStatusBuildFail: 4, // Worst
1062 testStatusFail: 3,
1063 testStatusSkip: 2,
1064 testStatusPass: 1,
1065 testStatusUnknown: 0, // Least bad - avoids false positives
1066}
1067
1068// isRegression determines if a test has regressed based on before and after status
1069// A regression is defined as an increase in badness level
1070func isRegression(before, after testStatus) bool {
1071 // Higher badness level means worse status
1072 return badnessLevels[after] > badnessLevels[before]
1073}
1074
1075// formatTestRegressions generates a human-readable summary of test regressions
1076func (r *CodeReviewer) formatTestRegressions(regressions []testRegression) string {
1077 if len(regressions) == 0 {
1078 return ""
1079 }
1080
1081 var sb strings.Builder
1082 sb.WriteString(fmt.Sprintf("Test regressions detected between initial commit (%s) and HEAD:\n\n", r.initialCommit))
1083
1084 for i, reg := range regressions {
1085 // Describe the regression
1086 sb.WriteString(fmt.Sprintf("%d. %s: ", i+1, reg.Info.String()))
1087
1088 switch {
1089 case reg.BeforeStatus == testStatusUnknown && reg.AfterStatus == testStatusFail:
1090 sb.WriteString("New test is failing")
1091 case reg.BeforeStatus == testStatusUnknown && reg.AfterStatus == testStatusBuildFail:
1092 sb.WriteString("New test has build errors")
1093 case reg.BeforeStatus == testStatusPass && reg.AfterStatus == testStatusFail:
1094 sb.WriteString("Was passing, now failing")
1095 case reg.BeforeStatus == testStatusPass && reg.AfterStatus == testStatusBuildFail:
1096 sb.WriteString("Was passing, now has build errors")
1097 case reg.BeforeStatus == testStatusSkip && reg.AfterStatus == testStatusFail:
1098 sb.WriteString("Was skipped, now failing")
1099 case reg.BeforeStatus == testStatusSkip && reg.AfterStatus == testStatusBuildFail:
1100 sb.WriteString("Was skipped, now has build errors")
1101 default:
1102 sb.WriteString("Regression detected")
1103 }
1104 sb.WriteString("\n")
1105
1106 // Add failure output with indentation for readability
1107 if reg.Output != "" {
1108 outputLines := strings.Split(strings.TrimSpace(reg.Output), "\n")
1109 // Limit output to first 10 lines to avoid overwhelming feedback
1110 shownLines := min(len(outputLines), 10)
1111
1112 sb.WriteString(" Output:\n")
1113 for _, line := range outputLines[:shownLines] {
1114 sb.WriteString(fmt.Sprintf(" | %s\n", line))
1115 }
1116 if shownLines < len(outputLines) {
1117 sb.WriteString(fmt.Sprintf(" | ... (%d more lines)\n", len(outputLines)-shownLines))
1118 }
1119 }
1120 sb.WriteString("\n")
1121 }
1122
1123 sb.WriteString("Please fix these test failures before proceeding.")
1124 return sb.String()
1125}