From dd0c027cf18448f50b42accd043f04385bc6ece6 Mon Sep 17 00:00:00 2001 From: "Juan C. Andreu" Date: Mon, 25 May 2026 15:45:59 +0000 Subject: [PATCH 1/5] feat(review): add --post flag for headless GitHub review submission `prr review --post` posts the synthesized findings as a single GitHub Review (event=COMMENT) with one inline comment per file+line-anchored finding. Mirrors the TUI's Ctrl+S batch-submit path so headless and interactive runs produce equivalent reviews. Confirms via stdin by default; --yes / -y bypasses the prompt and is required in non-TTY contexts (CI fails fast instead of blocking on a stdin read). --- cmd/prr/main.go | 102 ++++++++++++++++++++++++++++++++++++- internal/review/publish.go | 86 +++++++++++++++++++++++++++++++ 2 files changed, 187 insertions(+), 1 deletion(-) create mode 100644 internal/review/publish.go diff --git a/cmd/prr/main.go b/cmd/prr/main.go index cd0bab7..f7f21c3 100644 --- a/cmd/prr/main.go +++ b/cmd/prr/main.go @@ -473,6 +473,8 @@ func runReview(debug bool, args []string) { quiet := false reviewDebug := debug bugPriors := false + post := false + postYes := false var reviewModeStr string // Parse flags and find the PR number @@ -490,6 +492,10 @@ func runReview(debug bool, args []string) { reviewDebug = true } else if arg == "--bug-priors" { bugPriors = true + } else if arg == "--post" { + post = true + } else if arg == "--yes" || arg == "-y" { + postYes = true } else if after, ok := strings.CutPrefix(arg, "--review-mode="); ok { reviewModeStr = after } else if arg == "--help" || arg == "-h" { @@ -735,6 +741,98 @@ func runReview(debug bool, args []string) { fmt.Fprintf(os.Stderr, " Report saved to %s\n\n", cliInfo.Render(outputPath)) } } + + // Post findings to GitHub as a single batch review (event=COMMENT) + // when --post is set. Mirrors the TUI's Ctrl+S batch-submit path. + if post { + if result.StructuredReview == nil || len(result.StructuredReview.Findings) == 0 { + if !quiet { + fmt.Fprintf(os.Stderr, " %s no findings to post\n\n", cliDim.Render("--post:")) + } + } else if result.PR == nil || result.PR.HeadRefOid == "" { + printError(fmt.Errorf("--post: missing PR head SHA; cannot anchor inline comments")) + os.Exit(1) + } else { + ok, err := confirmPost(prNumber, result.StructuredReview.Findings, postYes) + if err != nil { + printError(err) + os.Exit(1) + } + if !ok { + fmt.Fprintf(os.Stderr, " %s post cancelled\n\n", cliDim.Render("--post:")) + } else { + n, err := review.PostStructuredReview(prNumber, result.PR.HeadRefOid, result.StructuredReview.Findings) + if err != nil { + printError(fmt.Errorf("posting review: %w", err)) + os.Exit(1) + } + if !quiet { + fmt.Fprintf(os.Stderr, " Posted review with %s inline comment(s) to PR #%s\n\n", + cliInfo.Render(fmt.Sprintf("%d", n)), prNumber) + } + } + } + } +} + +// confirmPost prompts the user to confirm posting findings to GitHub. +// When yes=true the prompt is skipped. In a non-interactive context +// (stdin is not a TTY) the call fails loudly so a CI job without --yes +// errors immediately instead of blocking forever on a stdin read. +func confirmPost(prNumber string, findings []state.ReviewFinding, yes bool) (bool, error) { + inline := 0 + for _, f := range findings { + if f.File != "" && f.Line > 0 { + inline++ + } + } + summary := fmt.Sprintf("%d finding(s), %d inline, severity: %s", + len(findings), inline, summarizeSeverities(findings)) + + if yes { + fmt.Fprintf(os.Stderr, " %s %s — posting (--yes)\n\n", + cliHeader.Render("Review:"), cliInfo.Render(summary)) + return true, nil + } + + stat, err := os.Stdin.Stat() + if err != nil || (stat.Mode()&os.ModeCharDevice) == 0 { + return false, fmt.Errorf("--post requires confirmation; stdin is not a TTY. Re-run with --yes to confirm non-interactively") + } + + fmt.Fprintf(os.Stderr, "\n %s %s\n", cliHeader.Render("Review:"), cliInfo.Render(summary)) + fmt.Fprintf(os.Stderr, " Post to PR #%s? [y/N] ", prNumber) + + reader := bufio.NewReader(os.Stdin) + line, err := reader.ReadString('\n') + if err != nil && err != io.EOF { + return false, fmt.Errorf("reading confirmation: %w", err) + } + ans := strings.ToLower(strings.TrimSpace(line)) + return ans == "y" || ans == "yes", nil +} + +// summarizeSeverities returns a compact "2 critical · 3 high · 1 med" +// breakdown for the confirmation prompt. +func summarizeSeverities(findings []state.ReviewFinding) string { + counts := map[string]int{} + for _, f := range findings { + counts[f.Severity]++ + } + var parts []string + for _, sev := range []string{"critical", "high", "medium", "low", "nit"} { + if n, ok := counts[sev]; ok { + label := sev + if sev == "medium" { + label = "med" + } + parts = append(parts, fmt.Sprintf("%d %s", n, label)) + } + } + if len(parts) == 0 { + return "none" + } + return strings.Join(parts, " · ") } // marshalReviewResult builds the JSON byte payload for a review run. @@ -773,7 +871,9 @@ func printReviewUsage() { fmt.Fprintf(os.Stderr, " --debug Print LLM tool calls, user messages, and responses\n") fmt.Fprintf(os.Stderr, " (compact by default; PRR_DEBUG_VERBOSE=1 for full prompts)\n") fmt.Fprintf(os.Stderr, " --bug-priors Inject recent fix-shaped commits as known-failure priors\n") - fmt.Fprintf(os.Stderr, " --review-mode= full (default) reviews every file; aoi-only skips files without AOIs\n\n") + fmt.Fprintf(os.Stderr, " --review-mode= full (default) reviews every file; aoi-only skips files without AOIs\n") + fmt.Fprintf(os.Stderr, " --post Post findings to GitHub as a batch review (event=COMMENT)\n") + fmt.Fprintf(os.Stderr, " --yes, -y Skip the confirmation prompt for --post (required in non-TTY contexts)\n\n") } func createAIClient(cfg *config.Config) ai.Client { diff --git a/internal/review/publish.go b/internal/review/publish.go new file mode 100644 index 0000000..a8ac3a3 --- /dev/null +++ b/internal/review/publish.go @@ -0,0 +1,86 @@ +package review + +import ( + "fmt" + "strings" + + "github.com/andreujuanc/prr/internal/git" + "github.com/andreujuanc/prr/internal/state" +) + +// PostStructuredReview submits the synthesized findings as a single +// GitHub PR review (event=COMMENT) with one inline comment per finding +// that has a valid file and line. Findings without a target line are +// summarized in the review body. +// +// Mirrors the TUI's batch-submit path (publishBatchReview) so headless +// and interactive runs produce equivalent reviews on GitHub. +func PostStructuredReview(prNumber, commitSHA string, findings []state.ReviewFinding) (posted int, err error) { + comments := make([]git.ReviewFindingComment, 0, len(findings)) + for _, f := range findings { + if f.File != "" && f.Line > 0 { + comments = append(comments, git.ReviewFindingComment{ + Path: f.File, + Line: f.Line, + Body: formatFindingMarkdown(f), + Side: "RIGHT", + }) + } + } + body := formatBatchBody(findings) + if err := git.SubmitReviewWithFindings(prNumber, commitSHA, body, comments); err != nil { + return 0, err + } + return len(comments), nil +} + +func formatFindingMarkdown(f state.ReviewFinding) string { + var b strings.Builder + fmt.Fprintf(&b, "**[%s/%s] %s**\n\n", f.Severity, f.Category, f.Title) + b.WriteString(f.Detail + "\n") + if f.Suggestion != "" { + fmt.Fprintf(&b, "\n> **Suggestion:** %s\n", f.Suggestion) + } + b.WriteString("\n---\n_Posted by [prr](https://github.com/andreujuanc/prr) AI review_") + return b.String() +} + +func formatBatchBody(findings []state.ReviewFinding) string { + counts := severityCounts(findings) + files := uniqueFindingFiles(findings) + var b strings.Builder + b.WriteString("## AI Review Summary\n\n") + fmt.Fprintf(&b, "**%d findings** across %d files: %s\n\n", len(findings), len(files), counts) + b.WriteString("---\n_Posted by [prr](https://github.com/andreujuanc/prr) AI review_") + return b.String() +} + +func severityCounts(findings []state.ReviewFinding) string { + counts := map[string]int{} + for _, f := range findings { + counts[f.Severity]++ + } + var parts []string + for _, sev := range []string{"critical", "high", "medium", "low", "nit"} { + if n, ok := counts[sev]; ok { + label := sev + if sev == "medium" { + label = "med" + } + parts = append(parts, fmt.Sprintf("%d %s", n, label)) + } + } + return strings.Join(parts, " · ") +} + +func uniqueFindingFiles(findings []state.ReviewFinding) []string { + seen := make(map[string]bool) + var files []string + for _, f := range findings { + if f.File != "" && !seen[f.File] { + seen[f.File] = true + files = append(files, f.File) + } + } + return files +} From 8ab10148ab2736c147ff950b03b45e95a85a3185 Mon Sep 17 00:00:00 2001 From: "Juan C. Andreu" Date: Mon, 25 May 2026 15:46:06 +0000 Subject: [PATCH 2/5] fix(config): mark anthropic Opus 4.7 as AOI-capable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The aoi-benchmark-rework run in 4e91b34 only flipped AOI: true on the claude-code Opus 4.7 row; the anthropic-direct row was missed. Same model weights, same capabilities — surface it in the AOI picker for users on the direct API. --- internal/config/known_models.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/config/known_models.go b/internal/config/known_models.go index c0ef1c6..61a9936 100644 --- a/internal/config/known_models.go +++ b/internal/config/known_models.go @@ -38,7 +38,7 @@ var knownModels = []KnownModel{ InputPricePer1M: 0.25, OutputPricePer1M: 1.50, Speed: "fast"}, // ── Anthropic (direct API) ───────────────────────────────────────── - {ID: "claude-opus-4-7", Label: "Claude Opus 4.7", Provider: "anthropic", Thinking: true, Review: true, + {ID: "claude-opus-4-7", Label: "Claude Opus 4.7", Provider: "anthropic", Thinking: true, Review: true, AOI: true, InputPricePer1M: 5.00, OutputPricePer1M: 25.00, Speed: "slow"}, {ID: "claude-sonnet-4-6", Label: "Claude Sonnet 4.6", Provider: "anthropic", Thinking: true, Review: true, AOI: true, InputPricePer1M: 3.00, OutputPricePer1M: 15.00, Speed: "medium"}, From 762347de1da09968a54f5293db336c7bab0ca477 Mon Sep 17 00:00:00 2001 From: "Juan C. Andreu" Date: Mon, 25 May 2026 17:14:25 +0000 Subject: [PATCH 3/5] fix(review): build synthesis findings deterministically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The synthesis system prompt listed "bug | security | performance | testing | style | architecture | docs" as allowed categories, but five of those are not in the embedded category slug set used by state.Category's UnmarshalJSON. Whenever the LLM took the prompt literally (e.g. picked "bug" for an error-handling finding), the whole ReviewOutput parse failed and the headless renderer fell through to dumping the raw JSON. Move findings construction out of the LLM: synthesis now writes only the four narrative fields it actually authors (summary, verdict, missing_tests, questions_for_author) and the findings array is built in Go from the post-recheck DeepFindings via BuildFindingsFromDeep — straight field copy, source_ids back to F-IDs, sorted by severity. ParseReviewOutput now hard-rejects empty summary and off-list verdict so the existing retry path engages on bad wrappers; if both attempts fail, RunSynthesis still returns a usable ReviewOutput with the deterministic findings and a verdict derived from severity counts. --- internal/ai/prompts/review_synthesis.md | 85 +++++----------------- internal/ai/review.go | 27 ++++++- internal/ai/review_test.go | 54 ++++++++++++++ internal/review/batch.go | 70 ++++++++++++++++-- internal/review/findings_from_deep.go | 65 +++++++++++++++++ internal/review/findings_from_deep_test.go | 82 +++++++++++++++++++++ internal/review/pipeline.go | 2 +- internal/ui/review.go | 2 +- 8 files changed, 307 insertions(+), 80 deletions(-) create mode 100644 internal/review/findings_from_deep.go create mode 100644 internal/review/findings_from_deep_test.go diff --git a/internal/ai/prompts/review_synthesis.md b/internal/ai/prompts/review_synthesis.md index bba7a5e..a498de1 100644 --- a/internal/ai/prompts/review_synthesis.md +++ b/internal/ai/prompts/review_synthesis.md @@ -1,9 +1,12 @@ You are a senior engineer producing the final, presentation-quality -synthesis of a PR review. The upstream phases — deep review (Phase 1) -plus adversarial recheck (Phase 1c) — already investigated, gathered -evidence, and filtered false positives. **You trust their output.** +wrapper around an already-completed PR review. The upstream phases — +deep review (Phase 1) plus adversarial recheck (Phase 1c) — already +investigated, gathered evidence, filtered false positives, and produced +the final list of findings. **You trust their output.** -Your job is **condense and present**, not to re-verify. +The findings list is built deterministically downstream from the +post-recheck data; you do NOT emit findings yourself. Your only job is +to author the four narrative fields that wrap them. You do NOT have tools. The investigation already happened. Do not attempt tool calls. @@ -13,36 +16,16 @@ attempt tool calls. You will receive: 1. PR metadata (title, description, branch info) 2. A list of changed files with adds/deletes -3. The post-recheck findings, each carrying a `finding_id` (e.g. `F-003`) - -You will reference each input finding via `source_ids` so consumers -can trace your output back to the underlying evidence. +3. The post-recheck findings text (for context only — do not re-emit them) ## Output -Produce a JSON object with these fields: +Produce a JSON object with EXACTLY these four fields and nothing else: ```json { "summary": "one paragraph capturing what the PR does and overall quality", "verdict": "approve | request_changes | comment", - "findings": [ - { - "severity": "critical | high | medium | low | nit", - "confidence_score": 78, - "confidence_reasoning": "carried from the source finding's reasoning, possibly trimmed", - "category": "bug | security | performance | testing | style | architecture | docs", - "source_ids": ["F-001", "F-007"], - "file": "path/to/file.go", - "line": 42, - "title": "short title", - "detail": "one or two sentences; consumers dereference source_ids for full evidence", - "suggestion": "concrete fix, code snippet preferred", - "cwe": "CWE-XXX (security findings only; omit otherwise)", - "exploitability": "trivial | moderate | difficult (security findings only)", - "impact": "critical | high | medium | low (security findings only)" - } - ], "missing_tests": ["behaviors that should be tested but aren't"], "questions_for_author": ["genuine ambiguities, not rhetorical"] } @@ -50,48 +33,18 @@ Produce a JSON object with these fields: ## Rules -- Be specific — every finding's `source_ids` MUST list one or more - IDs from the input. Do not paraphrase, do not invent IDs. -- Every input finding must be accounted for, either as a `findings` - entry or via the `source_ids` of a consolidated entry. Silently - dropping a finding is a contract violation. If the PR has Phase 1c - findings, your `findings` array cannot be empty. -- Consolidate when same root cause: multiple input findings sharing - one cause merge into one entry with all their IDs in `source_ids`, - using the highest severity from the group. -- `findings` sorted by severity: critical first, nit last. -- `file`/`line`: one representative location per finding (per-site - ranges live in the source deep findings — consumers dereference). -- Severity comes from upstream; don't adjust unless consolidation - warrants (multiple medium → systemic high). -- **Confidence is upstream-only — do NOT re-grade.** Carry - `confidence_score` and `confidence_reasoning` from the input - finding. On consolidation of multiple input findings into one, - use the LOWEST confidence_score among the group (uncertainty - dominates) and keep its reasoning. You do not have tools and - cannot verify; the upstream phases already did that work. -- `detail`: one or two sentences. Full evidence and triggers live in - the source deep findings. -- `suggestion` scope is absolute: do NOT propose new utilities, - helper functions, abstractions, refactors of adjacent code, or - pattern changes not already in the codebase. Mirror what recheck - output; do not expand. +- Do NOT include a `findings` field. The downstream pipeline builds it + from the rechecked deep findings — anything you put there would be + discarded. +- `summary`: one paragraph. State what the PR does and the overall + quality bar. If recheck returned findings, mention the dominant theme + (e.g. "several error-handling gaps"); don't enumerate them. +- `verdict`: `approve` when recheck returned no findings; otherwise + `comment` for low/medium-only sets and `request_changes` when any + critical/high finding survived recheck. - `missing_tests`: populate when the PR adds new behavior without - test coverage. Don't leave empty out of caution. + test coverage. Don't leave empty out of caution; don't pad either. - `questions_for_author`: populate when something is genuinely uncertain or needs author input. Don't leave empty out of caution. -- `cwe`/`exploitability`/`impact`: security findings only — omit - otherwise. -- If recheck returned no findings, return `verdict: "approve"` with - empty `findings`. Otherwise, every recheck finding must reach - output via `source_ids`. - Return ONLY the JSON object. No markdown fences, no prose, no preamble, no tool calls. - -## Severity definitions (for reference; do not re-grade) - -- **critical**: Data loss/corruption, RCE, auth bypass, SQL injection on sensitive data, SSRF to internal services, crashes in production, breaking API changes without migration. -- **high**: XSS, privilege escalation, hardcoded secrets, insecure deserialization, missing authorization, significant correctness bugs. -- **medium**: Open redirect, weak crypto, missing rate limiting, info disclosure, race conditions, perf issues on hot paths, logic bugs in auth/permission, missing tests for critical behavior. -- **low**: Defense-in-depth, minor readability, cold-path perf, doc gaps. -- **nit**: Cosmetic, formatting, naming preferences. diff --git a/internal/ai/review.go b/internal/ai/review.go index 7453ea0..1647f73 100644 --- a/internal/ai/review.go +++ b/internal/ai/review.go @@ -32,13 +32,20 @@ func ParseReviewOutput(raw string) *state.ReviewOutput { return nil } - // Validate minimum structure - if out.Summary == "" && out.Verdict == "" { + // Wrapper-shape validation. The 4-field synthesis schema requires a + // non-empty summary and a verdict in the allowed set. Empty/off-list + // values surface here as nil so RunSynthesis's retry path engages + // (and so the deterministic fallback can stamp deriveVerdict). + out.Summary = strings.TrimSpace(out.Summary) + if out.Summary == "" { + log.Printf("ParseReviewOutput: rejecting — summary is empty") return nil } - - // Normalize verdict out.Verdict = normalizeVerdict(out.Verdict) + if !isValidVerdict(out.Verdict) { + log.Printf("ParseReviewOutput: rejecting — verdict %q not in {approve, request_changes, comment}", out.Verdict) + return nil + } // Sort findings by severity sort.Slice(out.Findings, func(i, j int) bool { @@ -191,6 +198,8 @@ func findAllJSONObjects(s string) []string { } // normalizeVerdict normalizes verdict strings to canonical values. +// Returns the raw (lowercased, trimmed) input when no pattern matches — +// the caller (isValidVerdict) then rejects it. func normalizeVerdict(v string) string { v = strings.TrimSpace(strings.ToLower(v)) switch { @@ -204,3 +213,13 @@ func normalizeVerdict(v string) string { return v } } + +// isValidVerdict reports whether v is one of the three allowed verdicts. +// Called after normalizeVerdict at the parse boundary. +func isValidVerdict(v string) bool { + switch v { + case "approve", "request_changes", "comment": + return true + } + return false +} diff --git a/internal/ai/review_test.go b/internal/ai/review_test.go index 5cfb396..45606cd 100644 --- a/internal/ai/review_test.go +++ b/internal/ai/review_test.go @@ -263,6 +263,60 @@ Now let me finalize the review based on verified findings. } } +func TestParseReviewOutput_RejectsInvalidVerdict(t *testing.T) { + // After the synthesis prompt was trimmed to a 4-field wrapper, + // off-list verdicts must be rejected at the parse boundary so + // RunSynthesis's retry path engages. + tests := []string{ + `{"summary":"x","verdict":"looks good"}`, + `{"summary":"x","verdict":"lgtm"}`, + `{"summary":"x","verdict":""}`, + `{"summary":"x","verdict":"ship it"}`, + } + for _, raw := range tests { + t.Run(raw, func(t *testing.T) { + if result := ParseReviewOutput(raw); result != nil { + t.Errorf("expected nil for off-list verdict, got %+v", result) + } + }) + } +} + +func TestParseReviewOutput_RejectsEmptySummary(t *testing.T) { + tests := []string{ + `{"summary":"","verdict":"approve"}`, + `{"summary":" ","verdict":"approve"}`, + } + for _, raw := range tests { + t.Run(raw, func(t *testing.T) { + if result := ParseReviewOutput(raw); result != nil { + t.Errorf("expected nil for empty summary, got %+v", result) + } + }) + } +} + +func TestParseReviewOutput_WrapperOnly(t *testing.T) { + // The trimmed synthesis prompt asks the LLM for only these four + // fields — no findings array. Parser must accept that shape. + raw := `{ + "summary": "Solid PR.", + "verdict": "comment", + "missing_tests": ["edge case for empty input"], + "questions_for_author": ["Is the new flag intended for CI use?"] + }` + result := ParseReviewOutput(raw) + if result == nil { + t.Fatal("expected non-nil result for wrapper-only JSON") + } + if result.Verdict != "comment" || result.Summary != "Solid PR." { + t.Errorf("verdict/summary: %q / %q", result.Verdict, result.Summary) + } + if len(result.Findings) != 0 { + t.Errorf("expected 0 findings (downstream fills them), got %d", len(result.Findings)) + } +} + func TestParseReviewOutput_NilArraysNormalized(t *testing.T) { raw := `{"summary":"test","verdict":"approve"}` result := ParseReviewOutput(raw) diff --git a/internal/review/batch.go b/internal/review/batch.go index a849b29..2f27d81 100644 --- a/internal/review/batch.go +++ b/internal/review/batch.go @@ -671,7 +671,17 @@ type SynthesisResult struct { Structured *state.ReviewOutput } -// RunSynthesis runs Phase 2: synthesize all findings into a final review. +// RunSynthesis runs Phase 2: produce the narrative wrapper around the +// already-final findings list. +// +// The LLM is asked for only four fields — summary, verdict, missing_tests, +// questions_for_author. The findings array is built deterministically +// from deepFindings via BuildFindingsFromDeep, so even a totally +// malformed LLM response cannot drop or invalidate findings. +// +// deepFindings is the post-recheck source of truth. Pass an empty +// slice from legacy non-AOI paths — the returned Structured will then +// have an empty Findings list. func RunSynthesis( ctx context.Context, client ai.Client, @@ -680,6 +690,7 @@ func RunSynthesis( customInstructions string, allFindings string, fileFindings map[string]string, + deepFindings []state.DeepFinding, rr Reporter, ) (*SynthesisResult, error) { if ctx.Err() != nil { @@ -713,7 +724,7 @@ func RunSynthesis( } synthesisMessages := []ai.Message{ - {Role: "user", Content: "Synthesize the per-file findings into a final PR review. Use tools to verify any findings you are uncertain about. Return ONLY the JSON review object."}, + {Role: "user", Content: "Produce the narrative wrapper (summary, verdict, missing_tests, questions_for_author) for this PR review. The findings array is built downstream; do not emit it. Return ONLY the JSON object."}, } onToken := func(string) {} @@ -740,18 +751,20 @@ func RunSynthesis( // Retry with correction log.Printf("Synthesis: initial JSON parse failed, retrying with correction prompt") - // Detect whether the response looks like truncated JSON. + // Detect whether the response looks like truncated JSON. The + // 4-field wrapper is small, but a long summary can still get + // cut off; the heuristic checks for an opened-but-unclosed + // object containing the summary key. trimmed := strings.TrimSpace(summary) - isTruncated := (strings.Contains(trimmed, `"summary"`) || strings.Contains(trimmed, `"findings"`)) && - !strings.HasSuffix(trimmed, "}") + isTruncated := strings.Contains(trimmed, `"summary"`) && !strings.HasSuffix(trimmed, "}") - correctionHint := "Your response was not valid JSON. Please return ONLY a valid JSON object matching the schema specified in the system prompt. No markdown, no prose — just the raw JSON object starting with { and ending with }." + correctionHint := "Your response was not a valid wrapper. Return ONLY a JSON object with these four fields: summary (non-empty string), verdict (one of: approve | request_changes | comment), missing_tests (array of strings), questions_for_author (array of strings). No markdown, no prose, no findings field." if isTruncated { - correctionHint = "Your response was truncated — it appears the output was cut off before the JSON was complete. Please return the SAME review but be MORE CONCISE in your detail/suggestion fields so the full JSON fits. Return ONLY the JSON object, no markdown fences, no prose." + correctionHint = "Your response was truncated — the JSON was cut off before completion. Return the SAME wrapper but make the summary MORE CONCISE so the full JSON fits. Required fields: summary, verdict (approve|request_changes|comment), missing_tests, questions_for_author. No markdown, no prose, no findings." } correctionMessages := []ai.Message{ - {Role: "user", Content: "Synthesize the per-file findings into a final PR review. Return ONLY the JSON review object."}, + {Role: "user", Content: "Produce the narrative wrapper (summary, verdict, missing_tests, questions_for_author) for this PR review. Return ONLY the JSON object."}, {Role: "assistant", Content: summary}, {Role: "user", Content: correctionHint}, } @@ -765,6 +778,32 @@ func RunSynthesis( } } + // Attach the deterministic findings list and ensure a non-nil + // structured result even when the LLM produced unparseable JSON. + // The fallback verdict is derived from severity counts so consumers + // always get a usable ReviewOutput. + findings := BuildFindingsFromDeep(deepFindings) + if structured == nil { + structured = &state.ReviewOutput{ + Summary: "", + Verdict: deriveVerdict(findings), + Findings: findings, + MissingTests: []string{}, + QuestionsForAuthor: []string{}, + } + } else { + structured.Findings = findings + if structured.Verdict == "" { + structured.Verdict = deriveVerdict(findings) + } + if structured.MissingTests == nil { + structured.MissingTests = []string{} + } + if structured.QuestionsForAuthor == nil { + structured.QuestionsForAuthor = []string{} + } + } + return &SynthesisResult{ Review: &state.AIReview{ Summary: summary, @@ -774,6 +813,21 @@ func RunSynthesis( }, nil } +// deriveVerdict picks an approve/comment/request_changes verdict from +// the finding severities. Used as the fallback when the LLM didn't +// supply one (or failed to produce valid JSON entirely). +func deriveVerdict(findings []state.ReviewFinding) string { + if len(findings) == 0 { + return "approve" + } + for _, f := range findings { + if f.Severity == "critical" || f.Severity == "high" { + return "request_changes" + } + } + return "comment" +} + // ── Fallback batch runner ─────────────────────────────────────────────── // RunBatchesOnly reviews batches sequentially or in parallel (no synthesis). diff --git a/internal/review/findings_from_deep.go b/internal/review/findings_from_deep.go new file mode 100644 index 0000000..b744036 --- /dev/null +++ b/internal/review/findings_from_deep.go @@ -0,0 +1,65 @@ +package review + +import ( + "sort" + "strconv" + "strings" + + "github.com/andreujuanc/prr/internal/state" +) + +// BuildFindingsFromDeep maps post-recheck DeepFindings to ReviewFindings +// deterministically — no LLM involvement. Each DeepFinding becomes one +// ReviewFinding with source_ids=[FindingID]. Category, severity, and all +// other fields are copied straight through, so the output cannot be +// invalidated by a synthesis LLM picking off-list category slugs. +// +// Recheck (Phase 1c) is responsible for cross-file consolidation and +// dismissal; by the time findings reach here they are already the final +// set. Sorted by severity (critical first, nit last) to match the order +// the legacy LLM-authored synthesis used. +func BuildFindingsFromDeep(deep []state.DeepFinding) []state.ReviewFinding { + out := make([]state.ReviewFinding, 0, len(deep)) + for _, d := range deep { + out = append(out, state.ReviewFinding{ + Severity: d.Severity, + ConfidenceScore: d.ConfidenceScore, + ConfidenceReasoning: d.ConfidenceReasoning, + Category: d.Category, + File: d.File, + Line: firstLine(d.Lines), + Title: d.Title, + Detail: d.Description, + Suggestion: d.Suggestion, + SourceIDs: sourceIDs(d.FindingID), + }) + } + sort.SliceStable(out, func(i, j int) bool { + return out[i].SeverityRank() < out[j].SeverityRank() + }) + return out +} + +// firstLine parses the first integer out of a Lines field like "31" or +// "31-33". Returns 0 when no number is present. +func firstLine(lines string) int { + s := strings.TrimSpace(lines) + if s == "" { + return 0 + } + if i := strings.IndexAny(s, "-,: "); i >= 0 { + s = s[:i] + } + n, err := strconv.Atoi(strings.TrimSpace(s)) + if err != nil { + return 0 + } + return n +} + +func sourceIDs(id string) []string { + if id == "" { + return nil + } + return []string{id} +} diff --git a/internal/review/findings_from_deep_test.go b/internal/review/findings_from_deep_test.go new file mode 100644 index 0000000..fad3ac0 --- /dev/null +++ b/internal/review/findings_from_deep_test.go @@ -0,0 +1,82 @@ +package review + +import ( + "testing" + + "github.com/andreujuanc/prr/internal/state" +) + +func TestBuildFindingsFromDeep_FieldMapping(t *testing.T) { + deep := []state.DeepFinding{ + { + FindingID: "F-001", + File: "internal/foo.go", + Lines: "42", + Severity: "medium", + Category: state.MustParseCategory("correctness"), + Title: "Title", + Description: "Detail goes here", + Suggestion: "Do X", + ConfidenceScore: 85, + ConfidenceReasoning: "Saw it directly", + }, + } + + got := BuildFindingsFromDeep(deep) + if len(got) != 1 { + t.Fatalf("len(got)=%d, want 1", len(got)) + } + f := got[0] + if f.File != "internal/foo.go" || f.Line != 42 { + t.Errorf("file/line: got %s:%d", f.File, f.Line) + } + if f.Severity != "medium" || f.Category.String() != "correctness" { + t.Errorf("severity/category: got %s / %s", f.Severity, f.Category) + } + if f.Title != "Title" || f.Detail != "Detail goes here" || f.Suggestion != "Do X" { + t.Errorf("title/detail/suggestion mismatch: %+v", f) + } + if f.ConfidenceScore != 85 || f.ConfidenceReasoning != "Saw it directly" { + t.Errorf("confidence mismatch: %d / %q", f.ConfidenceScore, f.ConfidenceReasoning) + } + if len(f.SourceIDs) != 1 || f.SourceIDs[0] != "F-001" { + t.Errorf("source_ids: got %v", f.SourceIDs) + } +} + +func TestBuildFindingsFromDeep_LineRange(t *testing.T) { + tests := []struct { + in string + want int + }{ + {"31", 31}, + {"31-33", 31}, + {" 42 ", 42}, + {"", 0}, + {"abc", 0}, + } + for _, tt := range tests { + got := firstLine(tt.in) + if got != tt.want { + t.Errorf("firstLine(%q) = %d, want %d", tt.in, got, tt.want) + } + } +} + +func TestBuildFindingsFromDeep_SortsBySeverity(t *testing.T) { + deep := []state.DeepFinding{ + {FindingID: "F-1", Severity: "low", Title: "lo"}, + {FindingID: "F-2", Severity: "critical", Title: "crit"}, + {FindingID: "F-3", Severity: "medium", Title: "med"}, + } + got := BuildFindingsFromDeep(deep) + if got[0].Severity != "critical" || got[1].Severity != "medium" || got[2].Severity != "low" { + t.Errorf("sort: got %s/%s/%s", got[0].Severity, got[1].Severity, got[2].Severity) + } +} + +func TestBuildFindingsFromDeep_EmptyInput(t *testing.T) { + if got := BuildFindingsFromDeep(nil); len(got) != 0 { + t.Errorf("len(got)=%d, want 0", len(got)) + } +} diff --git a/internal/review/pipeline.go b/internal/review/pipeline.go index 2d4e487..6b8667b 100644 --- a/internal/review/pipeline.go +++ b/internal/review/pipeline.go @@ -1187,7 +1187,7 @@ func RunReviewCore( } synthResult, synthErr := RunSynthesis(ctx, reviewClient, opts.PRMeta, opts.RawDiffs, - enhancedInstructions, allFindings.String(), allFileFindings, rr) + enhancedInstructions, allFindings.String(), allFileFindings, deepFindings, rr) if synthErr != nil { return nil, synthErr } diff --git a/internal/ui/review.go b/internal/ui/review.go index d08dda8..1f6061a 100644 --- a/internal/ui/review.go +++ b/internal/ui/review.go @@ -512,7 +512,7 @@ func runSynthesis( ) tea.Msg { adapter := &reviewReporterAdapter{rr: rr} - synthResult, err := review.RunSynthesis(ctx, client, prMeta, rawDiffs, customInstructions, allFindings, fileFindings, adapter) + synthResult, err := review.RunSynthesis(ctx, client, prMeta, rawDiffs, customInstructions, allFindings, fileFindings, nil, adapter) if err != nil { return AIChatDoneMsg{Err: err} } From 1c08a94f77636a27186a66ecf0c54eb251b7ffee Mon Sep 17 00:00:00 2001 From: "Juan C. Andreu" Date: Mon, 25 May 2026 17:43:38 +0000 Subject: [PATCH 4/5] fix(review): retry deep calls once on parse failure runReviewCallWithRetry used to short-circuit on errReviewParse with the reasoning that "retrying the same prompt won't fix prose-in-response." In practice LLM sampling variance is wide enough that a second attempt often produces valid JSON, and losing a 45s deep-review call (with up to 10 AOIs for grouped calls) to a one-off prose response is too expensive to leave unrecovered. Drop the short-circuit so parse failures get the same one-retry treatment as transient API errors. Context cancellation still short-circuits. Also flesh out TestParseReviewOutput_WrapperOnly to assert the missing_tests and questions_for_author fields parse through (caught by a review pass against the previous commit). --- internal/ai/review_test.go | 6 ++++ internal/review/execute.go | 22 +++++++------ internal/review/execute_retry_test.go | 45 ++++++++++++++++++++++----- 3 files changed, 56 insertions(+), 17 deletions(-) diff --git a/internal/ai/review_test.go b/internal/ai/review_test.go index 45606cd..fd8e005 100644 --- a/internal/ai/review_test.go +++ b/internal/ai/review_test.go @@ -315,6 +315,12 @@ func TestParseReviewOutput_WrapperOnly(t *testing.T) { if len(result.Findings) != 0 { t.Errorf("expected 0 findings (downstream fills them), got %d", len(result.Findings)) } + if len(result.MissingTests) != 1 || result.MissingTests[0] != "edge case for empty input" { + t.Errorf("missing_tests: got %v", result.MissingTests) + } + if len(result.QuestionsForAuthor) != 1 || result.QuestionsForAuthor[0] != "Is the new flag intended for CI use?" { + t.Errorf("questions_for_author: got %v", result.QuestionsForAuthor) + } } func TestParseReviewOutput_NilArraysNormalized(t *testing.T) { diff --git a/internal/review/execute.go b/internal/review/execute.go index dbe95d2..df4adf5 100644 --- a/internal/review/execute.go +++ b/internal/review/execute.go @@ -350,10 +350,13 @@ func shouldAggregateFailReview(failed, total int) bool { const reviewRetryBackoff = 1500 * time.Millisecond // runReviewCallWithRetry executes one deep review call and retries once -// on transient errors after reviewRetryBackoff. Parse-shape failures -// (errReviewParse) and context cancellation short-circuit immediately — -// retrying the same prompt won't fix prose-in-response, and a cancelled -// context can't carry the retry anyway. +// on any non-cancellation error after reviewRetryBackoff. That covers +// both transient API errors (5xx, network blips) and parse-shape +// failures (errReviewParse: prose-only response, off-list category that +// failed json.Unmarshal). Sampling variance is enough that a second +// attempt often produces parseable JSON even with the same prompt. +// Context cancellation still short-circuits — a cancelled context +// can't carry the retry anyway. // // Why retry deep reviews at all? They're the most expensive LLM calls // in the pipeline; losing one to a 503 after 45s of work also loses @@ -378,9 +381,6 @@ func runReviewCallWithRetry( time.Since(start).Round(time.Millisecond)) return result, nil } - if errors.Is(err, errReviewParse) { - return nil, err - } if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) { return nil, err } @@ -390,8 +390,12 @@ func runReviewCallWithRetry( case <-ctx.Done(): return nil, ctx.Err() } - log.Printf("review: retrying call %d (%s %s/%s) after transient error: %v", - callIndex+1, call.Type, call.Category, call.Subcategory, err) + reason := "transient error" + if errors.Is(err, errReviewParse) { + reason = "parse failure" + } + log.Printf("review: retrying call %d (%s %s/%s) after %s: %v", + callIndex+1, call.Type, call.Category, call.Subcategory, reason, err) result, err = doReviewCall(ctx, client, call, opts, callIndex) if err == nil { log.Printf("review: call %d (%s %s/%s) completed in %v (retry succeeded)", diff --git a/internal/review/execute_retry_test.go b/internal/review/execute_retry_test.go index b992902..f5da3c0 100644 --- a/internal/review/execute_retry_test.go +++ b/internal/review/execute_retry_test.go @@ -146,24 +146,53 @@ func TestRunReviewCallWithRetry_RetriesTransient(t *testing.T) { } } -func TestRunReviewCallWithRetry_DoesNotRetryParseErrors(t *testing.T) { - // Parse failure (model emits prose instead of JSON). Retrying - // the same prompt won't fix the model's behavior — just doubles - // the (very expensive) deep-review token spend. +func TestRunReviewCallWithRetry_RetriesParseErrors(t *testing.T) { + // Parse failure (model emits prose instead of JSON) gets one + // retry. LLM sampling variance is enough that a second attempt + // with the same prompt often produces valid JSON, and losing a + // 45s deep-review call to a one-off prose response is expensive + // enough to justify the extra attempt. client := &stubClient{ - responses: []string{"I cannot provide that review."}, + responses: []string{ + "I cannot provide that review.", + "I cannot provide that review.", + }, } opts := ExecuteOptions{Mode: ModeAudit} _, err := runReviewCallWithRetry(context.Background(), client, buildIndivCall(), opts, 0) if err == nil { - t.Fatal("expected parse error") + t.Fatal("expected parse error after both attempts") } if !errors.Is(err, errReviewParse) { t.Errorf("expected errReviewParse sentinel; got: %v", err) } - if client.CallCount() != 1 { - t.Errorf("parse errors must NOT retry; got %d calls", client.CallCount()) + if client.CallCount() != 2 { + t.Errorf("parse errors must retry once; got %d calls", client.CallCount()) + } +} + +func TestRunReviewCallWithRetry_ParseErrorRecoversOnRetry(t *testing.T) { + // First attempt returns prose, second returns valid JSON — the + // happy case for parse-failure retry. Result must come from the + // successful retry. + client := &stubClient{ + responses: []string{ + "I cannot provide that review.", + validFindingResponse, + }, + } + opts := ExecuteOptions{Mode: ModeAudit} + + result, err := runReviewCallWithRetry(context.Background(), client, buildIndivCall(), opts, 0) + if err != nil { + t.Fatalf("retry should have recovered; got: %v", err) + } + if result == nil || len(result.Findings) != 1 { + t.Fatalf("expected 1 finding after retry; got %+v", result) + } + if client.CallCount() != 2 { + t.Errorf("expected 2 calls (one failed parse + one retry); got %d", client.CallCount()) } } From 431fbf4ab12f770f89f2aaaa73cf88e21ae5c1ac Mon Sep 17 00:00:00 2001 From: "Juan C. Andreu" Date: Mon, 25 May 2026 17:54:01 +0000 Subject: [PATCH 5/5] feat(review): enrich posted review body with summary, missing tests, questions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The posted GitHub review body used to be a single line of severity counts. The summary paragraph, missing_tests, and questions_for_author fields that synthesis already authors weren't being surfaced to the PR reader — wasted context. Thread the full *state.ReviewOutput through PostStructuredReview and publishBatchReview (the TUI's Ctrl+S batch-submit) so both posting paths render the same enriched body: summary paragraph, severity counts, then optional Missing tests / Questions for the author sections. Empty sections are skipped. --- cmd/prr/main.go | 2 +- internal/review/publish.go | 44 ++++++++++++++++++++++++++---------- internal/ui/confirm_modal.go | 26 +++++++++++++++++---- internal/ui/model.go | 15 +++++++----- 4 files changed, 63 insertions(+), 24 deletions(-) diff --git a/cmd/prr/main.go b/cmd/prr/main.go index f7f21c3..d24d94b 100644 --- a/cmd/prr/main.go +++ b/cmd/prr/main.go @@ -761,7 +761,7 @@ func runReview(debug bool, args []string) { if !ok { fmt.Fprintf(os.Stderr, " %s post cancelled\n\n", cliDim.Render("--post:")) } else { - n, err := review.PostStructuredReview(prNumber, result.PR.HeadRefOid, result.StructuredReview.Findings) + n, err := review.PostStructuredReview(prNumber, result.PR.HeadRefOid, result.StructuredReview) if err != nil { printError(fmt.Errorf("posting review: %w", err)) os.Exit(1) diff --git a/internal/review/publish.go b/internal/review/publish.go index a8ac3a3..5c4187d 100644 --- a/internal/review/publish.go +++ b/internal/review/publish.go @@ -8,16 +8,20 @@ import ( "github.com/andreujuanc/prr/internal/state" ) -// PostStructuredReview submits the synthesized findings as a single +// PostStructuredReview submits the synthesized review as a single // GitHub PR review (event=COMMENT) with one inline comment per finding -// that has a valid file and line. Findings without a target line are -// summarized in the review body. +// that has a valid file and line. The review body carries the summary +// paragraph, severity counts, missing tests, and questions for the +// author — the narrative context that doesn't anchor to a line. // // Mirrors the TUI's batch-submit path (publishBatchReview) so headless // and interactive runs produce equivalent reviews on GitHub. -func PostStructuredReview(prNumber, commitSHA string, findings []state.ReviewFinding) (posted int, err error) { - comments := make([]git.ReviewFindingComment, 0, len(findings)) - for _, f := range findings { +func PostStructuredReview(prNumber, commitSHA string, sr *state.ReviewOutput) (posted int, err error) { + if sr == nil { + return 0, fmt.Errorf("nil ReviewOutput") + } + comments := make([]git.ReviewFindingComment, 0, len(sr.Findings)) + for _, f := range sr.Findings { if f.File != "" && f.Line > 0 { comments = append(comments, git.ReviewFindingComment{ Path: f.File, @@ -27,7 +31,7 @@ func PostStructuredReview(prNumber, commitSHA string, findings []state.ReviewFin }) } } - body := formatBatchBody(findings) + body := formatBatchBody(sr) if err := git.SubmitReviewWithFindings(prNumber, commitSHA, body, comments); err != nil { return 0, err } @@ -45,13 +49,29 @@ func formatFindingMarkdown(f state.ReviewFinding) string { return b.String() } -func formatBatchBody(findings []state.ReviewFinding) string { - counts := severityCounts(findings) - files := uniqueFindingFiles(findings) +func formatBatchBody(sr *state.ReviewOutput) string { + counts := severityCounts(sr.Findings) + files := uniqueFindingFiles(sr.Findings) var b strings.Builder b.WriteString("## AI Review Summary\n\n") - fmt.Fprintf(&b, "**%d findings** across %d files: %s\n\n", len(findings), len(files), counts) - b.WriteString("---\n_Posted by [prr](https://github.com/andreujuanc/prr) AI review_") + if sr.Summary != "" { + b.WriteString(sr.Summary) + b.WriteString("\n\n") + } + fmt.Fprintf(&b, "**%d findings** across %d files: %s\n", len(sr.Findings), len(files), counts) + if len(sr.MissingTests) > 0 { + b.WriteString("\n### Missing tests\n") + for _, t := range sr.MissingTests { + fmt.Fprintf(&b, "- %s\n", t) + } + } + if len(sr.QuestionsForAuthor) > 0 { + b.WriteString("\n### Questions for the author\n") + for _, q := range sr.QuestionsForAuthor { + fmt.Fprintf(&b, "- %s\n", q) + } + } + b.WriteString("\n---\n_Posted by [prr](https://github.com/andreujuanc/prr) AI review_") return b.String() } diff --git a/internal/ui/confirm_modal.go b/internal/ui/confirm_modal.go index cef575d..438569d 100644 --- a/internal/ui/confirm_modal.go +++ b/internal/ui/confirm_modal.go @@ -301,12 +301,28 @@ func formatFindingAsMarkdown(f state.ReviewFinding) string { } // formatBatchReviewBody formats the summary body for a batch review submission. -func formatBatchReviewBody(findings []state.ReviewFinding) string { +func formatBatchReviewBody(sr *state.ReviewOutput) string { var b strings.Builder b.WriteString("## AI Review Summary\n\n") - counts := severityCounts(findings) - files := uniqueFiles(findings) - b.WriteString(fmt.Sprintf("**%d findings** across %d files: %s\n\n", len(findings), len(files), counts)) - b.WriteString("---\n_Posted by [prr](https://github.com/andreujuanc/prr) AI review_") + if sr.Summary != "" { + b.WriteString(sr.Summary) + b.WriteString("\n\n") + } + counts := severityCounts(sr.Findings) + files := uniqueFiles(sr.Findings) + b.WriteString(fmt.Sprintf("**%d findings** across %d files: %s\n", len(sr.Findings), len(files), counts)) + if len(sr.MissingTests) > 0 { + b.WriteString("\n### Missing tests\n") + for _, t := range sr.MissingTests { + fmt.Fprintf(&b, "- %s\n", t) + } + } + if len(sr.QuestionsForAuthor) > 0 { + b.WriteString("\n### Questions for the author\n") + for _, q := range sr.QuestionsForAuthor { + fmt.Fprintf(&b, "- %s\n", q) + } + } + b.WriteString("\n---\n_Posted by [prr](https://github.com/andreujuanc/prr) AI review_") return b.String() } diff --git a/internal/ui/model.go b/internal/ui/model.go index 75b43f1..166b35e 100644 --- a/internal/ui/model.go +++ b/internal/ui/model.go @@ -673,10 +673,13 @@ func publishFindingAsComment(prNumber, commitSHA string, f state.ReviewFinding) } } -func publishBatchReview(prNumber, commitSHA string, findings []state.ReviewFinding) tea.Cmd { +func publishBatchReview(prNumber, commitSHA string, sr *state.ReviewOutput) tea.Cmd { return func() tea.Msg { - comments := make([]git.ReviewFindingComment, 0, len(findings)) - for _, f := range findings { + if sr == nil { + return FindingsBatchPublishedMsg{Err: fmt.Errorf("nil ReviewOutput")} + } + comments := make([]git.ReviewFindingComment, 0, len(sr.Findings)) + for _, f := range sr.Findings { if f.File != "" && f.Line > 0 { comments = append(comments, git.ReviewFindingComment{ Path: f.File, @@ -686,7 +689,7 @@ func publishBatchReview(prNumber, commitSHA string, findings []state.ReviewFindi }) } } - body := formatBatchReviewBody(findings) + body := formatBatchReviewBody(sr) err := git.SubmitReviewWithFindings(prNumber, commitSHA, body, comments) return FindingsBatchPublishedMsg{Count: len(comments), Err: err} } @@ -3332,8 +3335,8 @@ func (m *Model) executeConfirmAction() tea.Cmd { return publishFindingAsComment(m.prNumber, m.pr.HeadRefOid, *modal.finding) } case confirmBatchReview: - if len(modal.findings) > 0 { - return publishBatchReview(m.prNumber, m.pr.HeadRefOid, modal.findings) + if len(modal.findings) > 0 && m.reviewState != nil && m.reviewState.Review != nil && m.reviewState.Review.Structured != nil { + return publishBatchReview(m.prNumber, m.pr.HeadRefOid, m.reviewState.Review.Structured) } case confirmPRComment: if modal.finding != nil {