Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 28 additions & 3 deletions internal/cli/postreview.go
Original file line number Diff line number Diff line change
Expand Up @@ -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" {
Expand All @@ -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
Comment thread
rh-hemartin marked this conversation as resolved.
}
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
Expand Down
83 changes: 82 additions & 1 deletion internal/cli/postreview_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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{
Expand Down
18 changes: 18 additions & 0 deletions internal/forge/fake.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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()
Expand Down
3 changes: 3 additions & 0 deletions internal/forge/forge.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
25 changes: 25 additions & 0 deletions internal/forge/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading