diff --git a/cmd/roborev/compact_test.go b/cmd/roborev/compact_test.go index 94282e1e7..f2d987ff1 100644 --- a/cmd/roborev/compact_test.go +++ b/cmd/roborev/compact_test.go @@ -428,9 +428,6 @@ func TestCompactWorktreeBranchResolution(t *testing.T) { opts := compactOptions{quiet: true} _ = runCompact(cmd, opts) - // API query should use the main repo path, not the worktree path. - // Branch filtering is handled client-side by filterReachableJobs. require.NotEmpty(t, receivedRepo, "expected repo param to be sent") - assert.Equal(t, repo.Dir, receivedRepo, - "repo: want main repo path") + assert.Equal(t, repo.Dir, receivedRepo, "repo: want main repo path") } diff --git a/cmd/roborev/fix.go b/cmd/roborev/fix.go index 6118142b1..30530b4fc 100644 --- a/cmd/roborev/fix.go +++ b/cmd/roborev/fix.go @@ -17,6 +17,7 @@ import ( "github.com/roborev-dev/roborev/internal/config" "github.com/roborev-dev/roborev/internal/daemon" "github.com/roborev-dev/roborev/internal/git" + "github.com/roborev-dev/roborev/internal/prompt" "github.com/roborev-dev/roborev/internal/storage" "github.com/roborev-dev/roborev/internal/streamfmt" "github.com/spf13/cobra" @@ -558,7 +559,7 @@ func jobReachable( } // SHA ref: check commit graph reachability. - if looksLikeSHA(ref) { + if git.LooksLikeSHA(ref) { reachable, err := git.IsAncestor(worktreeRoot, ref, "HEAD") if err != nil || reachable { return true @@ -588,21 +589,6 @@ func branchMatch(matchBranch, jobBranch string) bool { return jobBranch == matchBranch } -// looksLikeSHA returns true if s looks like a hex commit SHA (7-40 -// hex characters). This avoids calling git merge-base on task labels -// and other non-commit refs. -func looksLikeSHA(s string) bool { - if len(s) < 7 || len(s) > 40 { - return false - } - for _, c := range []byte(s) { - if (c < '0' || c > '9') && (c < 'a' || c > 'f') { - return false - } - } - return true -} - func queryOpenJobs( ctx context.Context, repoRoot, branch string, @@ -876,6 +862,18 @@ func fixSingleJob(cmd *cobra.Command, repoRoot string, jobID int64, opts fixOpti } } + // Fetch user comments for context (including legacy commit-based comments) + var commitID int64 + var gitRef string + if job != nil { + commitID = job.CommitIDValue() + gitRef = job.GitRef + } + comments, commentsErr := fetchComments(ctx, addr, jobID, commitID, gitRef) + if commentsErr != nil && !opts.quiet { + cmd.Printf("Warning: could not fetch comments for job %d: %v\n", jobID, commentsErr) + } + if !opts.quiet { cmd.Printf("Running fix agent (%s) to apply changes...\n\n", fixAgent.Name()) } @@ -894,7 +892,7 @@ func fixSingleJob(cmd *cobra.Command, repoRoot string, jobID int64, opts fixOpti RepoRoot: repoRoot, Agent: fixAgent, Output: out, - }, buildGenericFixPrompt(review.Output, minSev)) + }, buildGenericFixPrompt(review.Output, minSev, comments)) if fmtr != nil { fmtr.Flush() } @@ -952,9 +950,10 @@ func fixSingleJob(cmd *cobra.Command, repoRoot string, jobID int64, opts fixOpti // batchEntry holds a fetched job and its review for batch processing. type batchEntry struct { - jobID int64 - job *storage.ReviewJob - review *storage.Review + jobID int64 + job *storage.ReviewJob + review *storage.Review + comments []storage.Response } // runFixBatch discovers jobs (or uses provided IDs), splits them into batches @@ -1041,7 +1040,17 @@ func runFixBatch(cmd *cobra.Command, jobIDs []int64, branch string, allBranches, } continue } - entries = append(entries, batchEntry{jobID: id, job: job, review: review}) + var batchCommitID int64 + var batchGitRef string + if job != nil { + batchCommitID = job.CommitIDValue() + batchGitRef = job.GitRef + } + comments, commentsErr := fetchComments(ctx, batchAddr, id, batchCommitID, batchGitRef) + if commentsErr != nil && !opts.quiet { + cmd.Printf("Warning: could not fetch comments for job %d: %v\n", id, commentsErr) + } + entries = append(entries, batchEntry{jobID: id, job: job, review: review, comments: comments}) } if len(entries) == 0 { @@ -1175,7 +1184,11 @@ const batchPromptFooter = "## Instructions\n\nPlease apply fixes for all the fin // batchEntrySize returns the size of a single entry in the batch prompt. // The index parameter is the 1-based position in the batch. func batchEntrySize(index int, e batchEntry) int { - return len(fmt.Sprintf("## Review %d (Job %d — %s)\n\n%s\n\n", index, e.jobID, git.ShortSHA(e.job.GitRef), e.review.Output)) + toolAttempts, userComments := prompt.SplitResponses(e.comments) + size := len(fmt.Sprintf("## Review %d (Job %d — %s)\n\n%s\n\n", index, e.jobID, git.ShortSHA(e.job.GitRef), e.review.Output)) + size += len(prompt.FormatToolAttempts(toolAttempts)) + size += len(prompt.FormatUserComments(userComments)) + return size } // splitIntoBatches groups entries into batches respecting maxSize. @@ -1215,6 +1228,7 @@ func splitIntoBatches( // buildBatchFixPrompt creates a concatenated prompt from multiple reviews. // When minSeverity is non-empty, a severity filtering instruction is injected. +// User comments attached to each entry are included inline. func buildBatchFixPrompt(entries []batchEntry, minSeverity string) string { var sb strings.Builder sb.WriteString(batchPromptHeader) @@ -1224,9 +1238,12 @@ func buildBatchFixPrompt(entries []batchEntry, minSeverity string) string { } for i, e := range entries { + toolAttempts, userComments := prompt.SplitResponses(e.comments) fmt.Fprintf(&sb, "## Review %d (Job %d — %s)\n\n", i+1, e.jobID, git.ShortSHA(e.job.GitRef)) sb.WriteString(e.review.Output) sb.WriteString("\n\n") + sb.WriteString(prompt.FormatToolAttempts(toolAttempts)) + sb.WriteString(prompt.FormatUserComments(userComments)) } sb.WriteString(batchPromptFooter) @@ -1308,9 +1325,75 @@ func fetchReview(ctx context.Context, serverAddr string, jobID int64) (*storage. }) } +// fetchComments retrieves comments/responses for a job, including legacy +// commit-based comments merged via storage.MergeResponses. Prefers commit_id +// (unambiguous) when available, falls back to SHA for legacy jobs. +func fetchComments(ctx context.Context, serverAddr string, jobID, commitID int64, gitRef string) ([]storage.Response, error) { + return withFixDaemonRetryContext(ctx, serverAddr, func(addr string) ([]storage.Response, error) { + client := getDaemonHTTPClient(30 * time.Second) + + // Fetch by job ID + req, err := http.NewRequestWithContext(ctx, "GET", fmt.Sprintf("%s/api/comments?job_id=%d", addr, jobID), nil) + if err != nil { + return nil, err + } + + resp, err := client.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + return nil, fmt.Errorf("server error (%d): %s", resp.StatusCode, body) + } + + var result struct { + Responses []storage.Response `json:"responses"` + } + if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + return nil, err + } + responses := result.Responses + + // Also fetch legacy commit-based comments and merge. + // Prefer commit_id (unambiguous), fall back to SHA only when + // gitRef looks like a hex SHA (not a task label like "run"). + var legacyURL string + if commitID > 0 { + legacyURL = fmt.Sprintf("%s/api/comments?commit_id=%d", addr, commitID) + } else if git.LooksLikeSHA(gitRef) { + legacyURL = fmt.Sprintf("%s/api/comments?sha=%s", addr, gitRef) + } + if legacyURL != "" { + legacyReq, err := http.NewRequestWithContext(ctx, "GET", legacyURL, nil) + if err == nil { + legacyResp, err := client.Do(legacyReq) + if err == nil { + defer legacyResp.Body.Close() + if legacyResp.StatusCode == http.StatusOK { + var legacyResult struct { + Responses []storage.Response `json:"responses"` + } + if json.NewDecoder(legacyResp.Body).Decode(&legacyResult) == nil { + responses = storage.MergeResponses(responses, legacyResult.Responses) + } + } + } + } + } + + return responses, nil + }) +} + // buildGenericFixPrompt creates a fix prompt without knowing the analysis type. // When minSeverity is non-empty, a severity filtering instruction is prepended. -func buildGenericFixPrompt(analysisOutput, minSeverity string) string { +// Responses are split into tool attempts and user comments so each type +// receives appropriate framing in the prompt. +func buildGenericFixPrompt(analysisOutput, minSeverity string, responses []storage.Response) string { + toolAttempts, userComments := prompt.SplitResponses(responses) var sb strings.Builder sb.WriteString("# Fix Request\n\n") if inst := config.SeverityInstruction(minSeverity); inst != "" { @@ -1320,7 +1403,10 @@ func buildGenericFixPrompt(analysisOutput, minSeverity string) string { sb.WriteString("An analysis was performed and produced the following findings:\n\n") sb.WriteString("## Analysis Findings\n\n") sb.WriteString(analysisOutput) - sb.WriteString("\n\n## Instructions\n\n") + sb.WriteString("\n\n") + sb.WriteString(prompt.FormatToolAttempts(toolAttempts)) + sb.WriteString(prompt.FormatUserComments(userComments)) + sb.WriteString("## Instructions\n\n") sb.WriteString("Please apply the suggested changes from the analysis above. ") sb.WriteString("Make the necessary edits to address each finding. ") sb.WriteString("Focus on the highest priority items first.\n\n") diff --git a/cmd/roborev/fix_test.go b/cmd/roborev/fix_test.go index 51dafe2c9..517f9fe28 100644 --- a/cmd/roborev/fix_test.go +++ b/cmd/roborev/fix_test.go @@ -87,7 +87,7 @@ func TestBuildGenericFixPrompt(t *testing.T) { - Long function in main.go:50 - Missing error handling` - prompt := buildGenericFixPrompt(analysisOutput, "") + prompt := buildGenericFixPrompt(analysisOutput, "", nil) // Should include the analysis output assert.Contains(t, prompt, "Issues Found") @@ -100,6 +100,63 @@ func TestBuildGenericFixPrompt(t *testing.T) { assert.Contains(t, prompt, "git commit") } +func TestBuildGenericFixPromptWithComments(t *testing.T) { + comments := []storage.Response{ + {Responder: "dev", Response: "Don't touch the helper function", CreatedAt: time.Date(2026, 3, 15, 10, 0, 0, 0, time.UTC)}, + } + p := buildGenericFixPrompt("Found bug in foo.go", "", comments) + + assert.Contains(t, p, "User Comments") + assert.Contains(t, p, "Don't touch the helper function") + assert.Contains(t, p, "Found bug in foo.go") +} + +func TestBuildGenericFixPromptSplitsMixedResponses(t *testing.T) { + responses := []storage.Response{ + {Responder: "roborev-fix", Response: "Fix applied (commit: abc123)", CreatedAt: time.Date(2026, 3, 15, 9, 0, 0, 0, time.UTC)}, + {Responder: "alice", Response: "This is a false positive", CreatedAt: time.Date(2026, 3, 15, 10, 0, 0, 0, time.UTC)}, + } + p := buildGenericFixPrompt("Found bug in foo.go", "", responses) + + // Tool attempts should appear under "Previous Addressing Attempts" + assert.Contains(t, p, "Previous Addressing Attempts") + assert.Contains(t, p, "roborev-fix") + + // User comments should appear under "User Comments" + assert.Contains(t, p, "User Comments") + assert.Contains(t, p, "false positive") + assert.Contains(t, p, "alice") +} + +func TestBuildBatchFixPromptSplitsMixedResponses(t *testing.T) { + entries := []batchEntry{ + { + jobID: 1, + job: &storage.ReviewJob{GitRef: "abc123"}, + review: &storage.Review{ + Output: "Found HIGH bug in foo.go", + }, + comments: []storage.Response{ + {Responder: "roborev-refine", Response: "Created commit def456", CreatedAt: time.Date(2026, 3, 15, 9, 0, 0, 0, time.UTC)}, + {Responder: "alice", Response: "The foo.go finding is a false positive", CreatedAt: time.Date(2026, 3, 15, 10, 0, 0, 0, time.UTC)}, + }, + }, + } + p := buildBatchFixPrompt(entries, "") + + // Tool attempts should appear under "Previous Addressing Attempts" + assert.Contains(t, p, "Previous Addressing Attempts") + assert.Contains(t, p, "roborev-refine") + + // User comments should appear under "User Comments" + assert.Contains(t, p, "User Comments") + assert.Contains(t, p, "false positive") + assert.Contains(t, p, "alice") + + // Review output should be present + assert.Contains(t, p, "Found HIGH bug in foo.go") +} + func TestBuildGenericCommitPrompt(t *testing.T) { prompt := buildGenericCommitPrompt() @@ -2692,7 +2749,7 @@ func TestBuildGenericFixPromptMinSeverity(t *testing.T) { output := "Found bug in foo.go" t.Run("no filter", func(t *testing.T) { - p := buildGenericFixPrompt(output, "") + p := buildGenericFixPrompt(output, "", nil) if strings.Contains(p, "Severity filter") { assert.Condition(t, func() bool { return false @@ -2706,7 +2763,7 @@ func TestBuildGenericFixPromptMinSeverity(t *testing.T) { }) t.Run("high filter", func(t *testing.T) { - p := buildGenericFixPrompt(output, "high") + p := buildGenericFixPrompt(output, "high", nil) if !strings.Contains(p, "Severity filter") { assert.Condition(t, func() bool { return false @@ -2725,7 +2782,7 @@ func TestBuildGenericFixPromptMinSeverity(t *testing.T) { }) t.Run("low filter is no-op", func(t *testing.T) { - p := buildGenericFixPrompt(output, "low") + p := buildGenericFixPrompt(output, "low", nil) if strings.Contains(p, "Severity filter") { assert.Condition(t, func() bool { return false @@ -3019,28 +3076,6 @@ func TestFilterReachableJobsDetachedHead(t *testing.T) { } } -func TestLooksLikeSHA(t *testing.T) { - tests := []struct { - s string - want bool - }{ - {"abc1234", true}, - {"0000000000000000000000000000000000000000", true}, - {"abcdef1234567890abcdef1234567890abcdef12", true}, - {"abc123", false}, // too short (6 chars) - {"", false}, // empty - {"dirty", false}, // non-hex - {"run", false}, // task label - {"ABCDEF1", false}, // uppercase not valid - {"abc123..def456", false}, // range - } - for _, tt := range tests { - t.Run(tt.s, func(t *testing.T) { - assert.Equal(t, tt.want, looksLikeSHA(tt.s)) - }) - } -} - func TestRunFixOpenFiltersUnreachableJobs(t *testing.T) { repo, worktreeDir := setupWorktree(t) diff --git a/cmd/roborev/refine.go b/cmd/roborev/refine.go index d91a1a737..b90f2966c 100644 --- a/cmd/roborev/refine.go +++ b/cmd/roborev/refine.go @@ -518,8 +518,14 @@ func runRefine(ctx RunContext, opts refineOptions) error { fmt.Printf("Addressing review (job %d)...\n", currentFailedReview.JobID) } - // Get previous attempts for context - previousAttempts, err := client.GetCommentsForJob(currentFailedReview.JobID) + // Get previous attempts for context (including legacy commit-based) + var reviewCommitID int64 + var reviewGitRef string + if currentFailedReview.Job != nil { + reviewCommitID = currentFailedReview.Job.CommitIDValue() + reviewGitRef = currentFailedReview.Job.GitRef + } + previousAttempts, err := client.GetAllCommentsForJob(currentFailedReview.JobID, reviewCommitID, reviewGitRef) if err != nil { return fmt.Errorf("fetch previous comments: %w", err) } diff --git a/cmd/roborev/show.go b/cmd/roborev/show.go index 3a1c736ac..af25844ca 100644 --- a/cmd/roborev/show.go +++ b/cmd/roborev/show.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "net/http" + "os" "strconv" "strings" "time" @@ -116,9 +117,16 @@ Examples: } if jsonOutput { + // Include comments so tools/skills can see developer feedback. + type reviewWithComments struct { + storage.Review + Comments []storage.Response `json:"comments,omitempty"` + } + out := reviewWithComments{Review: review} + out.Comments = fetchShowComments(client, addr, review) enc := json.NewEncoder(cmd.OutOrStdout()) enc.SetIndent("", " ") - return enc.Encode(&review) + return enc.Encode(&out) } // Avoid redundant "job X (job X, ...)" output @@ -139,27 +147,14 @@ Examples: fmt.Println(review.Output) } - // Fetch and display comments - commentsURL := addr + fmt.Sprintf( - "/api/comments?job_id=%d", review.JobID, - ) - commentsResp, cErr := client.Get(commentsURL) - if cErr == nil { - defer commentsResp.Body.Close() - if commentsResp.StatusCode == http.StatusOK { - var result struct { - Responses []storage.Response `json:"responses"` - } - if json.NewDecoder(commentsResp.Body).Decode(&result) == nil && - len(result.Responses) > 0 { - fmt.Println() - fmt.Println("--- Comments ---") - for _, r := range result.Responses { - ts := r.CreatedAt.Format("Jan 02 15:04") - fmt.Printf("\n[%s] %s:\n", ts, r.Responder) - fmt.Println(r.Response) - } - } + // Fetch and display comments (including legacy commit-based) + if allComments := fetchShowComments(client, addr, review); len(allComments) > 0 { + fmt.Println() + fmt.Println("--- Comments ---") + for _, r := range allComments { + ts := r.CreatedAt.Format("Jan 02 15:04") + fmt.Printf("\n[%s] %s:\n", ts, r.Responder) + fmt.Println(r.Response) } } @@ -172,3 +167,51 @@ Examples: cmd.Flags().BoolVar(&jsonOutput, "json", false, "output as JSON") return cmd } + +// fetchShowComments retrieves comments for a review, merging legacy +// SHA-based comments via storage.MergeResponses. +func fetchShowComments(client *http.Client, addr string, review storage.Review) []storage.Response { + var responses []storage.Response + + // Fetch by job ID + commentsURL := addr + fmt.Sprintf("/api/comments?job_id=%d", review.JobID) + if resp, err := client.Get(commentsURL); err != nil { + fmt.Fprintf(os.Stderr, "Warning: could not fetch comments for job %d: %v\n", review.JobID, err) + } else if resp != nil { + defer resp.Body.Close() + if resp.StatusCode == http.StatusOK { + var result struct { + Responses []storage.Response `json:"responses"` + } + if json.NewDecoder(resp.Body).Decode(&result) == nil { + responses = result.Responses + } + } + } + + // Also fetch legacy commit-based comments and merge. + // Prefer commit_id (unambiguous), fall back to SHA for legacy jobs. + var legacyURL string + if review.Job != nil && review.Job.CommitID != nil { + legacyURL = addr + fmt.Sprintf("/api/comments?commit_id=%d", *review.Job.CommitID) + } else if review.Job != nil && git.LooksLikeSHA(review.Job.GitRef) { + legacyURL = addr + fmt.Sprintf("/api/comments?sha=%s", review.Job.GitRef) + } + if legacyURL != "" { + if resp, err := client.Get(legacyURL); err != nil { + fmt.Fprintf(os.Stderr, "Warning: could not fetch legacy comments: %v\n", err) + } else if resp != nil { + defer resp.Body.Close() + if resp.StatusCode == http.StatusOK { + var result struct { + Responses []storage.Response `json:"responses"` + } + if json.NewDecoder(resp.Body).Decode(&result) == nil { + responses = storage.MergeResponses(responses, result.Responses) + } + } + } + } + + return responses +} diff --git a/cmd/roborev/tui/fetch.go b/cmd/roborev/tui/fetch.go index c24721b0f..574711c2b 100644 --- a/cmd/roborev/tui/fetch.go +++ b/cmd/roborev/tui/fetch.go @@ -7,7 +7,6 @@ import ( "fmt" "net/http" neturl "net/url" - "sort" "strconv" "strings" "time" @@ -480,7 +479,8 @@ func (m model) loadReview(jobID int64) (*storage.Review, error) { return &review, nil } -// loadResponses fetches responses for a job, merging legacy SHA-based responses. +// loadResponses fetches responses for a job, merging legacy SHA-based +// responses via storage.MergeResponses. func (m model) loadResponses(jobID int64, review *storage.Review) []storage.Response { var responses []storage.Response @@ -492,28 +492,20 @@ func (m model) loadResponses(jobID int64, review *storage.Review) []storage.Resp responses = jobResult.Responses } - // Also fetch legacy responses by SHA for single commits (not ranges or dirty reviews) - // and merge with job responses to preserve full history during migration - if review.Job != nil && !strings.Contains(review.Job.GitRef, "..") && review.Job.GitRef != "dirty" { - var shaResult struct { + // Also fetch legacy commit-based responses and merge. + // Prefer commit_id (unambiguous), fall back to SHA for legacy jobs. + var legacyPath string + if review.Job != nil && review.Job.CommitID != nil { + legacyPath = fmt.Sprintf("/api/comments?commit_id=%d", *review.Job.CommitID) + } else if review.Job != nil && git.LooksLikeSHA(review.Job.GitRef) { + legacyPath = fmt.Sprintf("/api/comments?sha=%s", review.Job.GitRef) + } + if legacyPath != "" { + var legacyResult struct { Responses []storage.Response `json:"responses"` } - if err := m.getJSON(fmt.Sprintf("/api/comments?sha=%s", review.Job.GitRef), &shaResult); err == nil { - // Merge and dedupe by ID - seen := make(map[int64]bool) - for _, r := range responses { - seen[r.ID] = true - } - for _, r := range shaResult.Responses { - if !seen[r.ID] { - seen[r.ID] = true - responses = append(responses, r) - } - } - // Sort merged responses by CreatedAt for chronological order - sort.Slice(responses, func(i, j int) bool { - return responses[i].CreatedAt.Before(responses[j].CreatedAt) - }) + if err := m.getJSON(legacyPath, &legacyResult); err == nil { + responses = storage.MergeResponses(responses, legacyResult.Responses) } } diff --git a/internal/daemon/client.go b/internal/daemon/client.go index 4dbc58c3d..8ed83ec8a 100644 --- a/internal/daemon/client.go +++ b/internal/daemon/client.go @@ -383,6 +383,43 @@ func (c *HTTPClient) GetCommentsForJob(jobID int64) ([]storage.Response, error) return result.Responses, nil } +// GetAllCommentsForJob fetches comments for a job, merging legacy +// commit-based comments via storage.MergeResponses. When commitID > 0, +// fetches legacy by commit ID. Otherwise, if gitRef looks like a SHA, +// fetches by SHA. Callers may pre-validate gitRef via git.LooksLikeSHA. +func (c *HTTPClient) GetAllCommentsForJob(jobID, commitID int64, gitRef string) ([]storage.Response, error) { + responses, err := c.GetCommentsForJob(jobID) + if err != nil { + return nil, err + } + + // Also fetch legacy commit-based comments. + // Prefer commit_id (unambiguous), fall back to SHA only when + // gitRef looks like a hex commit SHA (not a task label). + var legacyURL string + if commitID > 0 { + legacyURL = fmt.Sprintf("%s/api/comments?commit_id=%d", c.baseURL, commitID) + } else if git.LooksLikeSHA(gitRef) { + legacyURL = fmt.Sprintf("%s/api/comments?sha=%s", c.baseURL, gitRef) + } + if legacyURL != "" { + legacyResp, err := c.httpClient.Get(legacyURL) + if err == nil { + defer legacyResp.Body.Close() + if legacyResp.StatusCode == http.StatusOK { + var result struct { + Responses []storage.Response `json:"responses"` + } + if json.NewDecoder(legacyResp.Body).Decode(&result) == nil { + responses = storage.MergeResponses(responses, result.Responses) + } + } + } + } + + return responses, nil +} + // RemapResult is the response from POST /api/remap. type RemapResult struct { Remapped int `json:"remapped"` diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 23d620515..b645e5679 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -24,6 +24,7 @@ import ( "github.com/roborev-dev/roborev/internal/config" "github.com/roborev-dev/roborev/internal/git" "github.com/roborev-dev/roborev/internal/githook" + "github.com/roborev-dev/roborev/internal/prompt" "github.com/roborev-dev/roborev/internal/storage" "github.com/roborev-dev/roborev/internal/version" ) @@ -2014,7 +2015,7 @@ func (s *Server) handleListComments(w http.ResponseWriter, r *http.Request) { var responses []storage.Response var err error - // Support lookup by job_id (preferred) or sha (legacy) + // Support lookup by job_id (preferred), commit_id, or sha (legacy) if jobIDStr := r.URL.Query().Get("job_id"); jobIDStr != "" { jobID, parseErr := strconv.ParseInt(jobIDStr, 10, 64) if parseErr != nil { @@ -2026,6 +2027,17 @@ func (s *Server) handleListComments(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusInternalServerError, fmt.Sprintf("get responses: %v", err)) return } + } else if cidStr := r.URL.Query().Get("commit_id"); cidStr != "" { + commitID, parseErr := strconv.ParseInt(cidStr, 10, 64) + if parseErr != nil { + writeError(w, http.StatusBadRequest, "invalid commit_id") + return + } + responses, err = s.db.GetCommentsForCommit(commitID) + if err != nil { + writeError(w, http.StatusInternalServerError, fmt.Sprintf("get responses: %v", err)) + return + } } else if sha := r.URL.Query().Get("sha"); sha != "" { responses, err = s.db.GetCommentsForCommitSHA(sha) if err != nil { @@ -2033,7 +2045,7 @@ func (s *Server) handleListComments(w http.ResponseWriter, r *http.Request) { return } } else { - writeError(w, http.StatusBadRequest, "job_id or sha parameter required") + writeError(w, http.StatusBadRequest, "job_id, commit_id, or sha parameter required") return } @@ -2482,10 +2494,21 @@ func (s *Server) handleFixJob(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, "parent job has no review to fix") return } + // Fetch comments for context (user feedback, previous tool attempts), + // including legacy commit-based comments. + commitID := parentJob.CommitIDValue() + var fallbackSHA string + if commitID == 0 && git.LooksLikeSHA(parentJob.GitRef) { + fallbackSHA = parentJob.GitRef + } + comments, commentsErr := s.db.GetAllCommentsForJob(req.ParentJobID, commitID, fallbackSHA) + if commentsErr != nil { + log.Printf("fix job for parent %d: failed to fetch comments: %v", req.ParentJobID, commentsErr) + } if req.Prompt != "" { - fixPrompt = buildFixPromptWithInstructions(review.Output, req.Prompt) + fixPrompt = buildFixPromptWithInstructions(review.Output, req.Prompt, comments) } else { - fixPrompt = buildFixPrompt(review.Output) + fixPrompt = buildFixPromptWithInstructions(review.Output, "", comments) } } @@ -2788,23 +2811,22 @@ func parseDuration(s string) (time.Duration, error) { } } -// buildFixPrompt constructs a prompt for fixing review findings. -func buildFixPrompt(reviewOutput string) string { - return buildFixPromptWithInstructions(reviewOutput, "") -} - // buildFixPromptWithInstructions constructs a fix prompt that includes the review -// findings and optional user-provided instructions. -func buildFixPromptWithInstructions(reviewOutput, userInstructions string) string { - prompt := "# Fix Request\n\n" + +// findings, optional user-provided instructions, and any comments/responses +// (split into tool attempts and user comments for proper framing). +func buildFixPromptWithInstructions(reviewOutput, userInstructions string, responses []storage.Response) string { + toolAttempts, userComments := prompt.SplitResponses(responses) + p := "# Fix Request\n\n" + "An analysis was performed and produced the following findings:\n\n" + "## Analysis Findings\n\n" + reviewOutput + "\n\n" + p += prompt.FormatToolAttempts(toolAttempts) + p += prompt.FormatUserComments(userComments) if userInstructions != "" { - prompt += "## Additional Instructions\n\n" + + p += "## Additional Instructions\n\n" + userInstructions + "\n\n" } - prompt += "## Instructions\n\n" + + p += "## Instructions\n\n" + "Please apply the suggested changes from the analysis above. " + "Make the necessary edits to address each finding. " + "Focus on the highest priority items first.\n\n" + @@ -2812,7 +2834,7 @@ func buildFixPromptWithInstructions(reviewOutput, userInstructions string) strin "1. Verify the code still compiles/passes linting\n" + "2. Run any relevant tests to ensure nothing is broken\n" + "3. Stage the changes with git add but do NOT commit — the changes will be captured as a patch\n" - return prompt + return p } // buildRebasePrompt constructs a prompt for re-applying a stale patch to current HEAD. diff --git a/internal/git/git.go b/internal/git/git.go index 143eeec50..064d48547 100644 --- a/internal/git/git.go +++ b/internal/git/git.go @@ -1369,3 +1369,10 @@ func ShortSHA(sha string) string { } return sha } + +// LooksLikeSHA returns true if s looks like a git commit SHA (7-40 hex chars, +// case-insensitive). The 7-char minimum matches git's default abbreviation +// length and safely excludes short hex task labels like "dead" or "cafe". +func LooksLikeSHA(s string) bool { + return len(s) >= 7 && len(s) <= 40 && isHex(s) +} diff --git a/internal/git/git_test.go b/internal/git/git_test.go index 0553fd135..54ec66964 100644 --- a/internal/git/git_test.go +++ b/internal/git/git_test.go @@ -1575,3 +1575,29 @@ func TestValidateWorktreeForRepo(t *testing.T) { assert.False(t, ValidateWorktreeForRepo(subDir, repo.Dir)) }) } + +func TestLooksLikeSHA(t *testing.T) { + tests := []struct { + s string + want bool + }{ + {"abc1234", true}, + {"0000000000000000000000000000000000000000", true}, + {"abcdef1234567890abcdef1234567890abcdef12", true}, + {"ABCDEF1", true}, // uppercase hex is valid + {"AbCd1234", true}, // mixed case + {"abc123", false}, // too short (6 chars, need 7+) + {"abcd", false}, // too short (4 chars) + {"dead", false}, // short hex task label + {"cafe12", false}, // 6-char hex, still too short + {"", false}, // empty + {"dirty", false}, // non-hex + {"run", false}, // task label + {"abc123..def456", false}, // range + } + for _, tt := range tests { + t.Run(tt.s, func(t *testing.T) { + assert.Equal(t, tt.want, LooksLikeSHA(tt.s)) + }) + } +} diff --git a/internal/prompt/prompt.go b/internal/prompt/prompt.go index 92688f21d..a7e039dcb 100644 --- a/internal/prompt/prompt.go +++ b/internal/prompt/prompt.go @@ -929,6 +929,67 @@ Be pragmatic - if previous attempts were rejected for being too minor, make more If they were rejected for being over-engineered, keep it simpler. ` +// UserCommentsHeader introduces user-authored comments on a review. +const UserCommentsHeader = `## User Comments + +The following comments were left by the developer on this review. +Take them into account when applying fixes — they may flag false +positives, provide additional context, or request specific approaches. + +` + +// IsToolResponse returns true when the response was left by an automated +// tool (roborev-fix, roborev-refine, etc.) rather than a human user. +func IsToolResponse(r storage.Response) bool { + return strings.HasPrefix(r.Responder, "roborev-") +} + +// SplitResponses partitions responses into tool-generated attempts and +// user-authored comments based on the Responder field. +func SplitResponses(responses []storage.Response) (toolAttempts, userComments []storage.Response) { + for _, r := range responses { + if IsToolResponse(r) { + toolAttempts = append(toolAttempts, r) + } else { + userComments = append(userComments, r) + } + } + return +} + +// FormatToolAttempts renders automated tool responses (roborev-fix, +// roborev-refine) into a prompt section. Returns empty string when empty. +func FormatToolAttempts(attempts []storage.Response) string { + if len(attempts) == 0 { + return "" + } + var sb strings.Builder + sb.WriteString(PreviousAttemptsHeader) + sb.WriteString("\n") + for _, attempt := range attempts { + fmt.Fprintf(&sb, "--- Attempt by %s at %s ---\n", + attempt.Responder, attempt.CreatedAt.Format("2006-01-02 15:04")) + sb.WriteString(attempt.Response) + sb.WriteString("\n\n") + } + return sb.String() +} + +// FormatUserComments renders user-authored comments into a prompt section. +// Returns empty string when there are no comments. +func FormatUserComments(comments []storage.Response) string { + if len(comments) == 0 { + return "" + } + var sb strings.Builder + sb.WriteString(UserCommentsHeader) + for _, c := range comments { + fmt.Fprintf(&sb, "**%s** (%s):\n%s\n\n", + c.Responder, c.CreatedAt.Format("2006-01-02 15:04"), c.Response) + } + return sb.String() +} + // BuildAddressPrompt constructs a prompt for addressing review findings. // When minSeverity is non-empty, a severity filtering instruction is // injected before the findings section. @@ -944,17 +1005,10 @@ func (b *Builder) BuildAddressPrompt(repoPath string, review *storage.Review, pr b.writeProjectGuidelines(&sb, repoCfg.ReviewGuidelines) } - // Include previous attempts to avoid repeating failed approaches - if len(previousAttempts) > 0 { - sb.WriteString(PreviousAttemptsHeader) - sb.WriteString("\n") - for _, attempt := range previousAttempts { - fmt.Fprintf(&sb, "--- Attempt by %s at %s ---\n", - attempt.Responder, attempt.CreatedAt.Format("2006-01-02 15:04")) - sb.WriteString(attempt.Response) - sb.WriteString("\n\n") - } - } + // Split responses into tool attempts and user comments + toolAttempts, userComments := SplitResponses(previousAttempts) + sb.WriteString(FormatToolAttempts(toolAttempts)) + sb.WriteString(FormatUserComments(userComments)) // Severity filter instruction (before findings) if inst := config.SeverityInstruction(minSeverity); inst != "" { diff --git a/internal/prompt/prompt_test.go b/internal/prompt/prompt_test.go index a52ea73b0..3ff6905c3 100644 --- a/internal/prompt/prompt_test.go +++ b/internal/prompt/prompt_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "time" "unicode/utf8" "github.com/roborev-dev/roborev/internal/config" @@ -1197,3 +1198,95 @@ func TestBuildAddressPromptShowsFullDiff(t *testing.T) { assertContains(t, diffSection, "keep.go", "retained file should be in address prompt diff") assertContains(t, diffSection, "custom.dat", "excluded file should still be in address prompt diff") } + +func TestSplitResponses(t *testing.T) { + responses := []storage.Response{ + {Responder: "roborev-fix", Response: "Fix applied"}, + {Responder: "alice", Response: "This is a false positive"}, + {Responder: "roborev-refine", Response: "Created commit abc123"}, + {Responder: "bob", Response: "Please keep this function"}, + } + + tool, user := SplitResponses(responses) + + assert.Len(t, tool, 2) + assert.Equal(t, "roborev-fix", tool[0].Responder) + assert.Equal(t, "roborev-refine", tool[1].Responder) + + assert.Len(t, user, 2) + assert.Equal(t, "alice", user[0].Responder) + assert.Equal(t, "bob", user[1].Responder) +} + +func TestSplitResponsesEmpty(t *testing.T) { + tool, user := SplitResponses(nil) + assert.Nil(t, tool) + assert.Nil(t, user) +} + +func TestFormatUserComments(t *testing.T) { + t.Run("nil returns empty", func(t *testing.T) { + assert.Empty(t, FormatUserComments(nil)) + }) + + t.Run("empty slice returns empty", func(t *testing.T) { + assert.Empty(t, FormatUserComments([]storage.Response{})) + }) + + t.Run("includes comment content", func(t *testing.T) { + comments := []storage.Response{ + {Responder: "alice", Response: "This is a false positive", CreatedAt: time.Date(2026, 3, 15, 10, 30, 0, 0, time.UTC)}, + {Responder: "bob", Response: "Please use a different approach", CreatedAt: time.Date(2026, 3, 15, 11, 0, 0, 0, time.UTC)}, + } + result := FormatUserComments(comments) + assert.Contains(t, result, "User Comments") + assert.Contains(t, result, "false positive") + assert.Contains(t, result, "different approach") + assert.Contains(t, result, "alice") + assert.Contains(t, result, "bob") + }) +} + +func TestFormatToolAttempts(t *testing.T) { + t.Run("nil returns empty", func(t *testing.T) { + assert.Empty(t, FormatToolAttempts(nil)) + }) + + t.Run("includes attempt content", func(t *testing.T) { + attempts := []storage.Response{ + {Responder: "roborev-fix", Response: "Fix applied (commit: abc123)", CreatedAt: time.Date(2026, 3, 15, 10, 0, 0, 0, time.UTC)}, + } + result := FormatToolAttempts(attempts) + assert.Contains(t, result, "Previous Addressing Attempts") + assert.Contains(t, result, "roborev-fix") + assert.Contains(t, result, "abc123") + }) +} + +func TestBuildAddressPromptSplitsResponses(t *testing.T) { + r := newTestRepo(t) + + b := NewBuilder(nil) + responses := []storage.Response{ + {Responder: "roborev-fix", Response: "Fix applied", CreatedAt: time.Date(2026, 3, 15, 9, 0, 0, 0, time.UTC)}, + {Responder: "alice", Response: "This is a false positive", CreatedAt: time.Date(2026, 3, 15, 10, 0, 0, 0, time.UTC)}, + } + + review := &storage.Review{ + JobID: 1, + Agent: "test", + Output: "Found bug in foo.go", + } + + p, err := b.BuildAddressPrompt(r.dir, review, responses, "") + require.NoError(t, err) + + // Tool attempts should appear under "Previous Addressing Attempts" + assert.Contains(t, p, "Previous Addressing Attempts") + assert.Contains(t, p, "roborev-fix") + + // User comments should appear under "User Comments" + assert.Contains(t, p, "User Comments") + assert.Contains(t, p, "false positive") + assert.Contains(t, p, "alice") +} diff --git a/internal/skills/claude/roborev-fix/SKILL.md b/internal/skills/claude/roborev-fix/SKILL.md index e24bdbcd6..a518ae92f 100644 --- a/internal/skills/claude/roborev-fix/SKILL.md +++ b/internal/skills/claude/roborev-fix/SKILL.md @@ -88,6 +88,10 @@ The JSON output has this structure: - `job.verdict`: `"P"` for pass, `"F"` for fail (may be empty if the review errored) - `job.git_ref`: the reviewed git ref (SHA, range, or synthetic ref) - `closed`: whether this review has already been closed +- `comments`: array of comments left on this review (may be empty or absent) + - Each comment has `responder` (who left it) and `response` (the text) + - Comments from `roborev-fix` or `roborev-refine` are automated tool records + - All other comments are from the developer (user feedback) Skip any reviews where `job.verdict` is `"P"` (passing reviews have no findings to fix). Skip any reviews where `job.verdict` is empty or missing (the review may have errored and is not actionable). @@ -95,6 +99,8 @@ Skip any reviews where `closed` is `true`, unless the user explicitly provided t If all reviews are skipped, inform the user there is nothing to fix. +If the review has `comments`, respect any developer feedback (false positives, preferred approaches). + ### 3. Fix all findings If a finding's context is unclear from the review output alone and `job.git_ref` is not `"dirty"`, run `git show ` to see the original diff. Only do this when needed — the review output usually contains enough detail (file paths, line numbers, descriptions) to fix findings directly. @@ -123,7 +129,7 @@ Run these as **separate commands**, but only run `roborev close` after confirming the comment succeeded: ```bash -roborev comment --job "" +roborev comment --commenter roborev-fix --job "" # Only if the comment above succeeded: roborev close ``` @@ -157,9 +163,9 @@ Agent: 4. Fixes all 3 findings across both reviews, sorted by severity, grouped by file 5. Runs `go test ./...` to verify 6. Records comments and closes reviews: - - `roborev comment --job 1019 "Fixed null check and added error handling"` + - `roborev comment --commenter roborev-fix --job 1019 "Fixed null check and added error handling"` - `roborev close 1019` - - `roborev comment --job 1021 "Fixed missing validation"` + - `roborev comment --commenter roborev-fix --job 1021 "Fixed missing validation"` - `roborev close 1021` 7. Commits the changes per project conventions @@ -173,7 +179,7 @@ Agent: 3. Fixes the 2 findings from job 1019 4. Runs `go test ./...` to verify 5. Records comment and closes review: - - `roborev comment --job 1019 "Fixed null check in foo.go and error handling in bar.go"` + - `roborev comment --commenter roborev-fix --job 1019 "Fixed null check in foo.go and error handling in bar.go"` - `roborev close 1019` 6. Commits the changes per project conventions diff --git a/internal/skills/claude/roborev-refine/SKILL.md b/internal/skills/claude/roborev-refine/SKILL.md index 6c601a544..b78ac31dd 100644 --- a/internal/skills/claude/roborev-refine/SKILL.md +++ b/internal/skills/claude/roborev-refine/SKILL.md @@ -130,7 +130,7 @@ Commit first per the project's conventions (see CLAUDE.md). Only after the commit succeeds, record a summary comment on the review and close it: ```bash -roborev comment --job -m "$(cat <<'ROBOREV_COMMENT' +roborev comment --commenter roborev-refine --job -m "$(cat <<'ROBOREV_COMMENT' ROBOREV_COMMENT )" diff --git a/internal/skills/codex/roborev-fix/SKILL.md b/internal/skills/codex/roborev-fix/SKILL.md index fcd23346d..b99961e7d 100644 --- a/internal/skills/codex/roborev-fix/SKILL.md +++ b/internal/skills/codex/roborev-fix/SKILL.md @@ -88,6 +88,10 @@ The JSON output has this structure: - `job.verdict`: `"P"` for pass, `"F"` for fail (may be empty if the review errored) - `job.git_ref`: the reviewed git ref (SHA, range, or synthetic ref) - `closed`: whether this review has already been closed +- `comments`: array of comments left on this review (may be empty or absent) + - Each comment has `responder` (who left it) and `response` (the text) + - Comments from `roborev-fix` or `roborev-refine` are automated tool records + - All other comments are from the developer (user feedback) Skip any reviews where `job.verdict` is `"P"` (passing reviews have no findings to fix). Skip any reviews where `job.verdict` is empty or missing (the review may have errored and is not actionable). @@ -95,6 +99,8 @@ Skip any reviews where `closed` is `true`, unless the user explicitly provided t If all reviews are skipped, inform the user there is nothing to fix. +If the review has `comments`, respect any developer feedback (false positives, preferred approaches). + ### 3. Fix all findings If a finding's context is unclear from the review output alone and `job.git_ref` is not `"dirty"`, run `git show ` to see the original diff. Only do this when needed — the review output usually contains enough detail (file paths, line numbers, descriptions) to fix findings directly. @@ -123,7 +129,7 @@ Run these as **separate commands**, but only run `roborev close` after confirming the comment succeeded: ```bash -roborev comment --job "" +roborev comment --commenter roborev-fix --job "" # Only if the comment above succeeded: roborev close ``` @@ -157,9 +163,9 @@ Agent: 4. Fixes all 3 findings across both reviews, sorted by severity, grouped by file 5. Runs `go test ./...` to verify 6. Records comments and closes reviews: - - `roborev comment --job 1019 "Fixed null check and added error handling"` + - `roborev comment --commenter roborev-fix --job 1019 "Fixed null check and added error handling"` - `roborev close 1019` - - `roborev comment --job 1021 "Fixed missing validation"` + - `roborev comment --commenter roborev-fix --job 1021 "Fixed missing validation"` - `roborev close 1021` 7. Commits the changes per project conventions @@ -173,7 +179,7 @@ Agent: 3. Fixes the 2 findings from job 1019 4. Runs `go test ./...` to verify 5. Records comment and closes review: - - `roborev comment --job 1019 "Fixed null check in foo.go and error handling in bar.go"` + - `roborev comment --commenter roborev-fix --job 1019 "Fixed null check in foo.go and error handling in bar.go"` - `roborev close 1019` 6. Commits the changes per project conventions diff --git a/internal/skills/codex/roborev-refine/SKILL.md b/internal/skills/codex/roborev-refine/SKILL.md index 710a64896..e8982696b 100644 --- a/internal/skills/codex/roborev-refine/SKILL.md +++ b/internal/skills/codex/roborev-refine/SKILL.md @@ -130,7 +130,7 @@ Commit first per the project's conventions (see CLAUDE.md). Only after the commit succeeds, record a summary comment on the review and close it: ```bash -roborev comment --job -m "$(cat <<'ROBOREV_COMMENT' +roborev comment --commenter roborev-refine --job -m "$(cat <<'ROBOREV_COMMENT' ROBOREV_COMMENT )" diff --git a/internal/storage/models.go b/internal/storage/models.go index ba6b5cc16..57a3d9a8b 100644 --- a/internal/storage/models.go +++ b/internal/storage/models.go @@ -101,6 +101,14 @@ func (j ReviewJob) IsDirtyJob() bool { return j.DiffContent != nil || j.GitRef == "dirty" } +// CommitIDValue returns the commit ID as a plain int64 (0 if nil). +func (j ReviewJob) CommitIDValue() int64 { + if j.CommitID != nil { + return *j.CommitID + } + return 0 +} + // IsTaskJob returns true if this is a task job (run, analyze, custom label) rather than // a commit review or dirty review. Task jobs have pre-stored prompts and no verdicts. // Compact jobs are not considered task jobs since they produce P/F verdicts. diff --git a/internal/storage/reviews.go b/internal/storage/reviews.go index 7805e30c5..3b2495b08 100644 --- a/internal/storage/reviews.go +++ b/internal/storage/reviews.go @@ -3,6 +3,7 @@ package storage import ( "database/sql" "fmt" + "sort" "strings" "time" @@ -524,3 +525,51 @@ func (db *DB) GetCommentsForCommitSHA(sha string) ([]Response, error) { } return db.GetCommentsForCommit(commit.ID) } + +// GetAllCommentsForJob returns all comments for a job, merging legacy +// commit-based comments via MergeResponses. When commitID > 0, fetches +// legacy comments by commit ID. Otherwise, if fallbackSHA is non-empty, +// fetches by SHA. Callers should validate the SHA (e.g. via +// git.LooksLikeSHA) before passing it here. +func (db *DB) GetAllCommentsForJob(jobID, commitID int64, fallbackSHA string) ([]Response, error) { + responses, err := db.GetCommentsForJob(jobID) + if err != nil { + return nil, err + } + + var legacyResponses []Response + var legacyErr error + if commitID > 0 { + legacyResponses, legacyErr = db.GetCommentsForCommit(commitID) + } else if fallbackSHA != "" { + legacyResponses, legacyErr = db.GetCommentsForCommitSHA(fallbackSHA) + } + if legacyErr != nil { + return responses, fmt.Errorf("legacy comment lookup: %w", legacyErr) + } + + return MergeResponses(responses, legacyResponses), nil +} + +// MergeResponses deduplicates two Response slices by ID and returns +// a chronologically sorted result. This is used wherever job-based +// and legacy commit-based comments are merged. +func MergeResponses(primary, extra []Response) []Response { + if len(extra) == 0 { + return primary + } + seen := make(map[int64]bool, len(primary)) + for _, r := range primary { + seen[r.ID] = true + } + for _, r := range extra { + if !seen[r.ID] { + seen[r.ID] = true + primary = append(primary, r) + } + } + sort.Slice(primary, func(i, j int) bool { + return primary[i].CreatedAt.Before(primary[j].CreatedAt) + }) + return primary +} diff --git a/internal/storage/reviews_test.go b/internal/storage/reviews_test.go index 4f86804da..972d5558f 100644 --- a/internal/storage/reviews_test.go +++ b/internal/storage/reviews_test.go @@ -2,9 +2,10 @@ package storage import ( "database/sql" + "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "testing" ) // TestAddCommentToJobAllStates verifies that comments can be added to jobs @@ -127,6 +128,97 @@ func TestAddCommentToJobWithNoReview(t *testing.T) { assert.Equal(t, "Comment on job without review", resp.Response) } +func TestGetAllCommentsForJob(t *testing.T) { + db := openTestDB(t) + defer db.Close() + + _, commit, job := createJobChain(t, db, "/tmp/test-repo", "abc1234") + machineID, _ := db.GetMachineID() + + // Insert with explicit timestamps to avoid flaky ordering + t1 := "2026-03-15T10:00:00Z" + t2 := "2026-03-15T11:00:00Z" + t3 := "2026-03-15T12:00:00Z" + + _, err := db.Exec( + `INSERT INTO responses (job_id, responder, response, uuid, source_machine_id, created_at) + VALUES (?, ?, ?, ?, ?, ?)`, + job.ID, "alice", "Job-based comment", GenerateUUID(), machineID, t1, + ) + require.NoError(t, err) + + _, err = db.Exec( + `INSERT INTO responses (commit_id, responder, response, uuid, source_machine_id, created_at) + VALUES (?, ?, ?, ?, ?, ?)`, + commit.ID, "bob", "Legacy commit comment", GenerateUUID(), machineID, t2, + ) + require.NoError(t, err) + + t.Run("merges job and legacy commit comments", func(t *testing.T) { + all, err := db.GetAllCommentsForJob(job.ID, commit.ID, "") + require.NoError(t, err) + require.Len(t, all, 2) + assert.Equal(t, "alice", all[0].Responder) + assert.Equal(t, "bob", all[1].Responder) + }) + + t.Run("skips legacy fallback when commitID is zero and no gitRef", func(t *testing.T) { + all, err := db.GetAllCommentsForJob(job.ID, 0, "") + require.NoError(t, err) + require.Len(t, all, 1) + assert.Equal(t, "alice", all[0].Responder) + }) + + t.Run("falls back to SHA when commitID is zero", func(t *testing.T) { + all, err := db.GetAllCommentsForJob(job.ID, 0, "abc1234") + require.NoError(t, err) + require.Len(t, all, 2) + assert.Equal(t, "alice", all[0].Responder) + assert.Equal(t, "bob", all[1].Responder) + }) + + t.Run("skips fallback when SHA is empty", func(t *testing.T) { + // Callers pass "" when gitRef is not a valid SHA (e.g. ranges). + all, err := db.GetAllCommentsForJob(job.ID, 0, "") + require.NoError(t, err) + require.Len(t, all, 1) + }) + + t.Run("deduplicates overlapping comments", func(t *testing.T) { + // Insert a response linked to both job_id AND commit_id to + // exercise the dedup branch — it appears in both queries. + _, err := db.Exec( + `INSERT INTO responses (job_id, commit_id, responder, response, uuid, source_machine_id, created_at) + VALUES (?, ?, ?, ?, ?, ?, ?)`, + job.ID, commit.ID, "charlie", "Dual-linked comment", + GenerateUUID(), machineID, t3, + ) + require.NoError(t, err) + + all, err := db.GetAllCommentsForJob(job.ID, commit.ID, "") + require.NoError(t, err) + // alice (job), bob (commit), charlie (both) — charlie should + // appear only once despite matching both queries. + require.Len(t, all, 3) + assert.Equal(t, "alice", all[0].Responder) + assert.Equal(t, "bob", all[1].Responder) + assert.Equal(t, "charlie", all[2].Responder) + }) + + t.Run("returns error on legacy lookup failure", func(t *testing.T) { + // Use a hex string that looks like a SHA but doesn't exist in the + // commits table to trigger a legacy lookup error. + all, err := db.GetAllCommentsForJob(job.ID, 0, "deadbeefdeadbeef") + require.Error(t, err) + assert.Contains(t, err.Error(), "legacy comment lookup") + // Job-based comments should still be returned (alice + charlie + // which was dual-linked in the dedup subtest above). + require.Len(t, all, 2) + assert.Equal(t, "alice", all[0].Responder) + assert.Equal(t, "charlie", all[1].Responder) + }) +} + func TestGetReviewByJobIDIncludesModel(t *testing.T) { db := openTestDB(t) defer db.Close()