diff --git a/CLAUDE.md b/CLAUDE.md deleted file mode 100644 index 6b521bfc3..000000000 --- a/CLAUDE.md +++ /dev/null @@ -1,57 +0,0 @@ -# CLAUDE.md - -Fullsend is a platform for fully autonomous agentic development for GitHub-hosted organizations. It contains design documents organized by problem domain (`docs/`) and a Go CLI (`cmd/fullsend/`) that manages GitHub App setup and org configuration. See [README.md](README.md) for the full document index. - -## How to work in this repo - -- This is a design exploration, not a spec. Documents should present multiple options with trade-offs, not prescribe single solutions. -- Each problem document has an "Open questions" section — this is where unresolved issues live. -- When adding new problem areas, create a new file in `docs/problems/` and link it from `README.md`. -- The security threat model (threat priority: external injection > insider > drift > supply chain) should inform all other documents. -- Keep core problem documents organization-agnostic. Organization-specific details belong in `docs/problems/applied//`. -- The target audience is any contributor community considering autonomous agents — keep language accessible, avoid presuming solutions. -- Always run `make lint` before submitting changes and fix any failures. -- You **must** read and follow [COMMITS.md](COMMITS.md) when writing or reviewing commit messages. Getting the prefix right is not optional — GoReleaser uses it to build release notes. -- Never commit secrets (tokens, API keys, PEM keys, gcloud credentials) or sensitive data (GCP project names, service account identifiers, Model Armor template names, internal hostnames). Use environment variables with no defaults for sensitive values. - -## Go code - -**Mint function:** The mint Cloud Function source lives in two places that must stay in sync: -- `internal/mint/main.go` — the source of truth (has its own `go.mod`, tests run from `internal/mint/`) -- `internal/dispatch/gcf/mintsrc/main.go.embed` — the embedded copy deployed as a GCP Cloud Function - -When changing `internal/mint/main.go`, always copy it to `internal/dispatch/gcf/mintsrc/main.go.embed`. If `go.mod` or `go.sum` changed, sync those to `go.mod.embed` and `go.sum.embed` too. - -The `internal/mintcore/` module is shared between the mint and devmint. Its files are also embedded for Cloud Function deployment at `internal/dispatch/gcf/mintsrc/mintcore/*.embed`. When changing any file in `internal/mintcore/`, sync it to the corresponding `.embed` file under `mintsrc/mintcore/`. Note: the mint's `go.mod.embed` uses `replace mintcore => ./mintcore` (not `../mintcore`), because `provisioner.go` rewrites the replace directive at bundle time to match the deployed directory layout. - -**Dispatch workflows:** The scaffold `dispatch.yml` (at `internal/scaffold/fullsend-repo/.github/workflows/dispatch.yml`) and the repo's `reusable-dispatch.yml` (at `.github/workflows/reusable-dispatch.yml`) share identical routing logic for different installation modes (per-org vs per-repo). When changing the jq payload construction, stage routing, or input/secret threading in one, apply the same change to the other. - -**Forge abstraction:** All git forge operations must go through the `forge.Client` interface in `internal/forge/forge.go`. Do not use `exec.Command("gh", ...)` or direct GitHub API calls outside `internal/forge/github/`. See [AGENTS.md](AGENTS.md#forge-abstraction) for details. - -When making changes to Go code under `cmd/` or `internal/`: - -1. **Unit tests:** Run `make go-test` (or `go test ./...`) and fix any failures before committing. -2. **Vet:** Run `make go-vet` to catch common issues. -3. **E2E tests:** Run `make e2e-test` if your changes touch `internal/appsetup/`, `internal/forge/`, `internal/cli/`, or `internal/layers/`. These tests exercise the full admin install/uninstall flow against a live GitHub org using Playwright browser automation. - -### Running e2e tests - -The e2e tests require GitHub credentials. There are three ways to provide them: - -- **`E2E_GITHUB_PASSWORD` env var:** Set directly with the password. -- **`E2E_GITHUB_PASSWORD_FILE` env var:** Set to a file path containing the password (used in devaipod environments where secrets are mounted as files). -- **`E2E_GITHUB_SESSION_FILE` env var:** Set to a pre-exported Playwright session file (skips login). -- **`E2E_GITHUB_TOTP_SECRET` env var:** Optional. The TOTP secret (base32) for the test account's 2FA. Required only when the test account has 2FA enabled — used during session export and sudo confirmation. - -If only `E2E_GITHUB_USERNAME` and a password source are available, `make e2e-test` will automatically generate a session file before running tests. See `make help` for all available targets. - -## Key design decisions made - -- **Autonomy model:** Binary per-repo, with CODEOWNERS enforcing human approval on specific paths -- **Problem structure:** Problem-oriented documents (not ADRs or RFCs) that can evolve independently, with ADRs spun off later when decisions crystallize -- **Threat priority order:** External prompt injection > insider/compromised creds > agent drift > supply chain -- **Code generation is considered a solved problem.** The hard problems are review, intent, governance, and security. -- **Trust derives from repository permissions, not agent identity.** No agent trusts another based on who produced the output. -- **CODEOWNERS files are always human-owned.** Agents cannot modify their own guardrails. -- **The repo is the coordinator.** No coordinator agent — branch protection, CODEOWNERS, and status checks are the coordination layer. -- **Organization-specific content is cordoned.** Core problem docs are general; applied considerations live in `docs/problems/applied/`. diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index eb9be86eb..8004eafbd 100644 --- a/internal/cli/postreview.go +++ b/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 diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index 05b7866ca..e989374ae 100644 --- a/internal/cli/postreview_test.go +++ b/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") +} diff --git a/internal/cli/qf_body_consistency_test.go b/internal/cli/qf_body_consistency_test.go new file mode 100644 index 000000000..ce42441bd --- /dev/null +++ b/internal/cli/qf_body_consistency_test.go @@ -0,0 +1,366 @@ +package cli + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestEnsureBodyFindingsConsistency_QF covers the 17 STD scenarios for +// the ensureBodyFindingsConsistency and synthesizeReviewBody functions. +// Source: outputs/std/GH-78/GH-78_test_description.yaml + +func TestEnsureBodyFindingsConsistency_QF(t *testing.T) { + + // TS-GH-78-001: Contradictory body replaced for request-changes with critical findings. + t.Run("TS-GH-78-001 contradictory body replaced for request-changes with critical findings", func(t *testing.T) { + result := ReviewResult{ + Action: "request-changes", + Body: "No findings to report.", + Findings: []ReviewFinding{ + { + Category: "logic-error", + Severity: "critical", + File: "cmd/run.go", + Line: 42, + Description: "Pointer dereference without nil guard", + Remediation: "Add nil check before dereference", + }, + }, + } + + patched := ensureBodyFindingsConsistency(&result) + + require.True(t, patched, "function must return true when body is replaced") + assert.NotContains(t, result.Body, "No findings to report.", "original contradictory body must be overwritten") + assert.Contains(t, result.Body, "logic-error", "synthesized body must contain the critical finding category") + assert.Contains(t, result.Body, "Pointer dereference without nil guard", "synthesized body must contain the finding description") + }) + + // TS-GH-78-002: Severity sections ordered critical > high > medium > low > info. + t.Run("TS-GH-78-002 severity sections ordered critical high medium low info", func(t *testing.T) { + result := ReviewResult{ + Action: "request-changes", + Body: "No issues found.", + Findings: []ReviewFinding{ + {Category: "perf-issue", Severity: "low", Description: "Slow loop"}, + {Category: "logic-error", Severity: "critical", Description: "Nil deref"}, + {Category: "style-issue", Severity: "info", Description: "Naming"}, + {Category: "auth-bypass", Severity: "high", Description: "Missing auth"}, + {Category: "data-race", Severity: "medium", Description: "Race condition"}, + }, + } + + patched := ensureBodyFindingsConsistency(&result) + require.True(t, patched) + + body := result.Body + critIdx := strings.Index(body, "Critical") + highIdx := strings.Index(body, "High") + medIdx := strings.Index(body, "Medium") + lowIdx := strings.Index(body, "Low") + infoIdx := strings.Index(body, "Info") + + require.NotEqual(t, -1, critIdx, "Critical section must be present") + require.NotEqual(t, -1, highIdx, "High section must be present") + require.NotEqual(t, -1, medIdx, "Medium section must be present") + require.NotEqual(t, -1, lowIdx, "Low section must be present") + require.NotEqual(t, -1, infoIdx, "Info section must be present") + + assert.Less(t, critIdx, highIdx, "Critical must appear before High") + assert.Less(t, highIdx, medIdx, "High must appear before Medium") + assert.Less(t, medIdx, lowIdx, "Medium must appear before Low") + assert.Less(t, lowIdx, infoIdx, "Low must appear before Info") + }) + + // TS-GH-78-003: Synthesized body includes correct headings and bullet format. + t.Run("TS-GH-78-003 synthesized body includes headings and bullet format", func(t *testing.T) { + result := ReviewResult{ + Action: "request-changes", + Body: "LGTM", + Findings: []ReviewFinding{ + { + Category: "logic-error", + Severity: "critical", + File: "pkg/handler.go", + Line: 55, + Description: "Dereference of potentially nil pointer", + }, + }, + } + + patched := ensureBodyFindingsConsistency(&result) + require.True(t, patched) + + assert.Contains(t, result.Body, "## Review", "body must contain Review heading") + assert.Contains(t, result.Body, "### Findings", "body must contain Findings heading") + assert.Contains(t, result.Body, "#### Critical", "body must contain severity sub-section") + assert.Contains(t, result.Body, "- **[logic-error]**", "finding must be rendered as bullet with category") + assert.Contains(t, result.Body, "Dereference of potentially nil pointer", "finding description must be present") + }) + + // TS-GH-78-004: Reject action triggers body replacement with critical findings. + t.Run("TS-GH-78-004 reject action triggers body replacement", func(t *testing.T) { + result := ReviewResult{ + Action: "reject", + Body: "Looks good overall.", + Findings: []ReviewFinding{ + { + Category: "security-vuln", + Severity: "critical", + Description: "Unsanitized input in query", + }, + }, + } + + patched := ensureBodyFindingsConsistency(&result) + require.True(t, patched, "reject maps to REQUEST_CHANGES and must trigger replacement") + assert.Contains(t, result.Body, "security-vuln", "synthesized body must contain finding category") + assert.Contains(t, result.Body, "Unsanitized input in query", "synthesized body must contain finding description") + }) + + // TS-GH-78-005: No-op when body contains finding category string. + t.Run("TS-GH-78-005 no-op when body contains finding category", func(t *testing.T) { + originalBody := "Found a logic-error in the handler that needs fixing." + result := ReviewResult{ + Action: "request-changes", + Body: originalBody, + Findings: []ReviewFinding{ + { + Category: "logic-error", + Severity: "critical", + Description: "Handler does not check for nil", + }, + }, + } + + patched := ensureBodyFindingsConsistency(&result) + assert.False(t, patched, "body references finding category, should not be replaced") + assert.Equal(t, originalBody, result.Body, "body must remain unchanged") + }) + + // TS-GH-78-006: Case-insensitive category matching prevents unnecessary replacement. + t.Run("TS-GH-78-006 case-insensitive category matching", func(t *testing.T) { + originalBody := "There is a Logic-Error in the code." + result := ReviewResult{ + Action: "request-changes", + Body: originalBody, + Findings: []ReviewFinding{ + { + Category: "logic-error", + Severity: "critical", + Description: "Missing nil check", + }, + }, + } + + patched := ensureBodyFindingsConsistency(&result) + assert.False(t, patched, "case-insensitive match must detect the category reference") + }) + + // TS-GH-78-007: Approve action never triggers body replacement. + t.Run("TS-GH-78-007 approve action never triggers replacement", func(t *testing.T) { + originalBody := "No issues." + result := ReviewResult{ + Action: "approve", + Body: originalBody, + Findings: []ReviewFinding{ + { + Category: "logic-error", + Severity: "critical", + Description: "Possible nil deref", + }, + }, + } + + patched := ensureBodyFindingsConsistency(&result) + assert.False(t, patched, "approve action must not trigger body replacement") + assert.Equal(t, originalBody, result.Body, "body must remain unchanged") + }) + + // TS-GH-78-008: Comment action never triggers body replacement. + t.Run("TS-GH-78-008 comment action never triggers replacement", func(t *testing.T) { + originalBody := "Everything looks fine." + result := ReviewResult{ + Action: "comment", + Body: originalBody, + Findings: []ReviewFinding{ + { + Category: "perf-issue", + Severity: "high", + Description: "N+1 query detected", + }, + }, + } + + patched := ensureBodyFindingsConsistency(&result) + assert.False(t, patched, "comment action must not trigger body replacement") + assert.Equal(t, originalBody, result.Body, "body must remain unchanged") + }) + + // TS-GH-78-009: Low/medium-only findings do not trigger replacement. + t.Run("TS-GH-78-009 low-medium-only findings do not trigger replacement", func(t *testing.T) { + originalBody := "No significant issues." + result := ReviewResult{ + Action: "request-changes", + Body: originalBody, + Findings: []ReviewFinding{ + {Category: "style-issue", Severity: "low", Description: "Variable name does not follow convention"}, + {Category: "perf-issue", Severity: "medium", Description: "Could use buffer pool"}, + }, + } + + patched := ensureBodyFindingsConsistency(&result) + assert.False(t, patched, "only low/medium findings must not trigger replacement") + assert.Equal(t, originalBody, result.Body, "body must remain unchanged") + }) + + // TS-GH-78-010: File:line rendered in backtick block in synthesized body. + t.Run("TS-GH-78-010 file-line rendered in backtick format", func(t *testing.T) { + result := ReviewResult{ + Action: "request-changes", + Body: "LGTM", + Findings: []ReviewFinding{ + { + Category: "logic-error", + Severity: "critical", + File: "pkg/processor.go", + Line: 127, + Description: "Loop bounds incorrect", + }, + }, + } + + patched := ensureBodyFindingsConsistency(&result) + require.True(t, patched) + assert.Contains(t, result.Body, "`pkg/processor.go:127`", "file:line must be rendered in backtick format") + }) + + // TS-GH-78-011: Findings without file path render without backtick location. + t.Run("TS-GH-78-011 findings without file render without location block", func(t *testing.T) { + result := ReviewResult{ + Action: "request-changes", + Body: "All clear.", + Findings: []ReviewFinding{ + { + Category: "architecture", + Severity: "high", + Description: "No global error handler defined", + }, + }, + } + + patched := ensureBodyFindingsConsistency(&result) + require.True(t, patched) + assert.Contains(t, result.Body, "architecture", "finding category must be present") + assert.Contains(t, result.Body, "No global error handler defined", "finding description must be present") + // No backtick-wrapped location should appear for findings without a file. + assert.NotContains(t, result.Body, "``", "no empty backtick blocks should appear") + }) + + // TS-GH-78-012: Remediation text rendered for findings that have it. + t.Run("TS-GH-78-012 remediation text rendered for findings", func(t *testing.T) { + result := ReviewResult{ + Action: "request-changes", + Body: "Ship it.", + Findings: []ReviewFinding{ + { + Category: "logic-error", + Severity: "critical", + File: "pkg/calc.go", + Line: 33, + Description: "Divisor not validated", + Remediation: "Add a zero-check guard before the division", + }, + }, + } + + patched := ensureBodyFindingsConsistency(&result) + require.True(t, patched) + assert.Contains(t, result.Body, "Add a zero-check guard before the division", + "remediation text must be included in synthesized body") + }) + + // TS-GH-78-013: Unpopulated severity sections are absent from output. + t.Run("TS-GH-78-013 unpopulated severity sections absent from output", func(t *testing.T) { + result := ReviewResult{ + Action: "request-changes", + Body: "Nothing to see here.", + Findings: []ReviewFinding{ + {Category: "logic-error", Severity: "critical", Description: "Nil deref"}, + {Category: "perf-issue", Severity: "low", Description: "Unnecessary alloc"}, + }, + } + + patched := ensureBodyFindingsConsistency(&result) + require.True(t, patched) + + assert.Contains(t, result.Body, "#### Critical", "Critical section must be present") + assert.Contains(t, result.Body, "#### Low", "Low section must be present") + assert.NotContains(t, result.Body, "#### High", "High section must be absent (no high findings)") + assert.NotContains(t, result.Body, "#### Medium", "Medium section must be absent (no medium findings)") + assert.NotContains(t, result.Body, "#### Info", "Info section must be absent (no info findings)") + }) + + // TS-GH-78-014: Nil input returns false without panic. + t.Run("TS-GH-78-014 nil input returns false without panic", func(t *testing.T) { + assert.NotPanics(t, func() { + patched := ensureBodyFindingsConsistency(nil) + assert.False(t, patched, "nil input must return false") + }) + }) + + // TS-GH-78-015: Empty findings returns false. + t.Run("TS-GH-78-015 empty findings returns false", func(t *testing.T) { + originalBody := "No findings." + result := ReviewResult{ + Action: "request-changes", + Body: originalBody, + Findings: []ReviewFinding{}, + } + + patched := ensureBodyFindingsConsistency(&result) + assert.False(t, patched, "empty findings array must return false") + assert.Equal(t, originalBody, result.Body, "body must remain unchanged") + }) + + // TS-GH-78-016: Unknown action returns false without modification. + t.Run("TS-GH-78-016 unknown action returns false", func(t *testing.T) { + originalBody := "No issues." + result := ReviewResult{ + Action: "unknown-action", + Body: originalBody, + Findings: []ReviewFinding{ + {Category: "logic-error", Severity: "critical", Description: "Serious issue"}, + }, + } + + patched := ensureBodyFindingsConsistency(&result) + assert.False(t, patched, "unknown action must not trigger replacement") + assert.Equal(t, originalBody, result.Body, "body must remain unchanged") + }) + + // TS-GH-78-017: File without line number renders cleanly (no ":0" artifact). + t.Run("TS-GH-78-017 file without line number renders cleanly", func(t *testing.T) { + result := ReviewResult{ + Action: "request-changes", + Body: "Clean code.", + Findings: []ReviewFinding{ + { + Category: "logic-error", + Severity: "critical", + File: "pkg/handler.go", + Line: 0, + Description: "Function falls through", + }, + }, + } + + patched := ensureBodyFindingsConsistency(&result) + require.True(t, patched) + assert.Contains(t, result.Body, "pkg/handler.go", "file path must be present in body") + assert.NotContains(t, result.Body, ":0", "no ':0' artifact should appear for zero line number") + }) +} diff --git a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md index c27da6d55..246cebefd 100644 --- a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md +++ b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md @@ -694,6 +694,12 @@ where `[open]` = `<` + `!--` and `[close]` = `--` + `>`. entirely. If the only findings are medium/low/info, only show that section. If there are no findings at all, state "No findings." in place of the findings section. +- **Body-verdict consistency:** When the action is `request-changes` or + `reject`, the body MUST list the findings that drove that verdict. + Never produce a body that says "No findings" alongside a blocking + action with populated `findings[]`. The CLI has a safety net that + detects this contradiction and synthesizes a replacement body, but + getting it right at the source avoids the fallback. - **No footer.** Do not repeat the outcome or include boilerplate about pushes clearing the review.