claudetool: add experimental LLM reviewer stage
diff --git a/claudetool/codereview.go b/claudetool/codereview.go
index f305b4b..2a9f9ea 100644
--- a/claudetool/codereview.go
+++ b/claudetool/codereview.go
@@ -18,12 +18,19 @@
initialStatus []fileStatus // git status of files at initial commit, absolute paths
reviewed []string // history of all commits which have been reviewed
initialWorktree string // git worktree at initial commit, absolute path
+ llmReview bool // enables a subagent LLM review
}
-func NewCodeReviewer(ctx context.Context, repoRoot, initialCommit string) (*CodeReviewer, error) {
+const (
+ NoLLMReview = false
+ DoLLMReview = true
+)
+
+func NewCodeReviewer(ctx context.Context, repoRoot, initialCommit string, llmReview bool) (*CodeReviewer, error) {
r := &CodeReviewer{
repoRoot: repoRoot,
initialCommit: initialCommit,
+ llmReview: llmReview,
}
if r.repoRoot == "" {
return nil, fmt.Errorf("NewCodeReviewer: repoRoot must be non-empty")
diff --git a/claudetool/codereview_test.go b/claudetool/codereview_test.go
index 0d0c143..e189584 100644
--- a/claudetool/codereview_test.go
+++ b/claudetool/codereview_test.go
@@ -234,7 +234,7 @@
// Create a code reviewer for the repository
ctx := context.Background()
- reviewer, err := NewCodeReviewer(ctx, dir, initialCommit)
+ reviewer, err := NewCodeReviewer(ctx, dir, initialCommit, NoLLMReview)
if err != nil {
return "", fmt.Errorf("error creating code reviewer: %w", err)
}
@@ -268,7 +268,7 @@
return nil, fmt.Errorf("initial commit not set, cannot run autoformat")
}
ctx := context.Background()
- reviewer, err := NewCodeReviewer(ctx, dir, initialCommit)
+ reviewer, err := NewCodeReviewer(ctx, dir, initialCommit, NoLLMReview)
if err != nil {
return nil, fmt.Errorf("error creating code reviewer: %w", err)
}
diff --git a/claudetool/differential.go b/claudetool/differential.go
index d76209a..1d7ff6e 100644
--- a/claudetool/differential.go
+++ b/claudetool/differential.go
@@ -116,6 +116,17 @@
errorMessages = append(errorMessages, goplsMsg)
}
+ if r.llmReview {
+ llmComments, err := r.doLLMReview(ctx)
+ if err != nil {
+ // Log the error but don't fail the codereview if this check fails
+ slog.WarnContext(ctx, "CodeReviewer.Run: error doing LLM review", "err", err)
+ }
+ if llmComments != "" {
+ infoMessages = append(infoMessages, llmComments)
+ }
+ }
+
// NOTE: If you change this output format, update the corresponding UI parsing in:
// webui/src/web-components/sketch-tool-card.ts (SketchToolCardCodeReview.getStatusIcon)
buf := new(strings.Builder)
diff --git a/claudetool/llm_codereview_prompt.txt b/claudetool/llm_codereview_prompt.txt
new file mode 100644
index 0000000..920fc9f
--- /dev/null
+++ b/claudetool/llm_codereview_prompt.txt
@@ -0,0 +1,191 @@
+You are a code review assistant focused on identifying very specific issues.
+You analyze diffs and provide clear identification of problematic patterns.
+
+<issues>
+<issue>
+<name>Historical Comments</name>
+<description>
+Comments that describe past implementations or change history rather than explaining the current code's rationale or behavior. These are often more appropriate for commit messages than inline code comments. This also includes meta-comments that are only relevant to the current code review and wouldn't be useful to future code readers.
+</description>
+
+<identification_criteria>
+- References to past implementations (e.g., "The code used to do X", "Previously, we did Y")
+- Change history information (e.g., "This has changed", "This was modified", "Added support for")
+- Completed TODOs transformed into statements (e.g., "Implemented X", "Fixed Y")
+- Time-bound references (e.g., "As of May 2023", "In the new version")
+- Comments that won't make sense to future readers without knowledge of this specific change
+- Justifications meant only for reviewers (e.g., "Keeping as...", "Left this as...", "Moved this because...")
+- Comments explaining why something wasn't changed or was kept the same
+- Notes about implementation decisions that are only relevant during code review
+- Comments that reference the review process itself
+</identification_criteria>
+
+<exceptions>
+- Comments that explain why a particular approach was chosen over alternatives based on historical lessons
+- Comments that warn against specific approaches due to past failures
+- Comments that provide important context for why something unusual is necessary
+
+These exceptions must be written in a way that remains clear and useful to future readers without knowledge of the specific change history and should focus on the "why" behind decisions, not just documenting that a decision was made.
+</exceptions>
+
+<scope>ONLY newly added or modified comments in the diff. Do not flag existing comments or non-comment code changes.</scope>
+
+<examples>
+<example_input flag="true">
+- TODO: thread context through from caller
++ Threaded context through from caller.
+</example_input>
+<example_output>
+Historical comment: This comment only documents that a change was made, not why it matters. Consider moving to commit message.
+File: filepath/filename.ext
+Line: + Threaded context through from caller.
+</example_output>
+
+<example_input flag="true">
++ // We now use the new API for this operation
+</example_input>
+<example_output>
+Historical comment: References a change without explaining why it matters to current code readers. Consider moving to commit message.
+File: filepath/filename.ext
+Line: + // We now use the new API for this operation
+</example_output>
+
+<example_input flag="true">
++ ReadWriteTx TransactionType = "rw" // Keeping as string literal as this is a different type
+</example_input>
+<example_output>
+Historical comment: This comment is only relevant to the current code change. Consider moving to commit message or PR description.
+File: tx/tx.go
+Line: + ReadWriteTx TransactionType = "rw" // Keeping as string literal as this is a different type
+</example_output>
+
+<example_input flag="false">
++ // It is tempting to short-circuit here.
++ // However, in the past, that has created thundering herds and metastable failures,
++ // so just do all the work every time.
+</example_input>
+<example_output>
+</example_output>
+</examples>
+</issue>
+
+<issue>
+<name>String Iteration Patterns (Go 1.24+)</name>
+<description>
+Newly added code that iterates over lines in a string using traditional patterns like strings.Split or bufio.Scanner when strings.Lines could be used instead. Also covers other string splitting patterns where strings.SplitSeq would be more efficient.
+</description>
+
+<identification_criteria>
+For newline iteration patterns:
+- strings.Split with newline separator followed by iteration
+- bufio.Scanner with ScanLines split (explicit or implicit)
+- Any other pattern that processes lines one by one
+
+For general string splitting patterns:
+- strings.Split with non-newline separators
+</identification_criteria>
+<scope>ONLY newly added code in the diff. Do not flag existing code.</scope>
+<examples>
+<example_input flag="true">
++ lines := strings.Split(s, "\n")
++ for _, line := range lines {
++ // process line
++ }
+</example_input>
+<example_output>
+Line iteration pattern using strings.Split with iteration can be improved.
+
+Use strings.Lines instead:
+```go
+for line := range strings.Lines(s) {
+ // process line
+}
+```
+
+Important: strings.Lines preserves terminating newlines and handles empty strings correctly. The yielded lines include their terminating newlines unless the input doesn't end with one.
+
+File: filepath/filename.ext
+Line: + lines := strings.Split(s, "\n")
+ + for _, line := range lines {
+ + // process line
+ + }
+</example_output>
+<example_input flag="true">
++ scanner := bufio.NewScanner(strings.NewReader(text))
++ for scanner.Scan() {
++ line := scanner.Text()
++ // process line
++ }
+</example_input>
+<example_output>
+Line iteration using bufio.Scanner with ScanLines can be simplified.
+
+Use strings.Lines instead:
+```go
+for line := range strings.Lines(text) {
+ // process line
+}
+```
+
+Important: strings.Lines preserves terminating newlines and handles empty strings correctly. The yielded lines include their terminating newlines unless the input doesn't end with one.
+
+File: filepath/filename.ext
+Line: + scanner := bufio.NewScanner(strings.NewReader(text))
+ + scanner.Split(bufio.ScanLines)
+ + for scanner.Scan() {
+ + line := scanner.Text()
+ + // process line
+ + }
+</example_output>
+<example_input flag="true">
++ parts := strings.Split(data, ",")
++ for _, part := range parts {
++ // process part
++ }
+</example_input>
+<example_output>
+String splitting with iteration can be more efficient.
+
+Use strings.SplitSeq instead:
+```go
+for part := range strings.SplitSeq(data, ",") {
+ // process part
+}
+```
+
+Important: strings.SplitSeq creates an iterator without constructing the full slice, improving memory efficiency for large strings.
+
+File: filepath/filename.ext
+Line: + parts := strings.Split(data, ",")
+ + for _, part := range parts {
+ + // process part
+ + }
+</example_output>
+<example_input flag="false">
+- lines := strings.Split(s, "\n")
+- for _, line := range lines {
+- // process line
+- }
+</example_input>
+<example_output>
+</example_output>
+</examples>
+</issue>
+</issues>
+
+Output format:
+For each identified issue:
+1. State the issue type directly
+2. Provide a clear explanation of the problem
+3. Include the file path and line numbers
+4. Quote the exact text if line numbers are unavailable
+5. If completely confident you MAY include a recommended solution with example code
+6. MUST include any important caveats or notes about the replacement
+
+Do not use phrases like "For each occurrence" or other meta descriptions.
+List each instance separately if multiple exist.
+If no issues are found, output only:
+
+```
+No comments.
+```
diff --git a/claudetool/llm_review.go b/claudetool/llm_review.go
new file mode 100644
index 0000000..09d41b9
--- /dev/null
+++ b/claudetool/llm_review.go
@@ -0,0 +1,50 @@
+package claudetool
+
+import (
+ "context"
+ _ "embed"
+ "fmt"
+ "os/exec"
+ "strings"
+
+ "sketch.dev/llm"
+ "sketch.dev/llm/conversation"
+)
+
+//go:embed llm_codereview_prompt.txt
+var llmCodereviewPrompt string
+
+// doLLMReview analyzes the diff using an LLM subagent.
+func (r *CodeReviewer) doLLMReview(ctx context.Context) (string, error) {
+ // Get the full diff between initial commit and HEAD
+ cmd := exec.CommandContext(ctx, "git", "diff", r.initialCommit, "HEAD")
+ cmd.Dir = r.repoRoot
+ out, err := cmd.CombinedOutput()
+ if err != nil {
+ return "", fmt.Errorf("failed to get diff: %w\n%s", err, out)
+ }
+
+ // If no diff, nothing to check
+ if len(out) == 0 {
+ return "", nil
+ }
+
+ info := conversation.ToolCallInfoFromContext(ctx)
+ convo := info.Convo.SubConvo()
+ convo.SystemPrompt = strings.TrimSpace(llmCodereviewPrompt)
+ initialMessage := llm.UserStringMessage("<diff>\n" + string(out) + "\n</diff>")
+
+ resp, err := convo.SendMessage(initialMessage)
+ if err != nil {
+ return "", fmt.Errorf("failed to send LLM codereview message: %w", err)
+ }
+ if len(resp.Content) != 1 {
+ return "", fmt.Errorf("unexpected number of content blocks in LLM codereview response: %d", len(resp.Content))
+ }
+
+ response := resp.Content[0].Text
+ if strings.TrimSpace(response) == "No comments." {
+ return "", nil
+ }
+ return response, nil
+}