Skip to content
3 changes: 0 additions & 3 deletions CLAUDE.md

This file was deleted.

43 changes: 32 additions & 11 deletions internal/cli/postreview.go
Original file line number Diff line number Diff line change
Expand Up @@ -326,13 +326,17 @@ func submitFormalReview(ctx context.Context, client forge.Client, owner, repo st
// accept review comments on lines outside the PR diff. The
// findings themselves remain in the sticky comment body and
// continue to influence the review verdict.
inlineComments, fileFiltered, lineFiltered := findingsToReviewComments(findings, diffHunks)
//
// Findings whose file is in the PR diff but whose line falls
// outside any diff hunk are posted as file-level comments so
// they remain visible on the PR code.
inlineComments, fileFiltered, fileLevelFallback := findingsToReviewComments(findings, diffHunks)

if fileFiltered > 0 {
printer.StepWarn(fmt.Sprintf("%d inline comment(s) omitted (file not in PR diff) — findings still count toward verdict", fileFiltered))
}
if lineFiltered > 0 {
printer.StepWarn(fmt.Sprintf("%d inline comment(s) omitted (line not in any diff hunk) — findings still count toward verdict", lineFiltered))
if fileLevelFallback > 0 {
printer.StepInfo(fmt.Sprintf("%d finding(s) posted as file-level comment(s) (line outside diff hunk)", fileLevelFallback))
}

// COMMENT verdicts skip the formal review unless there are inline-
Expand Down Expand Up @@ -366,15 +370,22 @@ 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.
//
// When diffHunks is non-nil, findings referencing files outside the PR
// diff or lines outside any diff hunk are omitted to avoid GitHub 422
// errors. Files with empty hunk lists (binary files, truncated patches)
// skip line-level filtering — the file is known to be in the diff but
// hunk coverage is unavailable. Returns the comments and counts of
// findings dropped for each reason (file not in diff, line not in hunk).
// diff are omitted to avoid GitHub 422 errors. Findings whose file is
// in the diff but whose line falls outside any diff hunk are posted as
// file-level comments (Line=0) so they remain visible on the PR code;
// the original line number is included in the comment body since file-
// level comments have no line annotation in the UI. Files with empty hunk lists (binary files, truncated
// patches) skip line-level filtering — the file is known to be in the
// diff but hunk coverage is unavailable.
//
// Returns the comments, count of findings dropped because their file
// was not in the diff, and count of findings that fell back to
// file-level comments.
func findingsToReviewComments(findings []ReviewFinding, diffHunks map[string][][2]int) ([]forge.ReviewComment, int, int) {
var comments []forge.ReviewComment
var fileFiltered, lineFiltered int
var fileFiltered, fileLevelFallback int
for _, f := range findings {
if f.File == "" || f.Line <= 0 {
continue
Expand All @@ -386,7 +397,17 @@ func findingsToReviewComments(findings []ReviewFinding, diffHunks map[string][][
continue
}
if len(hunks) > 0 && !lineInHunks(f.Line, hunks) {
lineFiltered++
// Fall back to file-level comments so findings
// remain visible on the PR even when the exact
// line is outside the changed region. Include the
// original line number in the body since file-level
// comments have no line annotation in the UI.
body := fmt.Sprintf("_Line %d_ · %s", f.Line, formatFindingComment(f))
comments = append(comments, forge.ReviewComment{
Path: f.File,
Body: body,
})
fileLevelFallback++
continue
}
}
Expand All @@ -396,7 +417,7 @@ func findingsToReviewComments(findings []ReviewFinding, diffHunks map[string][][
Body: formatFindingComment(f),
})
}
return comments, fileFiltered, lineFiltered
return comments, fileFiltered, fileLevelFallback
}

// formatFindingComment renders a single review finding as a Markdown
Expand Down
90 changes: 77 additions & 13 deletions internal/cli/postreview_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -826,9 +826,9 @@ func TestFindingsToReviewComments(t *testing.T) {
{File: "c.go", Line: 20, Severity: "critical", Category: "security", Description: "Desc C", Remediation: "Fix it"},
}

comments, fileFiltered, lineFiltered := findingsToReviewComments(findings, nil)
comments, fileFiltered, fileLevelFallback := findingsToReviewComments(findings, nil)
assert.Equal(t, 0, fileFiltered)
assert.Equal(t, 0, lineFiltered)
assert.Equal(t, 0, fileLevelFallback)
require.Len(t, comments, 2)

assert.Equal(t, "a.go", comments[0].Path)
Expand All @@ -854,14 +854,18 @@ func TestFindingsToReviewComments_FiltersByDiffHunks(t *testing.T) {
"also-changed.go": {{1, 10}},
}

comments, fileFiltered, lineFiltered := findingsToReviewComments(findings, diffHunks)
comments, fileFiltered, fileLevelFallback := findingsToReviewComments(findings, diffHunks)
assert.Equal(t, 1, fileFiltered)
assert.Equal(t, 1, lineFiltered)
require.Len(t, comments, 2)
assert.Equal(t, 1, fileLevelFallback, "low-severity out-of-hunk finding should fall back to file-level")
require.Len(t, comments, 3)
assert.Equal(t, "changed.go", comments[0].Path)
assert.Equal(t, 10, comments[0].Line)
assert.Equal(t, "also-changed.go", comments[1].Path)
assert.Equal(t, 3, comments[1].Line)
// The out-of-hunk low finding now falls back to file-level.
assert.Equal(t, "changed.go", comments[1].Path)
assert.Equal(t, 0, comments[1].Line)
assert.Contains(t, comments[1].Body, "Line 50", "file-level fallback should include original line number")
assert.Equal(t, "also-changed.go", comments[2].Path)
assert.Equal(t, 3, comments[2].Line)
}

func TestFindingsToReviewComments_EmptyPatchSkipsLineFiltering(t *testing.T) {
Expand All @@ -877,14 +881,69 @@ func TestFindingsToReviewComments_EmptyPatchSkipsLineFiltering(t *testing.T) {
"changed.go": {{5, 15}},
}

comments, fileFiltered, lineFiltered := findingsToReviewComments(findings, diffHunks)
comments, fileFiltered, fileLevelFallback := findingsToReviewComments(findings, diffHunks)
assert.Equal(t, 0, fileFiltered)
assert.Equal(t, 1, lineFiltered, "only the out-of-hunk finding on changed.go should be filtered")
require.Len(t, comments, 3)
assert.Equal(t, 1, fileLevelFallback, "out-of-hunk info finding on changed.go should fall back to file-level")
require.Len(t, comments, 4)
assert.Equal(t, "binary.png", comments[0].Path)
assert.Equal(t, "large.go", comments[1].Path)
assert.Equal(t, "changed.go", comments[2].Path)
assert.Equal(t, 10, comments[2].Line)
// The info finding outside the hunk now falls back to file-level.
assert.Equal(t, "changed.go", comments[3].Path)
assert.Equal(t, 0, comments[3].Line)
assert.Contains(t, comments[3].Body, "Line 50", "file-level fallback should include original line number")
}

func TestFindingsToReviewComments_AllSeveritiesPassThrough(t *testing.T) {
findings := []ReviewFinding{
{File: "a.go", Line: 10, Severity: "info", Category: "docs", Description: "Info finding with location"},
{File: "a.go", Line: 15, Severity: "Info", Category: "docs", Description: "Info finding case insensitive"},
{File: "a.go", Line: 20, Severity: "low", Category: "style", Description: "Low finding"},
{File: "a.go", Line: 25, Severity: "medium", Category: "bug", Description: "Medium finding"},
}

comments, fileFiltered, fileLevelFallback := findingsToReviewComments(findings, nil)
assert.Equal(t, 0, fileFiltered)
assert.Equal(t, 0, fileLevelFallback)
require.Len(t, comments, 4, "all findings should pass through regardless of severity")
assert.Contains(t, comments[0].Body, "Info finding with location")
assert.Contains(t, comments[1].Body, "Info finding case insensitive")
assert.Contains(t, comments[2].Body, "Low finding")
assert.Contains(t, comments[3].Body, "Medium finding")
}

func TestFindingsToReviewComments_AllSeveritiesFallbackToFileLevel(t *testing.T) {
findings := []ReviewFinding{
{File: "changed.go", Line: 10, Severity: "high", Category: "bug", Description: "In hunk"},
{File: "changed.go", Line: 50, Severity: "medium", Category: "logic-error", Description: "Medium outside hunk"},
{File: "changed.go", Line: 60, Severity: "critical", Category: "security", Description: "Critical outside hunk"},
{File: "changed.go", Line: 70, Severity: "low", Category: "style", Description: "Low outside hunk"},
{File: "changed.go", Line: 75, Severity: "info", Category: "docs", Description: "Info outside hunk"},
{File: "changed.go", Line: 80, Severity: "High", Category: "bug", Description: "High outside hunk case insensitive"},
}
diffHunks := map[string][][2]int{
"changed.go": {{5, 15}},
}

comments, fileFiltered, fileLevelFallback := findingsToReviewComments(findings, diffHunks)
assert.Equal(t, 0, fileFiltered)
assert.Equal(t, 5, fileLevelFallback, "all out-of-hunk findings should fall back to file-level")
require.Len(t, comments, 6)

// First comment: in-hunk high finding with line number.
assert.Equal(t, "changed.go", comments[0].Path)
assert.Equal(t, 10, comments[0].Line)

// Remaining: file-level fallback comments for all out-of-hunk findings.
expectedLines := []int{50, 60, 70, 75, 80}
for i, desc := range []string{"Medium outside hunk", "Critical outside hunk", "Low outside hunk", "Info outside hunk", "High outside hunk case insensitive"} {
idx := i + 1
assert.Equal(t, "changed.go", comments[idx].Path)
assert.Equal(t, 0, comments[idx].Line, "file-level comment should have Line=0")
assert.Contains(t, comments[idx].Body, desc)
assert.Contains(t, comments[idx].Body, fmt.Sprintf("Line %d", expectedLines[i]), "file-level fallback should include original line number")
}
}

func TestSubmitFormalReview_FiltersByPRFileDiffs(t *testing.T) {
Expand All @@ -909,11 +968,16 @@ func TestSubmitFormalReview_FiltersByPRFileDiffs(t *testing.T) {
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, "file-filtered and line-filtered findings should be omitted")
require.Len(t, fc.CreatedReviews[0].Comments, 3, "file-not-in-diff finding omitted; out-of-hunk finding falls back to file-level")
assert.Equal(t, "changed.go", fc.CreatedReviews[0].Comments[0].Path)
assert.Equal(t, "also-changed.go", fc.CreatedReviews[0].Comments[1].Path)
assert.Equal(t, 10, fc.CreatedReviews[0].Comments[0].Line)
// Out-of-hunk low finding falls back to file-level comment.
assert.Equal(t, "changed.go", fc.CreatedReviews[0].Comments[1].Path)
assert.Equal(t, 0, fc.CreatedReviews[0].Comments[1].Line)
assert.Contains(t, fc.CreatedReviews[0].Comments[1].Body, "Line 50", "file-level fallback should include original line number")
assert.Equal(t, "also-changed.go", fc.CreatedReviews[0].Comments[2].Path)
assert.Contains(t, out.String(), "1 inline comment(s) omitted (file not in PR diff) — findings still count toward verdict")
assert.Contains(t, out.String(), "1 inline comment(s) omitted (line not in any diff hunk) — findings still count toward verdict")
assert.Contains(t, out.String(), "1 finding(s) posted as file-level comment(s) (line outside diff hunk)")
}

func TestSubmitFormalReview_ListPRFileDiffsErrorFallsBack(t *testing.T) {
Expand Down
8 changes: 7 additions & 1 deletion internal/forge/forge.go
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,15 @@ type PullRequestReview struct {
// ReviewComment represents an inline comment on a specific line of a
// pull request diff. These are submitted as part of a formal PR review
// via the GitHub "Create a review" API.
//
// When Line is 0, the comment is attached to the file as a whole rather
// than a specific line. This is used for findings that reference a file
// in the diff but a line outside any diff hunk. Forge implementations
// translate Line==0 into the appropriate API representation (e.g.,
// GitHub's subject_type: "file").
type ReviewComment struct {
Path string // relative file path in the repository
Line int // line number in the diff (right side)
Line int // line number in the diff (right side); 0 for file-level comments
Body string // comment body (Markdown)
}

Expand Down
18 changes: 13 additions & 5 deletions internal/forge/github/github.go
Original file line number Diff line number Diff line change
Expand Up @@ -1754,11 +1754,15 @@ func (c *LiveClient) CreatePullRequestReview(ctx context.Context, owner, repo st
}

type reviewComment struct {
Path string `json:"path"`
Line int `json:"line,omitempty"`
Body string `json:"body"`
Path string `json:"path"`
Line int `json:"line,omitempty"`
Body string `json:"body"`
SubjectType string `json:"subject_type,omitempty"`
}

// GitHub's subject_type: "file" is inferred from Line==0 so forge
// callers don't need to know about this GitHub-specific field.

type reviewPayload struct {
Event string `json:"event"`
Body string `json:"body"`
Expand All @@ -1772,11 +1776,15 @@ func (c *LiveClient) CreatePullRequestReview(ctx context.Context, owner, repo st
CommitID: commitSHA,
}
for _, rc := range comments {
payload.Comments = append(payload.Comments, reviewComment{
c := reviewComment{
Path: rc.Path,
Line: rc.Line,
Body: rc.Body,
})
}
if rc.Line == 0 {
c.SubjectType = "file"
}
payload.Comments = append(payload.Comments, c)
}

resp, err := c.post(ctx, fmt.Sprintf("/repos/%s/%s/pulls/%d/reviews", owner, repo, number), payload)
Expand Down
8 changes: 8 additions & 0 deletions qf-tests/GH-41/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
# QualityFlow Tests — GH-41

Generated by the QualityFlow pipeline.

| Directory | Count | Framework |
|-----------|-------|-----------|
| `go/` | 1 files | Go |
| `python/` | 2 files | Python/pytest |
Loading
Loading