diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index c2c819bc5..93ec7eecd 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -2,6 +2,8 @@ package cli import ( "context" + "crypto/sha256" + "encoding/hex" "encoding/json" "fmt" "os" @@ -17,18 +19,22 @@ import ( ) const reviewMarker = "" +const reviewFollowupSummaryMarker = "" +const reviewFollowupIssueMarkerPrefix = "" +} + +func reviewFindingLocation(finding ReviewFinding) string { + if finding.Line > 0 { + return fmt.Sprintf("%s:%d", finding.File, finding.Line) + } + return finding.File +} + +func reviewFindingSummary(finding ReviewFinding) string { + summary := compactWhitespace(finding.Description) + if summary != "" { + return summary + } + return compactWhitespace(finding.Category) +} + +func compactWhitespace(s string) string { + return strings.Join(strings.Fields(s), " ") +} + +func truncate(s string, max int) string { + runes := []rune(s) + if len(runes) <= max { + return s + } + if max <= 0 { + return "" + } + if max <= 3 { + return string(runes[:max]) + } + return strings.TrimSpace(string(runes[:max-3])) + "..." +} + // dismissStaleRequestChanges dismisses the most recent CHANGES_REQUESTED // review by the authenticated user when the new verdict is softer. func dismissStaleRequestChanges(ctx context.Context, client forge.Client, owner, repo string, pr int, newEvent, user string, reviews []forge.PullRequestReview, printer *ui.Printer) { diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index 41d94a014..3b76af9f0 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -1,10 +1,12 @@ package cli import ( + "bytes" "context" "fmt" "io" "testing" + "unicode/utf8" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -61,6 +63,48 @@ func TestParseReviewResult_HeadSHA(t *testing.T) { assert.Equal(t, "abc1234", result.HeadSHA) } +func TestParseReviewResult_Findings(t *testing.T) { + input := `{"body":"Review","action":"approve","findings":[{"severity":"low","category":"docs","file":"README.md","line":12,"description":"Missing usage note","remediation":"Add a short note","actionable":true}]}` + result, err := parseReviewResult(input) + require.NoError(t, err) + require.Len(t, result.Findings, 1) + assert.Equal(t, "low", result.Findings[0].Severity) + assert.True(t, result.Findings[0].Actionable) +} + +func TestNewPostReviewCmd_MaxFollowUpIssuesDefault(t *testing.T) { + cmd := newPostReviewCmd() + flag := cmd.Flags().Lookup("max-follow-up-issues") + require.NotNil(t, flag) + assert.Equal(t, "3", flag.DefValue) +} + +func TestValidateMaxReviewFollowUpIssues(t *testing.T) { + tests := []struct { + name string + value int + wantErr bool + }{ + {name: "disabled", value: 0}, + {name: "within cap", value: 2}, + {name: "at cap", value: 3}, + {name: "negative", value: -1, wantErr: true}, + {name: "above cap", value: 4, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateMaxReviewFollowUpIssues(tt.value, "--max-follow-up-issues") + if tt.wantErr { + require.Error(t, err) + assert.Contains(t, err.Error(), "between 0 and 3") + } else { + require.NoError(t, err) + } + }) + } +} + func TestReviewActionToEvent(t *testing.T) { tests := []struct { action string @@ -70,7 +114,7 @@ func TestReviewActionToEvent(t *testing.T) { {"approve", "APPROVE", true}, {"Approve", "APPROVE", true}, {"request-changes", "REQUEST_CHANGES", true}, - {"request_changes", "REQUEST_CHANGES", true}, + {"request_changes", "", false}, {"comment", "COMMENT", true}, {"reject", "REQUEST_CHANGES", true}, {"Reject", "REQUEST_CHANGES", true}, @@ -312,14 +356,14 @@ func TestHexSHAValidation(t *testing.T) { sha string valid bool }{ - {"abc123f", false}, // too short (7 chars) - {"abc123def456", false}, // too short (12 chars) - {"abc123def456abc123def456abc123def456abcd", true}, // 40-char SHA-1 - {"abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789", true}, // 64-char SHA-256 + {"abc123f", false}, // too short (7 chars) + {"abc123def456", false}, // too short (12 chars) + {"abc123def456abc123def456abc123def456abcd", true}, // 40-char SHA-1 + {"abcdef0123456789abcdef0123456789abcdef0123456789abcdef0123456789", true}, // 64-char SHA-256 {"abcdef0123456789abcdef0123456789abcdef0123456789abcdef01234567890", false}, // 65 chars (too long) - {"", true}, // empty is valid (means "no SHA provided") - {"not-hex!", false}, // non-hex chars - {"abc 123", false}, // spaces + {"", true}, // empty is valid (means "no SHA provided") + {"not-hex!", false}, // non-hex chars + {"abc 123", false}, // spaces {"abc123`inject", false}, // backtick injection {"ABC123DEF456ABC123DEF456ABC123DEF456ABCD", true}, // uppercase 40-char } @@ -573,3 +617,371 @@ func TestSubmitFormalReview_ListErrorSkipsCleanup(t *testing.T) { assert.Empty(t, fc.MinimizedComments) require.Len(t, fc.CreatedReviews, 1, "review should still be created despite list error") } + +func TestPostApprovedFollowUpIssues_CreatesIssuesAndSummary(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-review[bot]" + printer := ui.New(io.Discard) + parsed := ReviewResult{ + Action: "approve", + Findings: []ReviewFinding{ + { + Severity: "low", + Category: "missing-test", + File: "internal/service.go", + Line: 42, + Description: "Add coverage for the empty response path.", + Remediation: "Add a unit test that exercises an empty upstream response.", + Actionable: true, + }, + }, + } + + err := postApprovedFollowUpIssues(context.Background(), fc, "acme", "repo", 9, parsed, false, maxReviewFollowUpIssues, printer) + require.NoError(t, err) + + require.Len(t, fc.CreatedIssues, 1) + created := fc.CreatedIssues[0] + assert.Equal(t, "acme", created.Owner) + assert.Equal(t, "repo", created.Repo) + assert.Contains(t, created.Title, "Follow-up from PR #9") + assert.Contains(t, created.Body, "https://github.com/acme/repo/pull/9") + assert.Contains(t, created.Body, "internal/service.go:42") + assert.Contains(t, created.Body, reviewFollowupIssueMarkerPrefix) + assert.Equal(t, []string{"type/chore"}, created.Labels) + + comments := fc.IssueComments["acme/repo/9"] + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "Created follow-up issues") + assert.Contains(t, comments[0].Body, "#1") +} + +func TestPostApprovedFollowUpIssues_SkipsNonActionableFindings(t *testing.T) { + fc := forge.NewFakeClient() + printer := ui.New(io.Discard) + parsed := ReviewResult{ + Action: "approve", + Findings: []ReviewFinding{ + { + Severity: "info", + Category: "style", + File: "README.md", + Description: "Nice-to-have wording improvement.", + Actionable: false, + }, + }, + } + + err := postApprovedFollowUpIssues(context.Background(), fc, "acme", "repo", 9, parsed, false, maxReviewFollowUpIssues, printer) + require.NoError(t, err) + assert.Empty(t, fc.CreatedIssues) + assert.Empty(t, fc.IssueComments) +} + +func TestPostApprovedFollowUpIssues_SkipsMediumFindings(t *testing.T) { + fc := forge.NewFakeClient() + printer := ui.New(io.Discard) + parsed := ReviewResult{ + Action: "approve", + Findings: []ReviewFinding{ + { + Severity: "medium", + Category: "missing-test", + File: "internal/service.go", + Description: "Medium findings should not be turned into approve follow-ups.", + Actionable: true, + }, + }, + } + + err := postApprovedFollowUpIssues(context.Background(), fc, "acme", "repo", 9, parsed, false, maxReviewFollowUpIssues, printer) + require.NoError(t, err) + assert.Empty(t, fc.CreatedIssues) + assert.Empty(t, fc.IssueComments) +} + +func TestPostApprovedFollowUpIssues_UsesExistingDuplicate(t *testing.T) { + finding := ReviewFinding{ + Severity: "info", + Category: "docs", + File: "README.md", + Line: 7, + Description: "Document the new flag.", + Actionable: true, + } + marker := reviewFollowupIssueMarker("acme", "repo", finding) + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-review[bot]" + fc.OpenIssues = map[string][]forge.Issue{ + "acme/repo": { + { + Number: 12, + Title: "Existing follow-up", + Body: marker + "\n\nAlready tracked.", + URL: "https://github.com/acme/repo/issues/12", + Labels: []string{"type/chore"}, + }, + }, + } + printer := ui.New(io.Discard) + parsed := ReviewResult{Action: "approve", Findings: []ReviewFinding{finding}} + + err := postApprovedFollowUpIssues(context.Background(), fc, "acme", "repo", 9, parsed, false, maxReviewFollowUpIssues, printer) + require.NoError(t, err) + + assert.Empty(t, fc.CreatedIssues) + comments := fc.IssueComments["acme/repo/9"] + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "Existing follow-up issues") + assert.Contains(t, comments[0].Body, "#12") +} + +func TestPostApprovedFollowUpIssues_DedupesAcrossPRs(t *testing.T) { + finding := ReviewFinding{ + Severity: "low", + Category: "docs", + File: "README.md", + Line: 7, + Description: "Document the new flag.", + Actionable: true, + } + marker := reviewFollowupIssueMarker("acme", "repo", finding) + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-review[bot]" + fc.OpenIssues = map[string][]forge.Issue{ + "acme/repo": { + { + Number: 12, + Title: "Existing follow-up from earlier PR", + Body: marker + "\n\nOriginally filed from PR #9.", + URL: "https://github.com/acme/repo/issues/12", + Labels: []string{"type/chore"}, + }, + }, + } + printer := ui.New(io.Discard) + parsed := ReviewResult{Action: "approve", Findings: []ReviewFinding{finding}} + + err := postApprovedFollowUpIssues(context.Background(), fc, "acme", "repo", 10, parsed, false, maxReviewFollowUpIssues, printer) + require.NoError(t, err) + + assert.Empty(t, fc.CreatedIssues) + comments := fc.IssueComments["acme/repo/10"] + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "Existing follow-up issues") + assert.Contains(t, comments[0].Body, "#12") +} + +func TestPostApprovedFollowUpIssues_WarnsOnDuplicateMarkers(t *testing.T) { + finding := ReviewFinding{ + Severity: "low", + Category: "docs", + File: "README.md", + Line: 7, + Description: "Document the new flag.", + Actionable: true, + } + marker := reviewFollowupIssueMarker("acme", "repo", finding) + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-review[bot]" + fc.OpenIssues = map[string][]forge.Issue{ + "acme/repo": { + { + Number: 12, + Title: "Existing follow-up", + Body: marker + "\n\nAlready tracked.", + URL: "https://github.com/acme/repo/issues/12", + Labels: []string{"type/chore"}, + }, + { + Number: 13, + Title: "Duplicate follow-up", + Body: marker + "\n\nManually duplicated.", + URL: "https://github.com/acme/repo/issues/13", + Labels: []string{"type/chore"}, + }, + }, + } + var out bytes.Buffer + printer := ui.New(&out) + parsed := ReviewResult{Action: "approve", Findings: []ReviewFinding{finding}} + + err := postApprovedFollowUpIssues(context.Background(), fc, "acme", "repo", 10, parsed, false, maxReviewFollowUpIssues, printer) + require.NoError(t, err) + + assert.Empty(t, fc.CreatedIssues) + comments := fc.IssueComments["acme/repo/10"] + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "#12") + assert.NotContains(t, comments[0].Body, "#13") + assert.Contains(t, out.String(), "Duplicate review follow-up marker found in issues #12 and #13; reusing #12") +} + +func TestPostApprovedFollowUpIssues_DryRun(t *testing.T) { + fc := forge.NewFakeClient() + printer := ui.New(io.Discard) + parsed := ReviewResult{ + Action: "approve", + Findings: []ReviewFinding{ + { + Severity: "low", + Category: "docs", + File: "README.md", + Description: "Document the behavior.", + Actionable: true, + }, + }, + } + + err := postApprovedFollowUpIssues(context.Background(), fc, "acme", "repo", 9, parsed, true, maxReviewFollowUpIssues, printer) + require.NoError(t, err) + assert.Empty(t, fc.CreatedIssues) + assert.Empty(t, fc.IssueComments) +} + +func TestPostApprovedFollowUpIssues_RespectsCreateCap(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-review[bot]" + printer := ui.New(io.Discard) + + findings := make([]ReviewFinding, 0, 5) + for i := 0; i < 5; i++ { + findings = append(findings, ReviewFinding{ + Severity: "low", + Category: "docs", + File: "README.md", + Line: i + 1, + Description: fmt.Sprintf("Document behavior %d.", i+1), + Actionable: true, + }) + } + parsed := ReviewResult{Action: "approve", Findings: findings} + + err := postApprovedFollowUpIssues(context.Background(), fc, "acme", "repo", 9, parsed, false, 2, printer) + require.NoError(t, err) + + require.Len(t, fc.CreatedIssues, 2) + assert.Contains(t, fc.CreatedIssues[0].Body, "Document behavior 1.") + assert.Contains(t, fc.CreatedIssues[1].Body, "Document behavior 2.") + + comments := fc.IssueComments["acme/repo/9"] + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "capped at 2") + assert.Contains(t, comments[0].Body, "3 actionable non-blocking finding(s) were not filed") +} + +func TestPostApprovedFollowUpIssues_ExistingIssuesDoNotConsumeCreateCap(t *testing.T) { + existingFinding := ReviewFinding{ + Severity: "info", + Category: "docs", + File: "README.md", + Line: 7, + Description: "Document the existing flag.", + Actionable: true, + } + marker := reviewFollowupIssueMarker("acme", "repo", existingFinding) + + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-review[bot]" + fc.OpenIssues = map[string][]forge.Issue{ + "acme/repo": { + { + Number: 12, + Title: "Existing follow-up", + Body: marker + "\n\nAlready tracked.", + URL: "https://github.com/acme/repo/issues/12", + Labels: []string{"type/chore"}, + }, + }, + } + printer := ui.New(io.Discard) + parsed := ReviewResult{ + Action: "approve", + Findings: []ReviewFinding{ + existingFinding, + { + Severity: "low", + Category: "docs", + File: "README.md", + Line: 8, + Description: "Document the new flag.", + Actionable: true, + }, + { + Severity: "low", + Category: "docs", + File: "README.md", + Line: 9, + Description: "Document another new flag.", + Actionable: true, + }, + }, + } + + err := postApprovedFollowUpIssues(context.Background(), fc, "acme", "repo", 9, parsed, false, 1, printer) + require.NoError(t, err) + + require.Len(t, fc.CreatedIssues, 1) + assert.Contains(t, fc.CreatedIssues[0].Body, "Document the new flag.") + + comments := fc.IssueComments["acme/repo/9"] + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "Existing follow-up issues") + assert.Contains(t, comments[0].Body, "#12") + assert.Contains(t, comments[0].Body, "capped at 1") + assert.Contains(t, comments[0].Body, "1 actionable non-blocking finding(s) were not filed") +} + +func TestPostApprovedFollowUpIssues_ListOpenIssuesError(t *testing.T) { + fc := forge.NewFakeClient() + fc.Errors["ListOpenIssues"] = fmt.Errorf("boom") + printer := ui.New(io.Discard) + parsed := ReviewResult{ + Action: "approve", + Findings: []ReviewFinding{ + { + Severity: "low", + Category: "docs", + File: "README.md", + Description: "Document the behavior.", + Actionable: true, + }, + }, + } + + err := postApprovedFollowUpIssues(context.Background(), fc, "acme", "repo", 9, parsed, false, maxReviewFollowUpIssues, printer) + require.Error(t, err) + assert.Contains(t, err.Error(), "duplicate detection") + assert.Empty(t, fc.CreatedIssues) + assert.Empty(t, fc.IssueComments) +} + +func TestReviewFollowupIssueMarkerGolden(t *testing.T) { + finding := ReviewFinding{ + Severity: "low", + Category: "docs", + File: "README.md", + Line: 7, + Description: "Document the new flag.", + Actionable: true, + } + + assert.Equal(t, + "", + reviewFollowupIssueMarker("acme", "repo", finding), + ) +} + +func TestCompactWhitespace(t *testing.T) { + assert.Equal(t, "one two three", compactWhitespace(" one\n\ttwo three ")) + assert.Equal(t, "", compactWhitespace(" \n\t ")) +} + +func TestTruncate(t *testing.T) { + assert.Equal(t, "", truncate("", 10)) + assert.Equal(t, "", truncate("abcdef", 0)) + assert.Equal(t, "ab", truncate("abcdef", 2)) + assert.Equal(t, "ab...", truncate("abcdef", 5)) + assert.Equal(t, "éé...", truncate("éééééé", 5)) + assert.True(t, utf8.ValidString(truncate("abéé", 4))) +} diff --git a/internal/forge/fake.go b/internal/forge/fake.go index a754f1cf6..7821b744e 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -55,6 +55,14 @@ type UpdatedCommentRecord struct { Body string } +// CreatedIssueRecord records an issue creation call. +type CreatedIssueRecord struct { + Owner, Repo string + Title, Body string + Labels []string + Number int +} + // MinimizedCommentRecord records a comment minimize call. type MinimizedCommentRecord struct { NodeID string @@ -117,6 +125,7 @@ type FakeClient struct { // Issue comments for ListIssueComments / UpdateIssueComment. IssueComments map[string][]IssueComment // key: "owner/repo/number" + OpenIssues map[string][]Issue // key: "owner/repo" // CommitFilesChanged controls the return value of CommitFiles (default true). CommitFilesChanged *bool @@ -140,6 +149,7 @@ type FakeClient struct { CreatedOrgSecrets []OrgSecretRecord CreatedOrgVariables []OrgVariableRecord DeletedOrgVariables []string // "org/name" + CreatedIssues []CreatedIssueRecord UpdatedComments []UpdatedCommentRecord MinimizedComments []MinimizedCommentRecord CreatedReviews []ReviewRecord @@ -149,6 +159,7 @@ type FakeClient struct { // internal counters proposalCounter int commentCounter int + issueCounter int } // err checks for an injected error for the given method name. @@ -612,13 +623,34 @@ func (f *FakeClient) DispatchWorkflow(_ context.Context, _, _, _, _ string, _ ma return nil } -func (f *FakeClient) CreateIssue(_ context.Context, _, _, _, _ string) (*Issue, error) { +func (f *FakeClient) CreateIssue(_ context.Context, owner, repo, title, body string, labels ...string) (*Issue, error) { f.mu.Lock() defer f.mu.Unlock() if e := f.err("CreateIssue"); e != nil { return nil, e } - return &Issue{Number: 1, Title: "fake", URL: "https://fake"}, nil + f.issueCounter++ + issue := Issue{ + Number: f.issueCounter, + Title: title, + Body: body, + URL: fmt.Sprintf("https://github.com/%s/%s/issues/%d", owner, repo, f.issueCounter), + Labels: append([]string(nil), labels...), + } + f.CreatedIssues = append(f.CreatedIssues, CreatedIssueRecord{ + Owner: owner, + Repo: repo, + Title: title, + Body: body, + Labels: append([]string(nil), labels...), + Number: issue.Number, + }) + key := owner + "/" + repo + if f.OpenIssues == nil { + f.OpenIssues = make(map[string][]Issue) + } + f.OpenIssues[key] = append(f.OpenIssues[key], issue) + return &issue, nil } func (f *FakeClient) CloseIssue(_ context.Context, _, _ string, _ int) error { @@ -627,6 +659,41 @@ func (f *FakeClient) CloseIssue(_ context.Context, _, _ string, _ int) error { return f.err("CloseIssue") } +func (f *FakeClient) ListOpenIssues(_ context.Context, owner, repo string, labels ...string) ([]Issue, error) { + f.mu.Lock() + defer f.mu.Unlock() + if e := f.err("ListOpenIssues"); e != nil { + return nil, e + } + if f.OpenIssues == nil { + return nil, nil + } + issues := f.OpenIssues[owner+"/"+repo] + if len(labels) == 0 { + return append([]Issue(nil), issues...), nil + } + filtered := make([]Issue, 0, len(issues)) + for _, issue := range issues { + if issueHasLabels(issue, labels) { + filtered = append(filtered, issue) + } + } + return filtered, nil +} + +func issueHasLabels(issue Issue, labels []string) bool { + present := make(map[string]struct{}, len(issue.Labels)) + for _, label := range issue.Labels { + present[label] = struct{}{} + } + for _, label := range labels { + if _, ok := present[label]; !ok { + return false + } + } + return true +} + func (f *FakeClient) ListIssueComments(_ context.Context, owner, repo string, number int) ([]IssueComment, error) { f.mu.Lock() defer f.mu.Unlock() diff --git a/internal/forge/forge.go b/internal/forge/forge.go index f2290ad45..861b30b0f 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -52,7 +52,9 @@ type WorkflowRun struct { type Issue struct { Number int Title string + Body string URL string + Labels []string } // IssueComment represents a comment on an issue. @@ -185,8 +187,9 @@ type Client interface { DispatchWorkflow(ctx context.Context, owner, repo, workflowFile, ref string, inputs map[string]string) error // Issue operations - CreateIssue(ctx context.Context, owner, repo, title, body string) (*Issue, error) + CreateIssue(ctx context.Context, owner, repo, title, body string, labels ...string) (*Issue, error) CloseIssue(ctx context.Context, owner, repo string, number int) error + ListOpenIssues(ctx context.Context, owner, repo string, labels ...string) ([]Issue, error) ListIssueComments(ctx context.Context, owner, repo string, number int) ([]IssueComment, error) CreateIssueComment(ctx context.Context, owner, repo string, number int, body string) (*IssueComment, error) UpdateIssueComment(ctx context.Context, owner, repo string, commentID int, body string) error diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 58d0a25bd..827346e86 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -12,6 +12,7 @@ import ( "io" "math" "net/http" + "net/url" "strconv" "strings" "time" @@ -49,6 +50,14 @@ func (c *LiveClient) WithBaseURL(url string) *LiveClient { type APIError struct { StatusCode int Message string + Errors []APIErrorDetail +} + +// APIErrorDetail is one validation error entry returned by GitHub. +type APIErrorDetail struct { + Resource string `json:"resource"` + Field string `json:"field"` + Code string `json:"code"` } func (e *APIError) Error() string { @@ -164,10 +173,11 @@ func checkStatus(resp *http.Response, acceptable ...int) error { data, _ := io.ReadAll(resp.Body) var msg struct { - Message string `json:"message"` + Message string `json:"message"` + Errors []APIErrorDetail `json:"errors"` } if json.Unmarshal(data, &msg) == nil && msg.Message != "" { - return &APIError{StatusCode: resp.StatusCode, Message: msg.Message} + return &APIError{StatusCode: resp.StatusCode, Message: msg.Message, Errors: msg.Errors} } return &APIError{StatusCode: resp.StatusCode, Message: http.StatusText(resp.StatusCode)} } @@ -1138,22 +1148,44 @@ func (c *LiveClient) DispatchWorkflow(ctx context.Context, owner, repo, workflow return nil } -// CreateIssue creates a new issue on a repository. -func (c *LiveClient) CreateIssue(ctx context.Context, owner, repo, title, body string) (*forge.Issue, error) { - payload := map[string]string{"title": title, "body": body} +// CreateIssue creates a new issue on a repository. Labels are best-effort: +// if GitHub rejects the create because a label is unavailable in the target +// repo, the request is retried without labels so issue creation still succeeds. +func (c *LiveClient) CreateIssue(ctx context.Context, owner, repo, title, body string, labels ...string) (*forge.Issue, error) { + payload := map[string]any{"title": title, "body": body} + if len(labels) > 0 { + payload["labels"] = labels + } resp, err := c.post(ctx, fmt.Sprintf("/repos/%s/%s/issues", owner, repo), payload) if err != nil { - return nil, fmt.Errorf("create issue: %w", err) + var apiErr *APIError + if len(labels) == 0 || !errors.As(err, &apiErr) || !isValidationErrorForField(apiErr, "labels") { + return nil, fmt.Errorf("create issue: %w", err) + } + resp, err = c.post(ctx, fmt.Sprintf("/repos/%s/%s/issues", owner, repo), map[string]any{"title": title, "body": body}) + if err != nil { + return nil, fmt.Errorf("create issue without labels after label rejection: %w", err) + } } var result struct { Number int `json:"number"` Title string `json:"title"` + Body string `json:"body"` HTMLURL string `json:"html_url"` + Labels []struct { + Name string `json:"name"` + } `json:"labels"` } if err := decodeJSON(resp, &result); err != nil { return nil, fmt.Errorf("decode issue: %w", err) } - return &forge.Issue{Number: result.Number, Title: result.Title, URL: result.HTMLURL}, nil + return &forge.Issue{ + Number: result.Number, + Title: result.Title, + Body: result.Body, + URL: result.HTMLURL, + Labels: labelNames(result.Labels), + }, nil } // CloseIssue closes an issue by number. @@ -1166,6 +1198,79 @@ func (c *LiveClient) CloseIssue(ctx context.Context, owner, repo string, number return nil } +func labelNames(labels []struct { + Name string `json:"name"` +}) []string { + names := make([]string, 0, len(labels)) + for _, label := range labels { + names = append(names, label.Name) + } + return names +} + +func isValidationErrorForField(err *APIError, field string) bool { + if err == nil || err.StatusCode != http.StatusUnprocessableEntity { + return false + } + for _, detail := range err.Errors { + if detail.Field == field { + return true + } + } + return false +} + +// ListOpenIssues returns open issues on a repository, excluding pull requests. +// When labels are provided, GitHub filters to issues carrying those labels. +func (c *LiveClient) ListOpenIssues(ctx context.Context, owner, repo string, labels ...string) ([]forge.Issue, error) { + var result []forge.Issue + + for page := 1; page <= 100; page++ { + query := url.Values{} + query.Set("state", "open") + query.Set("per_page", "100") + query.Set("page", strconv.Itoa(page)) + if len(labels) > 0 { + query.Set("labels", strings.Join(labels, ",")) + } + resp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/issues?%s", owner, repo, query.Encode())) + if err != nil { + return nil, fmt.Errorf("list open issues page %d: %w", page, err) + } + var raw []struct { + Number int `json:"number"` + Title string `json:"title"` + Body string `json:"body"` + HTMLURL string `json:"html_url"` + PullRequest *struct { + URL string `json:"url"` + } `json:"pull_request"` + Labels []struct { + Name string `json:"name"` + } `json:"labels"` + } + if err := decodeJSON(resp, &raw); err != nil { + return nil, fmt.Errorf("decode open issues page %d: %w", page, err) + } + for _, item := range raw { + if item.PullRequest != nil { + continue + } + result = append(result, forge.Issue{ + Number: item.Number, + Title: item.Title, + Body: item.Body, + URL: item.HTMLURL, + Labels: labelNames(item.Labels), + }) + } + if len(raw) < 100 { + break + } + } + return result, nil +} + // ListIssueComments returns all comments on an issue, paginating automatically. func (c *LiveClient) ListIssueComments(ctx context.Context, owner, repo string, number int) ([]forge.IssueComment, error) { var result []forge.IssueComment diff --git a/internal/forge/github/github_comment_test.go b/internal/forge/github/github_comment_test.go index d30af855d..0ee2786ee 100644 --- a/internal/forge/github/github_comment_test.go +++ b/internal/forge/github/github_comment_test.go @@ -3,14 +3,221 @@ package github import ( "context" "encoding/json" + "errors" "net/http" "net/http/httptest" + "strconv" "testing" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) +func TestCreateIssueWithLabels(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "POST", r.Method) + assert.Equal(t, "/repos/owner/repo/issues", r.URL.Path) + + var body map[string]any + json.NewDecoder(r.Body).Decode(&body) + assert.Equal(t, "Follow-up", body["title"]) + assert.Equal(t, "Body", body["body"]) + assert.Equal(t, []any{"type/chore"}, body["labels"]) + + w.WriteHeader(http.StatusCreated) + json.NewEncoder(w).Encode(map[string]any{ + "number": 77, + "title": "Follow-up", + "body": "Body", + "html_url": "https://github.com/owner/repo/issues/77", + "labels": []map[string]any{{"name": "type/chore"}}, + }) + })) + defer srv.Close() + + client := newTestClient(t, srv) + issue, err := client.CreateIssue(context.Background(), "owner", "repo", "Follow-up", "Body", "type/chore") + require.NoError(t, err) + assert.Equal(t, 77, issue.Number) + assert.Equal(t, "Follow-up", issue.Title) + assert.Equal(t, "Body", issue.Body) + assert.Equal(t, []string{"type/chore"}, issue.Labels) +} + +func TestCreateIssueRetriesWithoutLabelsOnValidationError(t *testing.T) { + call := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + call++ + assert.Equal(t, "POST", r.Method) + assert.Equal(t, "/repos/owner/repo/issues", r.URL.Path) + + var body map[string]any + json.NewDecoder(r.Body).Decode(&body) + if call == 1 { + assert.Equal(t, []any{"missing-label"}, body["labels"]) + w.WriteHeader(http.StatusUnprocessableEntity) + json.NewEncoder(w).Encode(map[string]any{ + "message": "Validation Failed", + "errors": []map[string]any{{"field": "labels", "code": "invalid"}}, + }) + return + } + + _, hasLabels := body["labels"] + assert.False(t, hasLabels) + w.WriteHeader(http.StatusCreated) + json.NewEncoder(w).Encode(map[string]any{ + "number": 78, + "title": "Follow-up", + "body": "Body", + "html_url": "https://github.com/owner/repo/issues/78", + }) + })) + defer srv.Close() + + client := newTestClient(t, srv) + issue, err := client.CreateIssue(context.Background(), "owner", "repo", "Follow-up", "Body", "missing-label") + require.NoError(t, err) + assert.Equal(t, 78, issue.Number) + assert.Equal(t, 2, call) +} + +func TestCreateIssueDoesNotRetryNonLabelValidationErrors(t *testing.T) { + call := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + call++ + w.WriteHeader(http.StatusUnprocessableEntity) + json.NewEncoder(w).Encode(map[string]any{ + "message": "Validation Failed", + "errors": []map[string]any{{"field": "title", "code": "missing"}}, + }) + })) + defer srv.Close() + + client := newTestClient(t, srv) + _, err := client.CreateIssue(context.Background(), "owner", "repo", "Follow-up", "Body", "type/chore") + require.Error(t, err) + assert.Equal(t, 1, call) + var apiErr *APIError + require.True(t, errors.As(err, &apiErr)) + assert.Equal(t, http.StatusUnprocessableEntity, apiErr.StatusCode) + require.Len(t, apiErr.Errors, 1) + assert.Equal(t, "title", apiErr.Errors[0].Field) +} + +func TestCreateIssueReturnsNonLabelErrors(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + json.NewEncoder(w).Encode(map[string]any{"message": "Resource not accessible by integration"}) + })) + defer srv.Close() + + client := newTestClient(t, srv) + _, err := client.CreateIssue(context.Background(), "owner", "repo", "Follow-up", "Body", "type/chore") + require.Error(t, err) + var apiErr *APIError + require.True(t, errors.As(err, &apiErr)) + assert.Equal(t, http.StatusForbidden, apiErr.StatusCode) +} + +func TestListOpenIssuesSkipsPullRequests(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "GET", r.Method) + assert.Equal(t, "/repos/owner/repo/issues", r.URL.Path) + assert.Equal(t, "open", r.URL.Query().Get("state")) + assert.Equal(t, "100", r.URL.Query().Get("per_page")) + assert.Equal(t, "1", r.URL.Query().Get("page")) + + json.NewEncoder(w).Encode([]map[string]any{ + { + "number": 1, + "title": "Issue", + "body": "Issue body", + "html_url": "https://github.com/owner/repo/issues/1", + "labels": []map[string]any{{"name": "type/chore"}}, + }, + { + "number": 2, + "title": "PR", + "body": "PR body", + "html_url": "https://github.com/owner/repo/pull/2", + "pull_request": map[string]any{"url": "https://api.github.com/repos/owner/repo/pulls/2"}, + }, + }) + })) + defer srv.Close() + + client := newTestClient(t, srv) + issues, err := client.ListOpenIssues(context.Background(), "owner", "repo") + require.NoError(t, err) + require.Len(t, issues, 1) + assert.Equal(t, 1, issues[0].Number) + assert.Equal(t, "Issue body", issues[0].Body) + assert.Equal(t, []string{"type/chore"}, issues[0].Labels) +} + +func TestListOpenIssuesFiltersByLabels(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, "GET", r.Method) + assert.Equal(t, "/repos/owner/repo/issues", r.URL.Path) + assert.Equal(t, "open", r.URL.Query().Get("state")) + assert.Equal(t, "type/chore", r.URL.Query().Get("labels")) + + json.NewEncoder(w).Encode([]map[string]any{ + { + "number": 1, + "title": "Issue", + "body": "Issue body", + "html_url": "https://github.com/owner/repo/issues/1", + "labels": []map[string]any{{"name": "type/chore"}}, + }, + }) + })) + defer srv.Close() + + client := newTestClient(t, srv) + issues, err := client.ListOpenIssues(context.Background(), "owner", "repo", "type/chore") + require.NoError(t, err) + require.Len(t, issues, 1) + assert.Equal(t, []string{"type/chore"}, issues[0].Labels) +} + +func TestListOpenIssuesPaginatesUntilShortPage(t *testing.T) { + call := 0 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + call++ + assert.Equal(t, "GET", r.Method) + assert.Equal(t, "/repos/owner/repo/issues", r.URL.Path) + assert.Equal(t, "open", r.URL.Query().Get("state")) + assert.Equal(t, "100", r.URL.Query().Get("per_page")) + assert.Equal(t, strconv.Itoa(call), r.URL.Query().Get("page")) + + items := make([]map[string]any, 0, 100) + limit := 100 + if call == 2 { + limit = 1 + } + for i := 0; i < limit; i++ { + number := ((call - 1) * 100) + i + 1 + items = append(items, map[string]any{ + "number": number, + "title": "Issue", + "body": "Issue body", + "html_url": "https://github.com/owner/repo/issues/1", + }) + } + json.NewEncoder(w).Encode(items) + })) + defer srv.Close() + + client := newTestClient(t, srv) + issues, err := client.ListOpenIssues(context.Background(), "owner", "repo") + require.NoError(t, err) + require.Len(t, issues, 101) + assert.Equal(t, 2, call) + assert.Equal(t, 101, issues[100].Number) +} + func TestCreateIssueComment(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, "POST", r.Method) diff --git a/internal/scaffold/fullsend-repo/agents/review.md b/internal/scaffold/fullsend-repo/agents/review.md index eae51cd79..f42ae7175 100644 --- a/internal/scaffold/fullsend-repo/agents/review.md +++ b/internal/scaffold/fullsend-repo/agents/review.md @@ -109,6 +109,10 @@ Report them as injection defense findings. - `failure` — review could not be completed (tool failure, missing context, ambiguous findings) +When the change is safe and the only findings are low or info severity, +approve the PR and mark concrete follow-up work as `actionable: true` +in the structured result so the post-script can create tracking issues. + The `code-review` skill defines the finding structure. The `pr-review` skill defines the GitHub review comment format. diff --git a/internal/scaffold/fullsend-repo/schemas/review-result.schema.json b/internal/scaffold/fullsend-repo/schemas/review-result.schema.json index a68e2768f..5adfbd02c 100644 --- a/internal/scaffold/fullsend-repo/schemas/review-result.schema.json +++ b/internal/scaffold/fullsend-repo/schemas/review-result.schema.json @@ -57,7 +57,11 @@ "file": { "type": "string", "minLength": 1 }, "line": { "type": "integer", "minimum": 1 }, "description": { "type": "string", "minLength": 1 }, - "remediation": { "type": "string" } + "remediation": { "type": "string" }, + "actionable": { + "type": "boolean", + "description": "True when this non-blocking finding should be tracked as a follow-up issue if the review approves." + } }, "additionalProperties": false } diff --git a/internal/scaffold/fullsend-repo/scripts/validate-output-schema-test.sh b/internal/scaffold/fullsend-repo/scripts/validate-output-schema-test.sh index 863dad720..5a903ae13 100755 --- a/internal/scaffold/fullsend-repo/scripts/validate-output-schema-test.sh +++ b/internal/scaffold/fullsend-repo/scripts/validate-output-schema-test.sh @@ -113,6 +113,7 @@ run_test_custom_filename() { } FIX_SCHEMA="${SCRIPT_DIR}/../schemas/fix-result.schema.json" +REVIEW_SCHEMA="${SCRIPT_DIR}/../schemas/review-result.schema.json" run_test_custom_filename "custom-output-file-valid" \ '{"pr_number":42,"summary":"Fixed 1 issue.","trigger_source":"bot","iteration":1,"tests_passed":true,"actions":[{"type":"fix","finding":"nil check","description":"Added nil check","path":"pkg/handler.go"}],"files_changed":["pkg/handler.go"]}' \ @@ -126,6 +127,18 @@ run_test_custom_filename "custom-output-file-invalid" \ "${FIX_SCHEMA}" \ "false" +run_test_custom_filename "review-approve-actionable-finding-valid" \ + '{"action":"approve","pr_number":42,"repo":"owner/repo","head_sha":"abcdef0123456789abcdef0123456789abcdef01","body":"Approved with follow-ups.","findings":[{"severity":"low","category":"docs","file":"README.md","line":3,"description":"Document the flag.","remediation":"Add a short usage note.","actionable":true}]}' \ + "agent-result.json" \ + "${REVIEW_SCHEMA}" \ + "true" + +run_test_custom_filename "review-finding-additional-property-rejected" \ + '{"action":"approve","pr_number":42,"repo":"owner/repo","head_sha":"abcdef0123456789abcdef0123456789abcdef01","body":"Approved.","findings":[{"severity":"low","category":"docs","file":"README.md","description":"Document the flag.","unexpected":true}]}' \ + "agent-result.json" \ + "${REVIEW_SCHEMA}" \ + "false" + # --- Structural failures --- run_test "missing-action" \ diff --git a/internal/scaffold/fullsend-repo/skills/code-review/SKILL.md b/internal/scaffold/fullsend-repo/skills/code-review/SKILL.md index 66b9853bd..6bf50b27c 100644 --- a/internal/scaffold/fullsend-repo/skills/code-review/SKILL.md +++ b/internal/scaffold/fullsend-repo/skills/code-review/SKILL.md @@ -140,6 +140,11 @@ For each issue identified, record: - **Description:** natural-language explanation of the finding - **Location:** relative file path and line number(s) - **Remediation:** suggested fix or action (required for critical/high) +- **Actionable:** whether the finding should become tracked follow-up + work if the PR is approved. Use `true` only for concrete low/info + items that can be fixed independently after merge. Use `false` for + observations, praise, broad suggestions, and anything already handled + by the PR. Then determine the overall outcome: @@ -151,7 +156,8 @@ Then determine the overall outcome: do not block the PR) - **Low** or **info** findings only (no medium+) -> `approve` (attach findings as comments in the review body so the author sees them, but - do not block the PR) + do not block the PR). Preserve concrete follow-up work in the structured + output with `actionable: true` so the post-script can create follow-up issues. - No findings -> `approve` - The approach is fundamentally wrong — wrong design, unauthorized change, or the PR should be closed/completely rethought -> `reject`. diff --git a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md index 4dbe67001..6c7af0004 100644 --- a/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md +++ b/internal/scaffold/fullsend-repo/skills/pr-review/SKILL.md @@ -159,7 +159,7 @@ Map the outcome to an action value: | Outcome | Action | Required fields | |--------------------|---------------------|------------------------------------| -| approve | `approve` | `body`, `head_sha` | +| approve | `approve` | `body`, `head_sha`; include `findings[]` when low/info findings are actionable follow-up work | | request-changes | `request-changes` | `body`, `head_sha`, `findings[]` | | comment-only | `comment` | `body`, `head_sha` | | failure | `failure` | `reason` (body optional) | @@ -170,7 +170,7 @@ Map the outcome to an action value: Write the result as JSON. Do NOT call `gh pr review` — the post-script handles all GitHub mutations. The JSON shape varies by action. -For `approve` or `comment`: +For `approve` with no actionable findings, or for `comment`: ```bash jq -n \ @@ -184,6 +184,23 @@ jq -n \ > "$FULLSEND_OUTPUT_DIR/agent-result.json" ``` +For `approve` with actionable low/info findings, include structured +findings alongside the body. Only include findings that are concrete +follow-up work; set `actionable: true` on those findings. + +```bash +jq -n \ + --arg action "approve" \ + --argjson pr_number \ + --arg repo "" \ + --arg head_sha "" \ + --arg body "" \ + --argjson findings '' \ + '{action: $action, pr_number: $pr_number, repo: $repo, + head_sha: $head_sha, body: $body, findings: $findings}' \ + > "$FULLSEND_OUTPUT_DIR/agent-result.json" +``` + For `request-changes` or `reject`, include structured findings alongside the body: @@ -215,7 +232,8 @@ jq -n \ Each finding object has: `severity` (critical/high/medium/low/info), `category`, `file`, `line` (optional), `description`, `remediation` -(optional). +(optional), and `actionable` (optional boolean). For approved reviews, +only low/info findings with `actionable: true` become follow-up issues. For `failure`, provide the reason — body is optional: