Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
50 changes: 2 additions & 48 deletions cmd/prr/render.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,20 +13,18 @@ import (
func renderMarkdown(review types.Review, prID int, prURL string, issues []types.RelatedIssue) string {
var builder strings.Builder

issueSummary, prSummary := splitIssueAndPRSummaries(review.Summary)

builder.WriteString("## Summary\n")
builder.WriteString(renderPRLine(prID, prURL))
builder.WriteString("\n")
builder.WriteString(renderIssuesLine(issues))
builder.WriteString("\n\n")

builder.WriteString("### Issue summary\n")
builder.WriteString(issueSummary)
builder.WriteString(review.IssueSummary)
builder.WriteString("\n\n")

builder.WriteString("### PR summary\n")
builder.WriteString(prSummary)
builder.WriteString(review.PRSummary)
builder.WriteString("\n\n")

builder.WriteString("## Review\n")
Expand Down Expand Up @@ -152,47 +150,3 @@ func buildPRURL(repoURL string, prID int) string {
parsed.Path = strings.TrimSuffix(parsed.Path, "/") + "/pull/" + strconv.Itoa(prID)
return parsed.String()
}

func splitIssueAndPRSummaries(summary string) (string, string) {
paragraphs := splitParagraphs(summary)
if len(paragraphs) == 0 {
return "No issue summary provided.", "No PR summary provided."
}

issueParas := paragraphs
if len(issueParas) > 2 {
issueParas = issueParas[:2]
}

prStart := len(issueParas)
prParas := []string{}
if prStart < len(paragraphs) {
prParas = paragraphs[prStart:]
}
if len(prParas) > 2 {
prParas = prParas[:2]
}
if len(prParas) == 0 {
prParas = issueParas
}

return strings.Join(issueParas, "\n\n"), strings.Join(prParas, "\n\n")
}

func splitParagraphs(text string) []string {
trimmed := strings.TrimSpace(text)
if trimmed == "" {
return nil
}

raw := strings.Split(trimmed, "\n\n")
paragraphs := make([]string, 0, len(raw))
for _, para := range raw {
cleaned := strings.TrimSpace(para)
if cleaned == "" {
continue
}
paragraphs = append(paragraphs, cleaned)
}
return paragraphs
}
36 changes: 25 additions & 11 deletions cmd/prr/review_render_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,11 @@ func TestReviewCommandEmitsStructuredJSONAndKeepsDiagnosticsOffStdout(t *testing
t.Fatalf("expected valid review JSON, got %v", err)
}

if payload["summary"] == "" {
t.Fatalf("expected summary field in review JSON")
if payload["issueSummary"] == "" {
t.Fatalf("expected issueSummary field in review JSON")
}
if payload["prSummary"] == "" {
t.Fatalf("expected prSummary field in review JSON")
}
if _, ok := payload["risk"].(map[string]any); !ok {
t.Fatalf("expected risk object in review JSON")
Expand Down Expand Up @@ -209,8 +212,11 @@ func TestReviewCommandAcceptsPRURLArgument(t *testing.T) {
if err := json.Unmarshal(bytes.TrimSpace(stdout.Bytes()), &payload); err != nil {
t.Fatalf("expected valid review JSON, got %v", err)
}
if payload["summary"] == "" {
t.Fatalf("expected summary in review JSON")
if payload["issueSummary"] == "" {
t.Fatalf("expected issueSummary in review JSON")
}
if payload["prSummary"] == "" {
t.Fatalf("expected prSummary in review JSON")
}
}

Expand Down Expand Up @@ -264,8 +270,11 @@ func TestReviewCommandAcceptsPipedCheckoutJSONWithoutArgs(t *testing.T) {
if err := json.Unmarshal(bytes.TrimSpace(stdout.Bytes()), &payload); err != nil {
t.Fatalf("expected valid review JSON, got %v", err)
}
if payload["summary"] == "" {
t.Fatalf("expected summary in review JSON")
if payload["issueSummary"] == "" {
t.Fatalf("expected issueSummary in review JSON")
}
if payload["prSummary"] == "" {
t.Fatalf("expected prSummary in review JSON")
}
}

Expand Down Expand Up @@ -325,8 +334,11 @@ func TestReviewCommandBypassesSetupWithAuthoritativeCheckoutJSON(t *testing.T) {
if err := json.Unmarshal(bytes.TrimSpace(stdout.Bytes()), &payload); err != nil {
t.Fatalf("expected valid review JSON, got %v", err)
}
if payload["summary"] == "" {
t.Fatalf("expected summary in review JSON")
if payload["issueSummary"] == "" {
t.Fatalf("expected issueSummary in review JSON")
}
if payload["prSummary"] == "" {
t.Fatalf("expected prSummary in review JSON")
}
}

Expand Down Expand Up @@ -455,7 +467,8 @@ func TestReviewCommandEmitsDeterministicJSONShape(t *testing.T) {
reviewEngineFactory = func() engine.ReviewEngine {
return reviewEngineFunc(func(_ context.Context, _ engine.ReviewInput) (types.Review, error) {
return types.Review{
Summary: "Deterministic review",
IssueSummary: "Deterministic issue summary",
PRSummary: "Deterministic PR summary",
Risk: types.Risk{
Score: 0.25,
Reasons: []string{"Stable fixture"},
Expand Down Expand Up @@ -484,7 +497,7 @@ func TestReviewCommandEmitsDeterministicJSONShape(t *testing.T) {
t.Fatalf("expected review command to succeed, got %v", err)
}

const expected = "{\"summary\":\"Deterministic review\",\"risk\":{\"score\":0.25,\"reasons\":[\"Stable fixture\"]},\"findings\":[{\"id\":\"F001\",\"file\":\"a.go\",\"line\":7,\"severity\":\"important\",\"category\":\"tests\",\"message\":\"Add coverage\",\"suggestion\":\"Add assertions\"}],\"checklist\":[\"Run CI\"]}\n"
const expected = "{\"issueSummary\":\"Deterministic issue summary\",\"prSummary\":\"Deterministic PR summary\",\"risk\":{\"score\":0.25,\"reasons\":[\"Stable fixture\"]},\"findings\":[{\"id\":\"F001\",\"file\":\"a.go\",\"line\":7,\"severity\":\"important\",\"category\":\"tests\",\"message\":\"Add coverage\",\"suggestion\":\"Add assertions\"}],\"checklist\":[\"Run CI\"]}\n"
if stdout.String() != expected {
t.Fatalf("expected deterministic JSON output.\nwant: %q\n got: %q", expected, stdout.String())
}
Expand Down Expand Up @@ -577,7 +590,8 @@ func (f reviewEngineFunc) Review(ctx context.Context, input engine.ReviewInput)

func deterministicReview() types.Review {
return types.Review{
Summary: "Deterministic review",
IssueSummary: "Deterministic issue summary",
PRSummary: "Deterministic PR summary",
Risk: types.Risk{
Score: 0.25,
Reasons: []string{"Stable fixture"},
Expand Down
24 changes: 13 additions & 11 deletions internal/engine/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,8 @@ func marshalBundlePayload(bundle types.BundleV1, reviewInstructions string) (str
2) Treat that JSON object as the complete review input.
3) Return ONLY valid JSON using this exact schema (no markdown fences or extra prose):
{
"summary": string,
"issueSummary": string,
"prSummary": string,
"risk": {"score": number, "reasons": string[]},
"findings": [
{
Expand All @@ -222,14 +223,12 @@ func marshalBundlePayload(bundle types.BundleV1, reviewInstructions string) (str
],
"checklist": string[]
}
4) summary MUST be concise and deterministic with:
- Issue summary in at most 2 paragraphs total across all linked issues.
- PR summary in at most 2 paragraphs focused on what changed (not critique).
- If needed, separate issue and PR summary blocks using exactly one blank line.
5) findings must support final markdown grouping under headings:
4) issueSummary MUST be derived ONLY from the linked issues in the bundle. It MUST NOT contain information from the PR diff or changed files. Summarise what problem(s) the issues describe in at most 2 paragraphs. If there are no linked issues, set issueSummary to "No linked issues."
5) prSummary MUST be derived ONLY from the PR diff and changed files. It MUST NOT contain information from the linked issues. Summarise what changes have been made in at most 2 paragraphs.
6) findings must support final markdown grouping under headings:
Blocker, Important, Suggestion, Nitpick (map Nitpick to severity "nit").
6) risk.score MUST be a decimal number between 0 and 1 inclusive.
7) Be deterministic and concise.`)
7) risk.score MUST be a decimal number between 0 and 1 inclusive.
8) Be deterministic and concise.`)

stdinEnvelope := strings.Join([]string{
ri,
Expand Down Expand Up @@ -305,13 +304,16 @@ func extractJSONObject(raw string) (string, error) {
}

func whatIfReview(bundle types.BundleV1) types.Review {
summary := "what-if: review agent was not executed"
issueSummary := "what-if: review agent was not executed"
prSummary := "what-if: review agent was not executed"
if bundle.PRID > 0 {
summary = fmt.Sprintf("what-if: review agent was not executed for PR #%d", bundle.PRID)
issueSummary = fmt.Sprintf("what-if: review agent was not executed for PR #%d", bundle.PRID)
prSummary = fmt.Sprintf("what-if: review agent was not executed for PR #%d", bundle.PRID)
}

return types.Review{
Summary: summary,
IssueSummary: issueSummary,
PRSummary: prSummary,
Risk: types.Risk{
Score: 0,
Reasons: []string{"What-if mode skips external agent execution."},
Expand Down
21 changes: 12 additions & 9 deletions internal/engine/engine_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ func TestCLIAdapterBuildsCommandWithStdinEnvelopeModelAndWorkDir(t *testing.T) {
capturedArgs = append([]string{}, args...)
capturedCwd = cwd
capturedStdin = stdinPayload
return commandResult{Stdout: `{"summary":"ok","risk":{"score":0.1,"reasons":["low"]},"findings":[],"checklist":["run tests"]}`}, nil
return commandResult{Stdout: `{"issueSummary":"issue ok","prSummary":"pr ok","risk":{"score":0.1,"reasons":["low"]},"findings":[],"checklist":["run tests"]}`}, nil
}},
}

Expand Down Expand Up @@ -106,7 +106,7 @@ func TestCLIAdapterWhatIfSkipsExecutionAndReturnsStructuredReview(t *testing.T)
if called {
t.Fatalf("expected no external execution in what-if mode")
}
if review.Summary == "" || review.Risk.Reasons == nil || review.Checklist == nil {
if review.IssueSummary == "" || review.PRSummary == "" || review.Risk.Reasons == nil || review.Checklist == nil {
t.Fatalf("expected structured review response in what-if mode")
}
if len(logs) == 0 {
Expand All @@ -122,7 +122,7 @@ func TestCLIAdapterParsesMixedTextAndJSONOutput(t *testing.T) {
adapter := &CLIAgentAdapter{
config: DefaultAgentConfig(),
runner: fakeRunner{run: func(_ context.Context, _ string, _ []string, _ string, _ string) (commandResult, error) {
return commandResult{Stdout: "info line\n{\"summary\":\"ok\",\"risk\":{\"score\":0.1,\"reasons\":[\"low\"]},\"findings\":[],\"checklist\":[\"run tests\"]}\ntrailing"}, nil
return commandResult{Stdout: "info line\n{\"issueSummary\":\"issue ok\",\"prSummary\":\"pr ok\",\"risk\":{\"score\":0.1,\"reasons\":[\"low\"]},\"findings\":[],\"checklist\":[\"run tests\"]}\ntrailing"}, nil
}},
}

Expand All @@ -133,16 +133,19 @@ func TestCLIAdapterParsesMixedTextAndJSONOutput(t *testing.T) {
if err != nil {
t.Fatalf("expected parser to extract JSON object, got %v", err)
}
if review.Summary != "ok" {
t.Fatalf("expected parsed summary 'ok', got %q", review.Summary)
if review.IssueSummary != "issue ok" {
t.Fatalf("expected parsed issueSummary 'issue ok', got %q", review.IssueSummary)
}
if review.PRSummary != "pr ok" {
t.Fatalf("expected parsed prSummary 'pr ok', got %q", review.PRSummary)
}
}

func TestCLIAdapterRejectsOutOfRangeRiskScore(t *testing.T) {
adapter := &CLIAgentAdapter{
config: DefaultAgentConfig(),
runner: fakeRunner{run: func(_ context.Context, _ string, _ []string, _ string, _ string) (commandResult, error) {
return commandResult{Stdout: `{"summary":"ok","risk":{"score":4,"reasons":["high churn"]},"findings":[{"id":"F001","file":"a.go","line":7,"severity":"important","category":"tests","message":"m","suggestion":"s"}],"checklist":["run tests"]}`}, nil
return commandResult{Stdout: `{"issueSummary":"issue ok","prSummary":"pr ok","risk":{"score":4,"reasons":["high churn"]},"findings":[{"id":"F001","file":"a.go","line":7,"severity":"important","category":"tests","message":"m","suggestion":"s"}],"checklist":["run tests"]}`}, nil
}},
}

Expand Down Expand Up @@ -193,7 +196,7 @@ func TestCLIAdapterVerboseLogsCopilotOutput(t *testing.T) {
config: DefaultAgentConfig(),
runner: fakeRunner{run: func(_ context.Context, _ string, _ []string, _ string, _ string) (commandResult, error) {
return commandResult{
Stdout: "{\"summary\":\"ok\",\"risk\":{\"score\":0.1,\"reasons\":[\"low\"]},\"findings\":[],\"checklist\":[\"run tests\"]}",
Stdout: "{\"issueSummary\":\"issue ok\",\"prSummary\":\"pr ok\",\"risk\":{\"score\":0.1,\"reasons\":[\"low\"]},\"findings\":[],\"checklist\":[\"run tests\"]}",
Stderr: "copilot diagnostic",
}, nil
}},
Expand Down Expand Up @@ -347,7 +350,7 @@ func TestCLIAdapterUsesCustomReviewInstructions(t *testing.T) {
config: DefaultAgentConfig(),
runner: fakeRunner{run: func(_ context.Context, _ string, _ []string, _ string, stdinPayload string) (commandResult, error) {
capturedStdin = stdinPayload
return commandResult{Stdout: `{"summary":"ok","risk":{"score":0.1,"reasons":["low"]},"findings":[],"checklist":["run tests"]}`}, nil
return commandResult{Stdout: `{"issueSummary":"issue ok","prSummary":"pr ok","risk":{"score":0.1,"reasons":["low"]},"findings":[],"checklist":["run tests"]}`}, nil
}},
}

Expand Down Expand Up @@ -377,7 +380,7 @@ func TestCLIAdapterUsesDefaultReviewInstructionsWhenEmpty(t *testing.T) {
config: DefaultAgentConfig(),
runner: fakeRunner{run: func(_ context.Context, _ string, _ []string, _ string, stdinPayload string) (commandResult, error) {
capturedStdin = stdinPayload
return commandResult{Stdout: `{"summary":"ok","risk":{"score":0.1,"reasons":["low"]},"findings":[],"checklist":["run tests"]}`}, nil
return commandResult{Stdout: `{"issueSummary":"issue ok","prSummary":"pr ok","risk":{"score":0.1,"reasons":["low"]},"findings":[],"checklist":["run tests"]}`}, nil
}},
}

Expand Down
20 changes: 13 additions & 7 deletions internal/types/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,11 @@ type Finding struct {
}

type Review struct {
Summary string `json:"summary"`
Risk Risk `json:"risk"`
Findings []Finding `json:"findings"`
Checklist []string `json:"checklist"`
IssueSummary string `json:"issueSummary"`
PRSummary string `json:"prSummary"`
Risk Risk `json:"risk"`
Findings []Finding `json:"findings"`
Checklist []string `json:"checklist"`
}

func NormalizeAndValidateReviewOutput(input Review) (Review, error) {
Expand All @@ -50,9 +51,14 @@ func NormalizeAndValidateReview(input Review) (Review, error) {
func validateReview(input Review, wrap wrapFunc, allowMissingID bool, validateRiskRange bool, label string) (Review, error) {
review := input

review.Summary = strings.TrimSpace(review.Summary)
if review.Summary == "" {
return Review{}, wrap(label+" is missing summary", nil)
review.IssueSummary = strings.TrimSpace(review.IssueSummary)
if review.IssueSummary == "" {
return Review{}, wrap(label+" is missing issueSummary", nil)
}

review.PRSummary = strings.TrimSpace(review.PRSummary)
if review.PRSummary == "" {
return Review{}, wrap(label+" is missing prSummary", nil)
}

if validateRiskRange {
Expand Down
Loading
Loading