fix(#2054): synthesize review body when findings contradict summary

internal/cli/postreview.go

@@ -87,6 +87,14 @@ has moved, a stale-head failure is posted instead.`,
 				return fmt.Errorf("parsing review result: %w", err)
 			}
 
+			// Ensure the summary body is consistent with the
+			// verdict and findings. A stale or multi-run scenario
+			// can produce a body that says "No findings" while the
+			// action is request-changes with critical findings.
+			if patched := ensureBodyFindingsConsistency(&parsed); patched {
+				printer.StepWarn("Review body was inconsistent with findings — synthesized body from structured findings")
+			}
+
 			// CLI flag takes precedence over JSON field.
 			if headSHA != "" {
 				parsed.HeadSHA = headSHA
@@ -506,6 +514,101 @@ func minimizeStaleReviews(ctx context.Context, client forge.Client, user string,
 	printer.StepDone("Stale reviews minimized")
 }
 
+// ensureBodyFindingsConsistency detects when the review body omits
+// significant findings despite the action mapping to REQUEST_CHANGES.
+// Instead of regex-patching individual phrases (which is fragile —
+// see #2055), it checks whether the body references any critical/high
+// finding categories. If none are referenced, the body is replaced
+// entirely with one synthesized from the structured findings array.
+// Returns true if the body was replaced.
+func ensureBodyFindingsConsistency(result *ReviewResult) bool {
+	if result == nil || len(result.Findings) == 0 {
+		return false
+	}
+
+	event, ok := reviewActionToEvent(result.Action)
+	if !ok || event != "REQUEST_CHANGES" {
+		return false
+	}
+
+	// Collect critical/high findings — these must be reflected in the body.
+	var significant []ReviewFinding
+	for _, f := range result.Findings {
+		switch strings.ToLower(f.Severity) {
+		case "critical", "high":
+			significant = append(significant, f)
+		}
+	}
+	if len(significant) == 0 {
+		return false
+	}
+
+	// Check whether the body already references any significant finding.
+	// A body is considered consistent if it mentions at least one
+	// critical/high finding's category. Categories are hyphenated tokens
+	// like "logic-error", "auth-bypass", "missing-test" — specific enough
+	// to avoid false positives against natural prose.
+	bodyLower := strings.ToLower(result.Body)
+	for _, f := range significant {
+		if f.Category != "" && strings.Contains(bodyLower, strings.ToLower(f.Category)) {
+			return false
+		}
+	}
+
+	// Body does not reference any significant findings — synthesize a
+	// complete replacement from the structured findings array.
+	result.Body = synthesizeReviewBody(result.Findings)
+	return true
+}
+
+// synthesizeReviewBody builds a review comment body from the structured
+// findings array, following the format defined in the pr-review skill
+// (step 7). Findings are grouped by severity level with only populated
+// severity sections included.
+func synthesizeReviewBody(findings []ReviewFinding) string {
+	// Group findings by severity.
+	order := []string{"critical", "high", "medium", "low", "info"}
+	groups := make(map[string][]ReviewFinding)
+	for _, f := range findings {
+		sev := strings.ToLower(f.Severity)
+		groups[sev] = append(groups[sev], f)
+	}
+
+	var b strings.Builder
+	b.WriteString("## Review\n\n### Findings\n")
+
+	for _, sev := range order {
+		fs, ok := groups[sev]
+		if !ok {
+			continue
+		}
+		// Title-case the severity for the section heading.
+		heading := strings.ToUpper(sev[:1]) + sev[1:]
+		fmt.Fprintf(&b, "\n#### %s\n\n", heading)
+		for _, f := range fs {
+			b.WriteString("- **[")
+			b.WriteString(f.Category)
+			b.WriteString("]**")
+			if f.File != "" {
+				fmt.Fprintf(&b, " `%s", f.File)
+				if f.Line > 0 {
+					fmt.Fprintf(&b, ":%d", f.Line)
+				}
+				b.WriteString("`")
+			}
+			b.WriteString(" — ")
+			b.WriteString(strings.TrimSpace(f.Description))
+			if f.Remediation != "" {
+				b.WriteString("\n  Remediation: ")
+				b.WriteString(strings.TrimSpace(f.Remediation))
+			}
+			b.WriteString("\n")
+		}
+	}
+
+	return b.String()
+}
+
 // parseReviewResult attempts to parse the body as a JSON ReviewResult.
 // If parsing fails, treats the entire input as a plain-text body.
 // Returns an error if the JSON is valid but the body field is empty

internal/cli/postreview_test.go

@@ -7,6 +7,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"io"
+	"strings"
 	"testing"
 
 	"github.com/fullsend-ai/fullsend/internal/forge"
@@ -1001,3 +1002,189 @@ func TestPostApprovedFollowUpIssues_DisabledIsNoop(t *testing.T) {
 	err := postApprovedFollowUpIssues(context.Background(), "acme", "repo", 9, parsed, printer)
 	require.NoError(t, err)
 }
+
+func TestEnsureBodyFindingsConsistency_SynthesizesBody(t *testing.T) {
+	result := ReviewResult{
+		Action: "request-changes",
+		Body:   "## Review\n### Findings\nNo findings.",
+		Findings: []ReviewFinding{
+			{
+				Severity:    "critical",
+				Category:    "logic-error",
+				File:        "pipeline.yaml",
+				Line:        42,
+				Description: "CEL expression uses wrong operator.",
+				Remediation: "Use && instead of ||.",
+			},
+		},
+	}
+
+	patched := ensureBodyFindingsConsistency(&result)
+	assert.True(t, patched)
+	// Body should be entirely replaced (not regex-patched).
+	assert.NotContains(t, result.Body, "No findings")
+	assert.Contains(t, result.Body, "## Review")
+	assert.Contains(t, result.Body, "### Findings")
+	assert.Contains(t, result.Body, "#### Critical")
+	assert.Contains(t, result.Body, "logic-error")
+	assert.Contains(t, result.Body, "pipeline.yaml:42")
+	assert.Contains(t, result.Body, "CEL expression uses wrong operator.")
+	assert.Contains(t, result.Body, "Remediation: Use && instead of ||.")
+}
+
+func TestEnsureBodyFindingsConsistency_MultipleSeverities(t *testing.T) {
+	result := ReviewResult{
+		Action: "request-changes",
+		Body:   "## Review\n### Findings\nNo findings.\n\n(Previous run had issues)",
+		Findings: []ReviewFinding{
+			{Severity: "critical", Category: "logic-error", File: "a.yaml", Line: 10, Description: "First bug."},
+			{Severity: "high", Category: "security", File: "b.go", Line: 20, Description: "Second bug."},
+			{Severity: "low", Category: "style", File: "c.go", Line: 5, Description: "Nitpick."},
+		},
+	}
+
+	patched := ensureBodyFindingsConsistency(&result)
+	assert.True(t, patched)
+	// Synthesized body includes ALL findings (not just critical/high).
+	assert.Contains(t, result.Body, "#### Critical")
+	assert.Contains(t, result.Body, "a.yaml:10")
+	assert.Contains(t, result.Body, "#### High")
+	assert.Contains(t, result.Body, "b.go:20")
+	assert.Contains(t, result.Body, "#### Low")
+	assert.Contains(t, result.Body, "Nitpick.")
+	// Old body content is fully replaced, not preserved.
+	assert.NotContains(t, result.Body, "Previous run had issues")
+}
+
+func TestEnsureBodyFindingsConsistency_NoopWhenBodyReferencesCategory(t *testing.T) {
+	result := ReviewResult{
+		Action: "request-changes",
+		Body:   "## Review\n### Findings\n#### Critical\n- **[logic-error]** `pipeline.yaml:42` — Bad CEL expression.",
+		Findings: []ReviewFinding{
+			{Severity: "critical", Category: "logic-error", Description: "Bad CEL expression."},
+		},
+	}
+
+	patched := ensureBodyFindingsConsistency(&result)
+	assert.False(t, patched, "body already references the finding category, should not be patched")
+}
+
+func TestEnsureBodyFindingsConsistency_NoopWhenApprove(t *testing.T) {
+	result := ReviewResult{
+		Action: "approve",
+		Body:   "## Review\n### Findings\nNo findings.",
+		Findings: []ReviewFinding{
+			{Severity: "low", Category: "style", Description: "Nitpick."},
+		},
+	}
+
+	patched := ensureBodyFindingsConsistency(&result)
+	assert.False(t, patched, "approve action should not trigger patching")
+}
+
+func TestEnsureBodyFindingsConsistency_NoopWhenComment(t *testing.T) {
+	result := ReviewResult{
+		Action: "comment",
+		Body:   "## Review\n### Findings\nNo findings.",
+		Findings: []ReviewFinding{
+			{Severity: "high", Category: "security", Description: "Auth bypass."},
+		},
+	}
+
+	patched := ensureBodyFindingsConsistency(&result)
+	assert.False(t, patched, "comment action should not trigger patching")
+}
+
+func TestEnsureBodyFindingsConsistency_NoopWhenOnlyLowFindings(t *testing.T) {
+	result := ReviewResult{
+		Action: "request-changes",
+		Body:   "## Review\n### Findings\nNo findings.",
+		Findings: []ReviewFinding{
+			{Severity: "low", Category: "style", Description: "Nitpick."},
+			{Severity: "medium", Category: "docs", Description: "Missing docs."},
+		},
+	}
+
+	patched := ensureBodyFindingsConsistency(&result)
+	assert.False(t, patched, "only low/medium findings should not trigger patching")
+}
+
+func TestEnsureBodyFindingsConsistency_NoopWhenNoFindings(t *testing.T) {
+	result := ReviewResult{
+		Action: "request-changes",
+		Body:   "## Review\n### Findings\nNo findings.",
+	}
+
+	patched := ensureBodyFindingsConsistency(&result)
+	assert.False(t, patched, "empty findings array should not trigger patching")
+}
+
+func TestEnsureBodyFindingsConsistency_NilResult(t *testing.T) {
+	patched := ensureBodyFindingsConsistency(nil)
+	assert.False(t, patched)
+}
+
+func TestEnsureBodyFindingsConsistency_RejectAction(t *testing.T) {
+	result := ReviewResult{
+		Action: "reject",
+		Body:   "## Review\n### Findings\nNo findings.",
+		Findings: []ReviewFinding{
+			{Severity: "high", Category: "auth-bypass", File: "auth.go", Line: 99, Description: "Auth bypass."},
+		},
+	}
+
+	patched := ensureBodyFindingsConsistency(&result)
+	assert.True(t, patched, "reject maps to REQUEST_CHANGES, should trigger patching")
+	assert.Contains(t, result.Body, "auth-bypass")
+	assert.Contains(t, result.Body, "Auth bypass.")
+}
+
+func TestEnsureBodyFindingsConsistency_FindingWithoutFile(t *testing.T) {
+	result := ReviewResult{
+		Action: "request-changes",
+		Body:   "## Review\n### Findings\nNo findings.",
+		Findings: []ReviewFinding{
+			{Severity: "critical", Category: "architecture", Description: "Major design flaw."},
+		},
+	}
+
+	patched := ensureBodyFindingsConsistency(&result)
+	assert.True(t, patched)
+	assert.Contains(t, result.Body, "architecture")
+	assert.Contains(t, result.Body, "Major design flaw.")
+	// No file location backtick in the output.
+	assert.NotContains(t, result.Body, "` —")
+}
+
+func TestEnsureBodyFindingsConsistency_CaseInsensitiveCategory(t *testing.T) {
+	result := ReviewResult{
+		Action: "request-changes",
+		Body:   "## Review\n#### Critical\n- **[Logic-Error]** Bad expression.",
+		Findings: []ReviewFinding{
+			{Severity: "critical", Category: "logic-error", Description: "Bad expression."},
+		},
+	}
+
+	patched := ensureBodyFindingsConsistency(&result)
+	assert.False(t, patched, "case-insensitive category match should detect the reference")
+}
+
+func TestSynthesizeReviewBody(t *testing.T) {
+	findings := []ReviewFinding{
+		{Severity: "high", Category: "missing-test", File: "svc.go", Line: 10, Description: "No test.", Remediation: "Add one."},
+		{Severity: "critical", Category: "logic-error", File: "main.go", Line: 5, Description: "Off by one."},
+		{Severity: "low", Category: "style", Description: "Naming."},
+	}
+
+	body := synthesizeReviewBody(findings)
+	// Critical should come before high, high before low.
+	critIdx := strings.Index(body, "#### Critical")
+	highIdx := strings.Index(body, "#### High")
+	lowIdx := strings.Index(body, "#### Low")
+	assert.Greater(t, highIdx, critIdx, "Critical should appear before High")
+	assert.Greater(t, lowIdx, highIdx, "High should appear before Low")
+	assert.Contains(t, body, "Remediation: Add one.")
+	assert.Contains(t, body, "`main.go:5`")
+	assert.NotContains(t, body, "#### Medium")
+	assert.NotContains(t, body, "#### Info")
+}