Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
102 changes: 101 additions & 1 deletion cmd/prr/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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" {
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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=<mode> full (default) reviews every file; aoi-only skips files without AOIs\n\n")
fmt.Fprintf(os.Stderr, " --review-mode=<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 {
Expand Down
85 changes: 19 additions & 66 deletions internal/ai/prompts/review_synthesis.md
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -13,85 +16,35 @@ 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"]
}
```

## 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.
27 changes: 23 additions & 4 deletions internal/ai/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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
}
54 changes: 54 additions & 0 deletions internal/ai/review_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Comment thread
andreujuanc marked this conversation as resolved.
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)
Expand Down
2 changes: 1 addition & 1 deletion internal/config/known_models.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand Down
Loading
Loading