diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index b0422bdf08..0f030fe72a 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -300,7 +300,23 @@ func submitFormalReview(ctx context.Context, client forge.Client, owner, repo st return nil } - inlineComments := findingsToReviewComments(findings) + var diffFiles map[string]bool + if prFiles, err := client.ListPullRequestFiles(ctx, owner, repo, pr); err != nil { + printer.StepInfo(fmt.Sprintf("Could not list PR files (%v), inline comments may be rejected", err)) + } else if len(prFiles) == 0 { + printer.StepInfo("PR file list is empty, inline comments disabled") + } else { + diffFiles = make(map[string]bool, len(prFiles)) + for _, f := range prFiles { + diffFiles[f] = true + } + } + + inlineComments, diffFiltered := findingsToReviewComments(findings, diffFiles) + + if diffFiltered > 0 { + printer.StepWarn(fmt.Sprintf("%d finding(s) omitted: file not in PR diff", diffFiltered)) + } var reviewBody string if event == "REQUEST_CHANGES" { @@ -324,19 +340,28 @@ func submitFormalReview(ctx context.Context, client forge.Client, owner, repo st // findingsToReviewComments converts review findings with file and line // locations into inline review comments. Findings without a file path // or line number are omitted — they remain in the sticky comment body. -func findingsToReviewComments(findings []ReviewFinding) []forge.ReviewComment { +// When diffFiles is non-nil, findings referencing files outside the PR +// diff are also omitted to avoid GitHub 422 errors. +// Returns the comments and the count of findings dropped because their +// file was not in the diff. +func findingsToReviewComments(findings []ReviewFinding, diffFiles map[string]bool) ([]forge.ReviewComment, int) { var comments []forge.ReviewComment + var diffFiltered int for _, f := range findings { if f.File == "" || f.Line <= 0 { continue } + if diffFiles != nil && !diffFiles[f.File] { + diffFiltered++ + continue + } comments = append(comments, forge.ReviewComment{ Path: f.File, Line: f.Line, Body: formatFindingComment(f), }) } - return comments + return comments, diffFiltered } // formatFindingComment renders a single review finding as a Markdown diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index b3cbc58b84..d2d49f947b 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -707,7 +707,8 @@ func TestFindingsToReviewComments(t *testing.T) { {File: "c.go", Line: 20, Severity: "critical", Category: "security", Description: "Desc C", Remediation: "Fix it"}, } - comments := findingsToReviewComments(findings) + comments, filtered := findingsToReviewComments(findings, nil) + assert.Equal(t, 0, filtered) require.Len(t, comments, 2) assert.Equal(t, "a.go", comments[0].Path) @@ -721,6 +722,86 @@ func TestFindingsToReviewComments(t *testing.T) { assert.Contains(t, comments[1].Body, "Fix it") } +func TestFindingsToReviewComments_FiltersByDiffFiles(t *testing.T) { + findings := []ReviewFinding{ + {File: "changed.go", Line: 10, Severity: "high", Category: "bug", Description: "In diff"}, + {File: "not-changed.go", Line: 5, Severity: "low", Category: "docs", Description: "Not in diff"}, + {File: "also-changed.go", Line: 20, Severity: "medium", Category: "style", Description: "Also in diff"}, + } + diffFiles := map[string]bool{ + "changed.go": true, + "also-changed.go": true, + } + + comments, filtered := findingsToReviewComments(findings, diffFiles) + assert.Equal(t, 1, filtered) + require.Len(t, comments, 2) + assert.Equal(t, "changed.go", comments[0].Path) + assert.Equal(t, "also-changed.go", comments[1].Path) +} + +func TestSubmitFormalReview_FiltersByPRFiles(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-bot" + fc.PRFiles = map[string][]string{ + "acme/repo/1": {"changed.go", "also-changed.go"}, + } + var out bytes.Buffer + printer := ui.New(&out) + + findings := []ReviewFinding{ + {File: "changed.go", Line: 10, Severity: "high", Category: "bug", Description: "In diff"}, + {File: "not-in-diff.go", Line: 5, Severity: "medium", Category: "style", Description: "Should be filtered"}, + {File: "also-changed.go", Line: 20, Severity: "low", Category: "docs", Description: "Also in diff"}, + } + + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, "request-changes", "", "", findings, false, printer) + require.NoError(t, err) + require.Len(t, fc.CreatedReviews, 1) + require.Len(t, fc.CreatedReviews[0].Comments, 2, "finding on not-in-diff.go should be filtered out") + assert.Equal(t, "changed.go", fc.CreatedReviews[0].Comments[0].Path) + assert.Equal(t, "also-changed.go", fc.CreatedReviews[0].Comments[1].Path) + assert.Contains(t, out.String(), "1 finding(s) omitted: file not in PR diff") +} + +func TestSubmitFormalReview_ListPRFilesErrorFallsBack(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-bot" + fc.Errors["ListPullRequestFiles"] = fmt.Errorf("API rate limited") + printer := ui.New(io.Discard) + + findings := []ReviewFinding{ + {File: "any-file.go", Line: 10, Severity: "high", Category: "bug", Description: "Should pass through"}, + } + + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, "request-changes", "", "", findings, false, printer) + require.NoError(t, err) + require.Len(t, fc.CreatedReviews, 1) + require.Len(t, fc.CreatedReviews[0].Comments, 1, "all comments should pass through when ListPullRequestFiles fails") + assert.Equal(t, "any-file.go", fc.CreatedReviews[0].Comments[0].Path) +} + +func TestSubmitFormalReview_EmptyPRFileListFallsBack(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-bot" + fc.PRFiles = map[string][]string{ + "acme/repo/1": {}, + } + var out bytes.Buffer + printer := ui.New(&out) + + findings := []ReviewFinding{ + {File: "any-file.go", Line: 10, Severity: "high", Category: "bug", Description: "Should pass through"}, + } + + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, "request-changes", "", "", findings, false, printer) + require.NoError(t, err) + require.Len(t, fc.CreatedReviews, 1) + // When PR file list is empty, filtering is disabled — all comments pass through unfiltered. + require.Len(t, fc.CreatedReviews[0].Comments, 1, "comments pass through unfiltered when PR file list is empty") + assert.Contains(t, out.String(), "PR file list is empty") +} + func TestFormatFindingComment(t *testing.T) { t.Run("with remediation", func(t *testing.T) { f := ReviewFinding{ diff --git a/internal/forge/fake.go b/internal/forge/fake.go index 5736670a56..088e3b0a10 100644 --- a/internal/forge/fake.go +++ b/internal/forge/fake.go @@ -136,6 +136,9 @@ type FakeClient struct { // Pull request head SHA for GetPullRequestHeadSHA. PullRequestHeadSHA string + // Pull request files for ListPullRequestFiles. + PRFiles map[string][]string // key: "owner/repo/number" + // Pull request reviews for ListPullRequestReviews. PRReviews map[string][]PullRequestReview // key: "owner/repo/number" @@ -800,6 +803,21 @@ func (f *FakeClient) GetPullRequestHeadSHA(_ context.Context, _, _ string, _ int return f.PullRequestHeadSHA, nil } +func (f *FakeClient) ListPullRequestFiles(_ context.Context, owner, repo string, number int) ([]string, error) { + f.mu.Lock() + defer f.mu.Unlock() + if e := f.err("ListPullRequestFiles"); e != nil { + return nil, e + } + if f.PRFiles != nil { + key := fmt.Sprintf("%s/%s/%d", owner, repo, number) + if files, ok := f.PRFiles[key]; ok { + return files, nil + } + } + return nil, nil +} + func (f *FakeClient) CreatePullRequestReview(_ context.Context, owner, repo string, number int, event, body, commitSHA string, comments []ReviewComment) error { f.mu.Lock() defer f.mu.Unlock() diff --git a/internal/forge/forge.go b/internal/forge/forge.go index f96851adad..298346fa40 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -211,6 +211,9 @@ type Client interface { // Pull request operations GetPullRequestHeadSHA(ctx context.Context, owner, repo string, number int) (string, error) + // ListPullRequestFiles returns the relative file paths changed by a pull + // request. On GitHub, the API caps results at 3000 files total. + ListPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]string, error) // Pull request review operations. // commitSHA, when non-empty, pins the review to a specific commit. diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index 7cc295cf61..0f9f6d95c1 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -1438,6 +1438,31 @@ func (c *LiveClient) GetPullRequestHeadSHA(ctx context.Context, owner, repo stri return pr.Head.SHA, nil } +// ListPullRequestFiles returns the file paths changed by a pull request. +// GitHub caps PR file lists at 3000 files total regardless of pagination. +func (c *LiveClient) ListPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]string, error) { + var files []string + for page := 1; page <= 100; page++ { + resp, err := c.get(ctx, fmt.Sprintf("/repos/%s/%s/pulls/%d/files?per_page=100&page=%d", owner, repo, number, page)) + if err != nil { + return nil, fmt.Errorf("list pull request files page %d: %w", page, err) + } + var raw []struct { + Filename string `json:"filename"` + } + if err := decodeJSON(resp, &raw); err != nil { + return nil, fmt.Errorf("decoding pull request files page %d: %w", page, err) + } + for _, f := range raw { + files = append(files, f.Filename) + } + if len(raw) < 100 { + break + } + } + return files, nil +} + // CreatePullRequestReview submits a formal review on a pull request. // The event must be one of: APPROVE, REQUEST_CHANGES, COMMENT. // When commitSHA is non-empty it is sent as commit_id, pinning the