claudetool: add go mod tidy check to codereview
Co-Authored-By: sketch <hello@sketch.dev>
diff --git a/claudetool/codereview/codereview.go b/claudetool/codereview/codereview.go
index bf118ee..640d37d 100644
--- a/claudetool/codereview/codereview.go
+++ b/claudetool/codereview/codereview.go
@@ -8,6 +8,7 @@
"os"
"os/exec"
"path/filepath"
+ "slices"
"strings"
"sketch.dev/claudetool"
@@ -59,11 +60,17 @@
return r, nil
}
-// Autoformat formats all files changed in HEAD.
+// autoformat formats all files changed in HEAD.
// It returns a list of all files that were formatted.
// It is best-effort only.
-func (r *CodeReviewer) Autoformat(ctx context.Context) []string {
- // Refuse to format if HEAD == r.InitialCommit
+func (r *CodeReviewer) autoformat(ctx context.Context) []string {
+ // Refuse to format if initial commit is not an ancestor of HEAD
+ err := r.requireHEADDescendantOfInitialCommit(ctx)
+ if err != nil {
+ slog.WarnContext(ctx, "CodeReviewer.Autoformat refusing to format", "err", err)
+ return nil
+ }
+
head, err := r.CurrentCommit(ctx)
if err != nil {
slog.WarnContext(ctx, "CodeReviewer.Autoformat unable to get current commit", "err", err)
@@ -74,10 +81,6 @@
slog.WarnContext(ctx, "CodeReviewer.Autoformat unable to get parent commit", "err", err)
return nil
}
- if head == r.initialCommit {
- slog.WarnContext(ctx, "CodeReviewer.Autoformat refusing to format because HEAD == InitialCommit")
- return nil
- }
// Retrieve a list of all files changed
// TODO: instead of one git diff --name-only and then N --name-status, do one --name-status.
changedFiles, err := r.changedFiles(ctx, r.initialCommit, head)
@@ -354,3 +357,81 @@
}
return files, nil
}
+
+// ModTidy runs go mod tidy.
+func (r *CodeReviewer) ModTidy(ctx context.Context) error {
+ err := r.requireHEADDescendantOfInitialCommit(ctx)
+ if err != nil {
+ return fmt.Errorf("cannot run ModTidy: %w", err)
+ }
+
+ cmd := exec.CommandContext(ctx, "go", "mod", "tidy")
+ cmd.Dir = r.repoRoot
+ out, err := cmd.CombinedOutput()
+ if err != nil {
+ return fmt.Errorf("go mod tidy failed: %w\n%s", err, out)
+ }
+
+ return nil
+}
+
+// RunMechanicalChecks runs all mechanical checks and returns a message describing any changes made.
+func (r *CodeReviewer) RunMechanicalChecks(ctx context.Context) string {
+ var actions []string
+
+ changed := r.autoformat(ctx)
+ if len(changed) > 0 {
+ actions = append(actions, "autoformatters")
+ }
+
+ err := r.ModTidy(ctx)
+ if err != nil {
+ slog.WarnContext(ctx, "CodeReviewer.RunMechanicalChecks: ModTidy failed", "err", err)
+ } else {
+ // Figure out which files go mod tidy changed, best effort.
+ // TODO: if we knew the repo was clean going in, this would be easier.
+ statusCmd := exec.CommandContext(ctx, "git", "status", "--porcelain")
+ statusCmd.Dir = r.repoRoot
+ statusOut, err := statusCmd.CombinedOutput()
+ if err != nil {
+ slog.WarnContext(ctx, "CodeReviewer.RunMechanicalChecks: unable to get git status", "err", err)
+ return ""
+ }
+
+ madeChanges := false
+ for line := range strings.Lines(string(statusOut)) {
+ if len(line) <= 3 {
+ // empty line, defensiveness to avoid panics
+ continue
+ }
+ file := line[3:]
+ // TODO: this is a rough heuristic, revisit
+ if !strings.Contains(file, "go.") {
+ continue
+ }
+ path := filepath.Join(r.repoRoot, file)
+ changed = append(changed, path)
+ madeChanges = true
+ }
+ if madeChanges {
+ actions = append(actions, "`go mod tidy`")
+ }
+ }
+
+ if len(changed) == 0 {
+ return ""
+ }
+
+ slices.Sort(changed)
+
+ msg := fmt.Sprintf(`I ran %s, which updated these files:
+
+%s
+
+Please amend your latest git commit with these changes and then continue with what you were doing.`,
+ strings.Join(actions, " and "),
+ strings.Join(changed, "\n"),
+ )
+
+ return msg
+}
diff --git a/claudetool/codereview/codereview_test.go b/claudetool/codereview/codereview_test.go
index 29e05ec..aa7b8f1 100644
--- a/claudetool/codereview/codereview_test.go
+++ b/claudetool/codereview/codereview_test.go
@@ -272,7 +272,7 @@
if err != nil {
return nil, fmt.Errorf("error creating code reviewer: %w", err)
}
- formattedFiles := reviewer.Autoformat(ctx)
+ formattedFiles := reviewer.autoformat(ctx)
normalizedFiles := make([]string, len(formattedFiles))
for i, file := range formattedFiles {
normalizedFiles[i] = normalizePaths(file, dir)
diff --git a/claudetool/codereview/differential.go b/claudetool/codereview/differential.go
index 6aa8dfe..f358594 100644
--- a/claudetool/codereview/differential.go
+++ b/claudetool/codereview/differential.go
@@ -494,6 +494,30 @@
return commit == r.initialCommit
}
+// requireHEADDescendantOfInitialCommit returns an error if HEAD is not a descendant of r.initialCommit.
+// This serves two purposes:
+// - ensures we're not still on the initial commit
+// - ensures we're not on a separate branch or upstream somewhere, which would be confusing
+func (r *CodeReviewer) requireHEADDescendantOfInitialCommit(ctx context.Context) error {
+ head, err := r.CurrentCommit(ctx)
+ if err != nil {
+ return err
+ }
+
+ // Note: Git's merge-base --is-ancestor checks strict ancestry (i.e., <), so a commit is NOT an ancestor of itself.
+ cmd := exec.CommandContext(ctx, "git", "merge-base", "--is-ancestor", r.initialCommit, head)
+ cmd.Dir = r.repoRoot
+ err = cmd.Run()
+ if err != nil {
+ if exitErr, ok := err.(*exec.ExitError); ok && exitErr.ExitCode() == 1 {
+ // Exit code 1 means not an ancestor
+ return fmt.Errorf("HEAD is not a descendant of the initial commit")
+ }
+ return fmt.Errorf("failed to check whether initial commit is ancestor: %w", err)
+ }
+ return nil
+}
+
// packagesForFiles returns maps of packages related to the given files:
// 1. directPkgs: packages that directly contain the changed files
// 2. allPkgs: all packages that might be affected, including downstream packages that depend on the direct packages