diff --git a/internal/prompt/prompt.go b/internal/prompt/prompt.go index a7e039dc..0e6b26a6 100644 --- a/internal/prompt/prompt.go +++ b/internal/prompt/prompt.go @@ -119,6 +119,17 @@ responses from developers. Use this context to: - Consider developer responses about why certain patterns exist ` +// InRangeReviewsHeader introduces per-commit reviews within a range review +const InRangeReviewsHeader = ` +## Per-Commit Reviews in This Range + +The following commits in this range have already been individually reviewed. +Issues found in earlier commits may have been fixed by later commits in the range. + +Do not re-raise issues identified below unless they persist in the final code. +Focus on cross-commit interactions and problems not caught by per-commit reviews. +` + // ReviewContext holds a commit SHA and its associated review (if any) plus responses type ReviewContext struct { SHA string @@ -607,6 +618,10 @@ func (b *Builder) buildRangePrompt(repoPath, rangeRef string, repoID int64, cont return "", fmt.Errorf("get range commits: %w", err) } + // Include per-commit reviews for commits in this range so the + // reviewer doesn't re-raise issues already found and addressed. + b.writeInRangeReviews(&optionalContext, commits) + // Commit range section var currentRequired strings.Builder currentRequired.WriteString("## Commit Range\n\n") @@ -801,37 +816,81 @@ func (b *Builder) writePreviousAttemptsForGitRef(sb *strings.Builder, gitRef str } } -// getPreviousReviewContexts gets the N commits before the target and looks up their reviews and responses -func (b *Builder) getPreviousReviewContexts(repoPath, sha string, count int) ([]ReviewContext, error) { - // Get parent commits from git - parentSHAs, err := git.GetParentCommits(repoPath, sha, count) - if err != nil { - return nil, fmt.Errorf("get parent commits: %w", err) +// writeInRangeReviews writes per-commit reviews for commits within a range. +// This gives the range reviewer context about issues already found and +// addressed, preventing duplicate findings. +func (b *Builder) writeInRangeReviews(sb *strings.Builder, commits []string) { + if b.db == nil || len(commits) == 0 { + return } - var contexts []ReviewContext - for _, parentSHA := range parentSHAs { - ctx := ReviewContext{SHA: parentSHA} + contexts := b.lookupReviewContexts(commits, true) + if len(contexts) == 0 { + return + } - // Try to look up review for this commit - review, err := b.db.GetReviewByCommitSHA(parentSHA) - if err == nil { - ctx.Review = review + sb.WriteString(InRangeReviewsHeader) + sb.WriteString("\n") - // Also fetch comments for this review's job - if review.JobID > 0 { - responses, err := b.db.GetCommentsForJob(review.JobID) - if err == nil { - ctx.Responses = responses - } + for _, ctx := range contexts { + shortSHA := git.ShortSHA(ctx.SHA) + verdict := storage.ParseVerdict(ctx.Review.Output) + verdictLabel := "unknown" + switch verdict { + case "P": + verdictLabel = "passed" + case "F": + verdictLabel = "failed" + } + + fmt.Fprintf(sb, "--- Commit %s (%s, %s) ---\n", + shortSHA, ctx.Review.Agent, verdictLabel) + sb.WriteString(ctx.Review.Output) + sb.WriteString("\n") + + if len(ctx.Responses) > 0 { + sb.WriteString("\nComments on this review:\n") + for _, resp := range ctx.Responses { + fmt.Fprintf(sb, "- %s: %q\n", resp.Responder, resp.Response) } } - // If no review found, ctx.Review stays nil + sb.WriteString("\n") + } +} +// lookupReviewContexts fetches review contexts for the given SHAs. +// When skipMissing is true, SHAs without reviews are omitted; when false, +// they are included with Review == nil (used by writePreviousReviews to +// show "No review available"). +func (b *Builder) lookupReviewContexts(shas []string, skipMissing bool) []ReviewContext { + var contexts []ReviewContext + for _, sha := range shas { + review, err := b.db.GetReviewByCommitSHA(sha) + if err != nil { + if skipMissing { + continue + } + contexts = append(contexts, ReviewContext{SHA: sha}) + continue + } + ctx := ReviewContext{SHA: sha, Review: review} + if review.JobID > 0 { + if responses, err := b.db.GetCommentsForJob(review.JobID); err == nil { + ctx.Responses = responses + } + } contexts = append(contexts, ctx) } + return contexts +} - return contexts, nil +// getPreviousReviewContexts gets the N commits before the target and looks up their reviews and responses +func (b *Builder) getPreviousReviewContexts(repoPath, sha string, count int) ([]ReviewContext, error) { + parentSHAs, err := git.GetParentCommits(repoPath, sha, count) + if err != nil { + return nil, fmt.Errorf("get parent commits: %w", err) + } + return b.lookupReviewContexts(parentSHAs, false), nil } // SystemPromptDesignReview is the base instruction for reviewing design documents. diff --git a/internal/prompt/prompt_test.go b/internal/prompt/prompt_test.go index 3ff6905c..4d00cb15 100644 --- a/internal/prompt/prompt_test.go +++ b/internal/prompt/prompt_test.go @@ -1290,3 +1290,70 @@ func TestBuildAddressPromptSplitsResponses(t *testing.T) { assert.Contains(t, p, "false positive") assert.Contains(t, p, "alice") } + +func TestBuildRangePrompt_IncludesInRangeReviews(t *testing.T) { + r := newTestRepo(t) + + // Create initial commit (will serve as range start / merge base) + require.NoError(t, os.WriteFile(filepath.Join(r.dir, "base.txt"), []byte("base\n"), 0o644)) + r.git("add", "base.txt") + r.git("commit", "-m", "initial") + baseSHA := r.git("rev-parse", "HEAD") + + // Create two commits on the "branch" + require.NoError(t, os.WriteFile(filepath.Join(r.dir, "base.txt"), []byte("change1\n"), 0o644)) + r.git("add", "base.txt") + r.git("commit", "-m", "first feature commit") + commit1 := r.git("rev-parse", "HEAD") + + require.NoError(t, os.WriteFile(filepath.Join(r.dir, "base.txt"), []byte("change2\n"), 0o644)) + r.git("add", "base.txt") + r.git("commit", "-m", "second feature commit") + commit2 := r.git("rev-parse", "HEAD") + + // Set up DB with repo and per-commit reviews + db := testutil.OpenTestDB(t) + repo, err := db.GetOrCreateRepo(r.dir) + require.NoError(t, err) + + testutil.CreateCompletedReview(t, db, repo.ID, commit1, "test", + "Found bug: missing null check in handler\n\nVerdict: FAIL") + testutil.CreateCompletedReview(t, db, repo.ID, commit2, "test", + "No issues found.\n\nVerdict: PASS") + + // Build range prompt + rangeRef := baseSHA + ".." + commit2 + builder := NewBuilder(db) + prompt, err := builder.Build(r.dir, rangeRef, repo.ID, 0, "test", "") + require.NoError(t, err) + + // Should contain the in-range reviews section + assert := assert.New(t) + assert.Contains(prompt, "Per-Commit Reviews in This Range") + assert.Contains(prompt, "Do not re-raise issues") + assert.Contains(prompt, "missing null check in handler") + assert.Contains(prompt, "No issues found.") + assert.Contains(prompt, "failed") + assert.Contains(prompt, "passed") +} + +func TestBuildRangePrompt_NoInRangeReviewsWithoutDB(t *testing.T) { + r := newTestRepo(t) + + require.NoError(t, os.WriteFile(filepath.Join(r.dir, "base.txt"), []byte("base\n"), 0o644)) + r.git("add", "base.txt") + r.git("commit", "-m", "initial") + baseSHA := r.git("rev-parse", "HEAD") + + require.NoError(t, os.WriteFile(filepath.Join(r.dir, "base.txt"), []byte("change\n"), 0o644)) + r.git("add", "base.txt") + r.git("commit", "-m", "feature") + headSHA := r.git("rev-parse", "HEAD") + + // Build without DB — should not include in-range reviews section + builder := NewBuilder(nil) + prompt, err := builder.Build(r.dir, baseSHA+".."+headSHA, 0, 0, "test", "") + require.NoError(t, err) + + assert.NotContains(t, prompt, "Per-Commit Reviews in This Range") +} diff --git a/internal/skills/claude/roborev-refine/SKILL.md b/internal/skills/claude/roborev-refine/SKILL.md index b78ac31d..e7bb8c7f 100644 --- a/internal/skills/claude/roborev-refine/SKILL.md +++ b/internal/skills/claude/roborev-refine/SKILL.md @@ -143,7 +143,8 @@ by interpolating dynamic text directly into a shell string. Review-derived content may contain shell metacharacters that could cause unintended execution. The comment should reference each finding by severity and file, state what was -fixed, and note any findings intentionally skipped. Keep it concise. +fixed, and note why any dismissed findings were skipped. These comments are +included in re-review prompts. Keep it concise. #### 3d. Re-review diff --git a/internal/skills/codex/roborev-refine/SKILL.md b/internal/skills/codex/roborev-refine/SKILL.md index e8982696..86051554 100644 --- a/internal/skills/codex/roborev-refine/SKILL.md +++ b/internal/skills/codex/roborev-refine/SKILL.md @@ -143,7 +143,8 @@ by interpolating dynamic text directly into a shell string. Review-derived content may contain shell metacharacters that could cause unintended execution. The comment should reference each finding by severity and file, state what was -fixed, and note any findings intentionally skipped. Keep it concise. +fixed, and note why any dismissed findings were skipped. These comments are +included in re-review prompts. Keep it concise. #### 3d. Re-review