diff --git a/CLAUDE.md b/CLAUDE.md deleted file mode 100644 index 32b39573f1..0000000000 --- a/CLAUDE.md +++ /dev/null @@ -1,3 +0,0 @@ -# CLAUDE.md - -Project rules and instructions live in [AGENTS.md](AGENTS.md). Read that file now — it is the single source of truth for all agent-facing guidance in this repo. diff --git a/internal/cli/postreview.go b/internal/cli/postreview.go index eb9be86eb2..6ef89a7aeb 100644 --- a/internal/cli/postreview.go +++ b/internal/cli/postreview.go @@ -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- @@ -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 @@ -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 } } @@ -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 diff --git a/internal/cli/postreview_test.go b/internal/cli/postreview_test.go index 05b7866ca0..5be6ac4be1 100644 --- a/internal/cli/postreview_test.go +++ b/internal/cli/postreview_test.go @@ -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) @@ -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) { @@ -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) { @@ -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) { diff --git a/internal/forge/forge.go b/internal/forge/forge.go index b6b295aca2..85a3a013e0 100644 --- a/internal/forge/forge.go +++ b/internal/forge/forge.go @@ -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) } diff --git a/internal/forge/github/github.go b/internal/forge/github/github.go index b110b55c3d..f0b3e2bb88 100644 --- a/internal/forge/github/github.go +++ b/internal/forge/github/github.go @@ -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"` @@ -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) diff --git a/qf-tests/GH-41/README.md b/qf-tests/GH-41/README.md new file mode 100644 index 0000000000..c27f6e4d1b --- /dev/null +++ b/qf-tests/GH-41/README.md @@ -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 | diff --git a/qf-tests/GH-41/go/findings_to_review_comments_test.go b/qf-tests/GH-41/go/findings_to_review_comments_test.go new file mode 100644 index 0000000000..419b0d3420 --- /dev/null +++ b/qf-tests/GH-41/go/findings_to_review_comments_test.go @@ -0,0 +1,352 @@ +package cli + +import ( + "bytes" + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/fullsend-ai/fullsend/internal/forge" + "github.com/fullsend-ai/fullsend/internal/ui" +) + +// TestFindingsToReviewComments_OutOfHunkFallbackToFileLevel verifies that a +// finding whose line falls outside every diff hunk is posted as a file-level +// comment with Line=0. This is the core behavioral change in GH-41. +// [test_id:TS-GH-41-001] +func TestFindingsToReviewComments_OutOfHunkFallbackToFileLevel(t *testing.T) { + findings := []ReviewFinding{ + {File: "main.go", Line: 150, Severity: "high", Category: "bug", Description: "Potential nil dereference"}, + } + diffHunks := map[string][][2]int{ + "main.go": {{10, 30}, {50, 70}}, + } + + comments, fileFiltered, fileLevelFallback := findingsToReviewComments(findings, diffHunks) + + require.Len(t, comments, 1, "out-of-hunk finding must not be dropped") + assert.Equal(t, 0, comments[0].Line, "out-of-hunk finding should become file-level (Line=0)") + assert.Equal(t, "main.go", comments[0].Path) + assert.Equal(t, 0, fileFiltered) + assert.Equal(t, 1, fileLevelFallback) +} + +// TestFindingsToReviewComments_NoFilePathSkipped verifies that findings with +// an empty file path are silently skipped and produce no ReviewComment. +// [test_id:TS-GH-41-002] +func TestFindingsToReviewComments_NoFilePathSkipped(t *testing.T) { + findings := []ReviewFinding{ + {File: "", Line: 10, Severity: "medium", Category: "style", Description: "General code smell"}, + } + diffHunks := map[string][][2]int{ + "main.go": {{1, 100}}, + } + + comments, fileFiltered, fileLevelFallback := findingsToReviewComments(findings, diffHunks) + + assert.Empty(t, comments, "finding without file path must be skipped") + assert.Equal(t, 0, fileFiltered) + assert.Equal(t, 0, fileLevelFallback) +} + +// TestFindingsToReviewComments_FallbackBodyContainsOriginalLine verifies that +// when a finding falls back to file-level, the comment body includes the +// original line number so reviewers retain location context. +// [test_id:TS-GH-41-004] +func TestFindingsToReviewComments_FallbackBodyContainsOriginalLine(t *testing.T) { + findings := []ReviewFinding{ + {File: "main.go", Line: 150, Severity: "high", Category: "bug", Description: "Potential nil dereference"}, + } + diffHunks := map[string][][2]int{ + "main.go": {{10, 30}, {50, 70}}, + } + + comments, _, _ := findingsToReviewComments(findings, diffHunks) + + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "150", "fallback body must contain original line number") +} + +// TestFindingsToReviewComments_FallbackBodyFormat verifies that file-level +// fallback comments use the exact format "_Line N_ . description". +// [test_id:TS-GH-41-005] +func TestFindingsToReviewComments_FallbackBodyFormat(t *testing.T) { + findings := []ReviewFinding{ + {File: "main.go", Line: 42, Severity: "medium", Category: "style", Description: "Unused variable detected"}, + } + diffHunks := map[string][][2]int{ + "main.go": {{10, 30}}, + } + + comments, _, _ := findingsToReviewComments(findings, diffHunks) + + require.Len(t, comments, 1) + assert.Contains(t, comments[0].Body, "_Line 42_", "fallback body must start with '_Line N_' prefix") + assert.Contains(t, comments[0].Body, "Unused variable detected") + assert.Contains(t, comments[0].Body, "**[medium]** style") +} + +// TestFindingsToReviewComments_InHunkRetainsLine verifies that findings whose +// line falls within a diff hunk retain the original line number and are not +// converted to file-level comments. +// [test_id:TS-GH-41-006] +func TestFindingsToReviewComments_InHunkRetainsLine(t *testing.T) { + findings := []ReviewFinding{ + {File: "main.go", Line: 25, Severity: "high", Category: "bug", Description: "Missing error check"}, + } + diffHunks := map[string][][2]int{ + "main.go": {{10, 30}}, + } + + comments, _, fileLevelFallback := findingsToReviewComments(findings, diffHunks) + + require.Len(t, comments, 1) + assert.Equal(t, 25, comments[0].Line, "in-hunk finding must retain its original line number") + assert.Equal(t, 0, fileLevelFallback) +} + +// TestFindingsToReviewComments_InHunkBodyNoLinePrefix verifies that in-hunk +// findings do NOT get the "_Line N_" prefix in their body, since they display +// at the correct line in the diff view. +// [test_id:TS-GH-41-007] +func TestFindingsToReviewComments_InHunkBodyNoLinePrefix(t *testing.T) { + findings := []ReviewFinding{ + {File: "main.go", Line: 25, Severity: "high", Category: "bug", Description: "Missing error check"}, + } + diffHunks := map[string][][2]int{ + "main.go": {{10, 30}}, + } + + comments, _, _ := findingsToReviewComments(findings, diffHunks) + + require.Len(t, comments, 1) + assert.NotContains(t, comments[0].Body, "_Line", "in-hunk comment body must not contain '_Line' prefix") + assert.Contains(t, comments[0].Body, "Missing error check") +} + +// TestFindingsToReviewComments_FileNotInDiffOmitted verifies that findings +// referencing files not present in diffHunks are filtered out entirely and +// increment the fileFiltered counter. +// [test_id:TS-GH-41-008] +func TestFindingsToReviewComments_FileNotInDiffOmitted(t *testing.T) { + findings := []ReviewFinding{ + {File: "other_file.go", Line: 10, Severity: "high", Category: "bug", Description: "Issue in unrelated file"}, + } + diffHunks := map[string][][2]int{ + "main.go": {{1, 100}}, + } + + comments, fileFiltered, fileLevelFallback := findingsToReviewComments(findings, diffHunks) + + assert.Empty(t, comments, "finding for file not in diff must be omitted") + assert.Equal(t, 1, fileFiltered) + assert.Equal(t, 0, fileLevelFallback) +} + +// TestFindingsToReviewComments_FileFilteredCount verifies that the +// fileFiltered counter accurately reflects the number of findings whose +// file is not in the PR diff. +// [test_id:TS-GH-41-009] +func TestFindingsToReviewComments_FileFilteredCount(t *testing.T) { + findings := []ReviewFinding{ + {File: "not-in-diff-1.go", Line: 10, Severity: "high", Category: "bug", Description: "Filtered 1"}, + {File: "not-in-diff-2.go", Line: 20, Severity: "medium", Category: "bug", Description: "Filtered 2"}, + {File: "in-diff.go", Line: 5, Severity: "low", Category: "style", Description: "Kept"}, + } + diffHunks := map[string][][2]int{ + "in-diff.go": {{1, 50}}, + } + + comments, fileFiltered, _ := findingsToReviewComments(findings, diffHunks) + + require.Len(t, comments, 1) + assert.Equal(t, 2, fileFiltered, "two findings for files not in diff should be counted") +} + +// TestFindingsToReviewComments_AllSeveritiesFallbackEqually verifies that +// the file-level fallback applies uniformly to all severity levels. +// [test_id:TS-GH-41-010] +func TestFindingsToReviewComments_AllSeveritiesFallbackEqually(t *testing.T) { + severities := []string{"info", "warning", "error", "critical"} + + for _, sev := range severities { + t.Run(sev, func(t *testing.T) { + findings := []ReviewFinding{ + {File: "main.go", Line: 150, Severity: sev, Category: "test", Description: "Out of hunk"}, + } + diffHunks := map[string][][2]int{ + "main.go": {{10, 30}}, + } + + comments, _, fileLevelFallback := findingsToReviewComments(findings, diffHunks) + + require.Len(t, comments, 1, "severity %q must not be filtered", sev) + assert.Equal(t, 0, comments[0].Line, "severity %q must fall back to file-level", sev) + assert.Equal(t, 1, fileLevelFallback) + }) + } +} + +// TestFindingsToReviewComments_CaseInsensitiveSeverity verifies that severity +// comparison is case-insensitive and mixed-case values all produce file-level +// fallback comments equally. +// [test_id:TS-GH-41-011] +func TestFindingsToReviewComments_CaseInsensitiveSeverity(t *testing.T) { + caseVariants := []string{"HIGH", "High", "high"} + + for _, sev := range caseVariants { + t.Run(sev, func(t *testing.T) { + findings := []ReviewFinding{ + {File: "main.go", Line: 150, Severity: sev, Category: "bug", Description: "Out of hunk"}, + } + diffHunks := map[string][][2]int{ + "main.go": {{10, 30}}, + } + + comments, _, fileLevelFallback := findingsToReviewComments(findings, diffHunks) + + require.Len(t, comments, 1, "severity %q must produce a comment", sev) + assert.Equal(t, 0, comments[0].Line, "severity %q must fall back to file-level", sev) + assert.Equal(t, 1, fileLevelFallback) + }) + } +} + +// TestFindingsToReviewComments_Line0ImpliesFileLevelSubjectType verifies that +// findingsToReviewComments outputs Line=0 for out-of-hunk findings, which the +// GitHub implementation translates to subject_type="file" in the API payload. +// [test_id:TS-GH-41-012] +func TestFindingsToReviewComments_Line0ImpliesFileLevelSubjectType(t *testing.T) { + findings := []ReviewFinding{ + {File: "main.go", Line: 150, Severity: "high", Category: "bug", Description: "Potential nil dereference"}, + } + diffHunks := map[string][][2]int{ + "main.go": {{10, 30}}, + } + + comments, _, _ := findingsToReviewComments(findings, diffHunks) + + require.Len(t, comments, 1) + assert.Equal(t, 0, comments[0].Line, "Line=0 signals file-level; GitHub client sets subject_type='file'") + assert.Equal(t, "main.go", comments[0].Path) +} + +// TestFindingsToReviewComments_InHunkLinePositiveNoSubjectType verifies that +// in-hunk findings produce Line>0, which the GitHub implementation handles +// by omitting subject_type from the API payload. +// [test_id:TS-GH-41-013] +func TestFindingsToReviewComments_InHunkLinePositiveNoSubjectType(t *testing.T) { + findings := []ReviewFinding{ + {File: "main.go", Line: 25, Severity: "high", Category: "bug", Description: "Missing error check"}, + } + diffHunks := map[string][][2]int{ + "main.go": {{10, 30}}, + } + + comments, _, _ := findingsToReviewComments(findings, diffHunks) + + require.Len(t, comments, 1) + assert.Greater(t, comments[0].Line, 0, "in-hunk finding must have positive Line (no subject_type in API)") + assert.Equal(t, 25, comments[0].Line) +} + +// TestFindingsToReviewComments_BinaryFileSkipsLineFiltering verifies that +// findings for binary files (present in diffHunks with nil/empty hunk list) +// bypass line-level filtering entirely and pass through. +// [test_id:TS-GH-41-015] +func TestFindingsToReviewComments_BinaryFileSkipsLineFiltering(t *testing.T) { + findings := []ReviewFinding{ + {File: "binary.png", Line: 1, Severity: "high", Category: "bug", Description: "On binary file"}, + } + diffHunks := map[string][][2]int{ + "binary.png": nil, + } + + comments, fileFiltered, fileLevelFallback := findingsToReviewComments(findings, diffHunks) + + require.Len(t, comments, 1, "binary file finding must not be filtered") + assert.Equal(t, "binary.png", comments[0].Path) + assert.Equal(t, 1, comments[0].Line, "binary file finding should retain original line") + assert.Equal(t, 0, fileFiltered) + assert.Equal(t, 0, fileLevelFallback) +} + +// TestFindingsToReviewComments_TruncatedPatchSkipsLineFiltering verifies that +// findings for files with truncated patches (empty hunk list) bypass line-level +// filtering and pass through, same as binary files. +// [test_id:TS-GH-41-016] +func TestFindingsToReviewComments_TruncatedPatchSkipsLineFiltering(t *testing.T) { + findings := []ReviewFinding{ + {File: "large.go", Line: 999, Severity: "medium", Category: "style", Description: "On truncated-patch file"}, + } + diffHunks := map[string][][2]int{ + "large.go": nil, + } + + comments, fileFiltered, fileLevelFallback := findingsToReviewComments(findings, diffHunks) + + require.Len(t, comments, 1, "truncated-patch file finding must not be filtered") + assert.Equal(t, "large.go", comments[0].Path) + assert.Equal(t, 999, comments[0].Line, "truncated-patch finding should retain original line") + assert.Equal(t, 0, fileFiltered) + assert.Equal(t, 0, fileLevelFallback) +} + +// TestSubmitFormalReview_LogsFileLevelFallbackCount verifies that when +// out-of-hunk findings fall back to file-level comments, the printer output +// includes a StepInfo message with the fallback count. +// [test_id:TS-GH-41-017] +func TestSubmitFormalReview_LogsFileLevelFallbackCount(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-bot" + fc.PRFileDiffs = map[string][]forge.PullRequestFileDiff{ + "acme/repo/1": { + {Path: "main.go", Patch: "@@ -10,20 +10,25 @@ func main() {"}, + }, + } + + var out bytes.Buffer + printer := ui.New(&out) + + findings := []ReviewFinding{ + {File: "main.go", Line: 10, Severity: "high", Category: "bug", Description: "In hunk"}, + {File: "main.go", Line: 100, Severity: "medium", Category: "style", Description: "Outside hunk"}, + } + + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, "request-changes", "", "", findings, false, printer) + require.NoError(t, err) + + output := out.String() + assert.Contains(t, output, "file-level comment", "printer output must mention file-level fallback") + assert.Contains(t, output, "1 finding(s) posted as file-level comment(s)", "printer output must contain the correct fallback count") +} + +// TestSubmitFormalReview_NoFallbackLogWhenCountZero verifies that when all +// findings are in-hunk (no fallbacks), the printer output does not contain +// any file-level fallback message. +// [test_id:TS-GH-41-018] +func TestSubmitFormalReview_NoFallbackLogWhenCountZero(t *testing.T) { + fc := forge.NewFakeClient() + fc.AuthenticatedUser = "fullsend-bot" + fc.PRFileDiffs = map[string][]forge.PullRequestFileDiff{ + "acme/repo/1": { + {Path: "main.go", Patch: "@@ -10,20 +10,25 @@ func main() {"}, + }, + } + + var out bytes.Buffer + printer := ui.New(&out) + + findings := []ReviewFinding{ + {File: "main.go", Line: 15, Severity: "high", Category: "bug", Description: "In hunk"}, + {File: "main.go", Line: 20, Severity: "medium", Category: "style", Description: "Also in hunk"}, + } + + err := submitFormalReview(context.Background(), fc, "acme", "repo", 1, "request-changes", "", "", findings, false, printer) + require.NoError(t, err) + + output := out.String() + assert.NotContains(t, output, "file-level comment", "no fallback log when all findings are in-hunk") +} diff --git a/qf-tests/GH-41/python/conftest.py b/qf-tests/GH-41/python/conftest.py new file mode 100644 index 0000000000..84f89d5c13 --- /dev/null +++ b/qf-tests/GH-41/python/conftest.py @@ -0,0 +1,73 @@ +"""Shared fixtures for GH-41 e2e tests.""" +import json +import logging +import os +import subprocess + +import pytest + +logger = logging.getLogger(__name__) + + +@pytest.fixture +def gh_api(): + """Helper to call GitHub API via gh CLI. + + Returns a callable that invokes ``gh api`` with the given HTTP method, + endpoint, and optional JSON body. The raw ``subprocess.CompletedProcess`` + is returned so callers can inspect both stdout and returncode. + """ + + def _call(method, endpoint, data=None): + cmd = ["gh", "api", "-X", method, endpoint] + if data: + input_data = json.dumps(data) + cmd.extend(["--input", "-"]) + result = subprocess.run( + cmd, + input=input_data, + capture_output=True, + text=True, + timeout=30, + ) + else: + result = subprocess.run(cmd, capture_output=True, text=True, timeout=30) + logger.debug( + "gh api %s %s -> rc=%d stdout=%s stderr=%s", + method, + endpoint, + result.returncode, + result.stdout[:200], + result.stderr[:200], + ) + return result + + return _call + + +@pytest.fixture +def github_repo(): + """Return the test repository in owner/repo format.""" + return os.environ.get("TEST_REPO", "") + + +@pytest.fixture +def pr_number(): + """Return the test PR number.""" + return int(os.environ.get("TEST_PR_NUMBER", "0")) + + +@pytest.fixture +def pr_files(gh_api, github_repo, pr_number): + """Return the list of files changed in the test PR. + + This is used by tests that need to pick a real file path present in + the PR diff when constructing review comment payloads. + """ + result = gh_api( + "GET", + f"/repos/{github_repo}/pulls/{pr_number}/files", + ) + assert result.returncode == 0, f"Failed to fetch PR files: {result.stderr}" + files = json.loads(result.stdout) + return [f["filename"] for f in files] diff --git a/qf-tests/GH-41/python/test_file_level_comment_e2e.py b/qf-tests/GH-41/python/test_file_level_comment_e2e.py new file mode 100644 index 0000000000..39c0138d05 --- /dev/null +++ b/qf-tests/GH-41/python/test_file_level_comment_e2e.py @@ -0,0 +1,365 @@ +""" +File-Level Comment End-to-End Tests -- GH-41 + +Tests that file-level comments (subject_type: "file") are correctly +handled by the GitHub Pull Request Review API. + +STP Reference: outputs/stp/GH-41/GH-41_test_plan.md +Jira: GH-41 +""" +import json +import logging +import os +import subprocess +import time + +import pytest + +logger = logging.getLogger(__name__) + +GITHUB_TOKEN = os.environ.get("GITHUB_TOKEN", "") +REPO = os.environ.get("TEST_REPO", "") +PR_NUMBER = os.environ.get("TEST_PR_NUMBER", "") + +pytestmark = [ + pytest.mark.e2e, + pytest.mark.tier2, + pytest.mark.skipif( + not all([GITHUB_TOKEN, REPO, PR_NUMBER]), + reason="Requires GITHUB_TOKEN, TEST_REPO, and TEST_PR_NUMBER environment variables", + ), +] + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _gh_api(method, endpoint, data=None): + """Call GitHub API via gh CLI and return the CompletedProcess.""" + cmd = ["gh", "api", "-X", method, endpoint] + if data is not None: + input_data = json.dumps(data) + cmd.extend(["--input", "-"]) + result = subprocess.run( + cmd, + input=input_data, + capture_output=True, + text=True, + timeout=30, + ) + else: + result = subprocess.run(cmd, capture_output=True, text=True, timeout=30) + return result + + +def _get_pr_files(): + """Return filenames changed in the test PR.""" + result = _gh_api("GET", f"/repos/{REPO}/pulls/{PR_NUMBER}/files") + assert result.returncode == 0, f"Failed to list PR files: {result.stderr}" + return [f["filename"] for f in json.loads(result.stdout)] + + +def _dismiss_review(review_id): + """Best-effort dismiss a review so tests leave no lasting side-effects.""" + try: + _gh_api( + "PUT", + f"/repos/{REPO}/pulls/{PR_NUMBER}/reviews/{review_id}/dismissals", + data={"message": "Automated test cleanup"}, + ) + logger.info("Dismissed review %s", review_id) + except Exception: + logger.warning("Could not dismiss review %s", review_id, exc_info=True) + + +def _delete_review_comment(comment_id): + """Best-effort delete a pull request review comment.""" + try: + _gh_api( + "DELETE", + f"/repos/{REPO}/pulls/comments/{comment_id}", + ) + logger.info("Deleted review comment %s", comment_id) + except Exception: + logger.warning( + "Could not delete review comment %s", comment_id, exc_info=True + ) + + +def _submit_file_level_review(file_path, body_text, event="COMMENT"): + """Submit a PR review containing a single file-level comment. + + A file-level comment has ``subject_type: "file"`` and no ``line`` field. + + Returns: + tuple: (review_id, parsed response dict) + """ + payload = { + "event": event, + "body": f"GH-41 automated e2e test review ({body_text})", + "comments": [ + { + "path": file_path, + "body": body_text, + "subject_type": "file", + } + ], + } + result = _gh_api( + "POST", + f"/repos/{REPO}/pulls/{PR_NUMBER}/reviews", + data=payload, + ) + logger.info( + "Review submission rc=%d stdout=%s stderr=%s", + result.returncode, + result.stdout[:300], + result.stderr[:300], + ) + assert result.returncode == 0, ( + f"Review submission failed (rc={result.returncode}): {result.stderr}" + ) + response = json.loads(result.stdout) + review_id = response.get("id") + assert review_id is not None, "Review response missing 'id' field" + return review_id, response + + +def _get_review_comments(review_id): + """Fetch all comments belonging to a specific review.""" + result = _gh_api( + "GET", + f"/repos/{REPO}/pulls/{PR_NUMBER}/reviews/{review_id}/comments", + ) + assert result.returncode == 0, ( + f"Failed to fetch review comments: {result.stderr}" + ) + return json.loads(result.stdout) + + +def _get_pr_review_comments(): + """Fetch all review comments on the PR.""" + result = _gh_api( + "GET", + f"/repos/{REPO}/pulls/{PR_NUMBER}/comments", + ) + assert result.returncode == 0, ( + f"Failed to fetch PR review comments: {result.stderr}" + ) + return json.loads(result.stdout) + + +# --------------------------------------------------------------------------- +# Tests +# --------------------------------------------------------------------------- + + +class TestFileLevelCommentPersistence: + """TS-GH-41-003: File-level comments survive review re-submission. + + Verifies that file-level comments persist when a PR review is + re-submitted. This tests the full GitHub API lifecycle for + file-level comments created via subject_type: "file". + """ + + def test_file_level_comments_persist_after_resubmission(self): + """[test_id:TS-GH-41-003] File-level comments survive review re-submission. + + Steps: + 1. Identify a file in the PR diff. + 2. Submit a review with a file-level comment (subject_type: "file"). + 3. Verify the file-level comment exists on the review. + 4. Submit a second review with another file-level comment. + 5. Verify that both sets of file-level comments are present on the PR. + """ + files = _get_pr_files() + assert len(files) > 0, "Test PR has no changed files" + target_file = files[0] + logger.info("Using target file: %s", target_file) + + review_ids = [] + comment_ids = [] + + try: + # -- Step 1: Submit first review with a file-level comment ------ + first_body = ( + "TS-GH-41-003 first submission: " + "file-level comment persistence test" + ) + review_id_1, _ = _submit_file_level_review(target_file, first_body) + review_ids.append(review_id_1) + logger.info("First review created: %s", review_id_1) + + # Allow a brief propagation window + time.sleep(2) + + # Verify the file-level comment exists on the first review + comments_1 = _get_review_comments(review_id_1) + assert len(comments_1) > 0, ( + "First review has no comments" + ) + file_comments_1 = [ + c for c in comments_1 + if c.get("subject_type") == "file" or c.get("path") == target_file + ] + assert len(file_comments_1) > 0, ( + "No file-level comment found on first review" + ) + for c in file_comments_1: + comment_ids.append(c["id"]) + + # -- Step 2: Submit second review with another file-level comment + second_body = ( + "TS-GH-41-003 second submission: " + "verifying persistence across reviews" + ) + review_id_2, _ = _submit_file_level_review(target_file, second_body) + review_ids.append(review_id_2) + logger.info("Second review created: %s", review_id_2) + + time.sleep(2) + + # -- Step 3: Verify both reviews' comments are present ---------- + # Check that the first review's comments are still accessible + comments_1_after = _get_review_comments(review_id_1) + assert len(comments_1_after) > 0, ( + "First review comments disappeared after second submission" + ) + + # Check the second review has its own file-level comment + comments_2 = _get_review_comments(review_id_2) + assert len(comments_2) > 0, ( + "Second review has no comments" + ) + for c in comments_2: + comment_ids.append(c["id"]) + + # Verify file-level comments from both reviews are on the PR + all_comments = _get_pr_review_comments() + our_comments = [ + c for c in all_comments + if "TS-GH-41-003" in c.get("body", "") + ] + assert len(our_comments) >= 2, ( + f"Expected at least 2 file-level comments from both reviews, " + f"found {len(our_comments)}" + ) + logger.info( + "Verified %d file-level comments persist across submissions", + len(our_comments), + ) + + finally: + # Cleanup: dismiss reviews and delete comments + for rid in review_ids: + _dismiss_review(rid) + for cid in comment_ids: + _delete_review_comment(cid) + + +class TestGitHubAPIAcceptsFileLevelPayload: + """TS-GH-41-014: GitHub API accepts file-level comment payload. + + Verifies that the GitHub Pull Request Review API accepts a review + payload containing a comment with subject_type: "file" and no line + field. This confirms the API contract that the application relies on. + """ + + def test_github_api_accepts_file_level_comment(self): + """[test_id:TS-GH-41-014] GitHub API accepts file-level comment payload. + + Steps: + 1. Build a review payload with a comment that has subject_type: "file" + and no ``line`` field. + 2. Submit the payload via POST /repos/{owner}/{repo}/pulls/{pr}/reviews. + 3. Assert the API returns successfully (rc 0 from gh, HTTP 200). + 4. Query the review's comments and verify the file-level comment exists. + """ + files = _get_pr_files() + assert len(files) > 0, "Test PR has no changed files" + target_file = files[0] + logger.info("Using target file: %s", target_file) + + review_id = None + comment_ids = [] + + try: + # -- Step 1: Build and submit a file-level comment payload ------ + comment_body = ( + "TS-GH-41-014: Verifying GitHub API accepts " + "subject_type file payload" + ) + payload = { + "event": "COMMENT", + "body": "GH-41 e2e: file-level comment API acceptance test", + "comments": [ + { + "path": target_file, + "body": comment_body, + "subject_type": "file", + } + ], + } + result = _gh_api( + "POST", + f"/repos/{REPO}/pulls/{PR_NUMBER}/reviews", + data=payload, + ) + + # -- Step 2: Assert HTTP 200 (gh returns rc 0 for 200) ---------- + assert result.returncode == 0, ( + f"GitHub API rejected file-level comment payload " + f"(rc={result.returncode}): {result.stderr}" + ) + response = json.loads(result.stdout) + review_id = response.get("id") + assert review_id is not None, "Response missing review id" + logger.info( + "Review %s created successfully with file-level comment", + review_id, + ) + + # Verify response state is as expected + assert response.get("state") is not None, ( + "Response missing 'state' field" + ) + + # -- Step 3: Verify the file-level comment is visible ----------- + time.sleep(2) + comments = _get_review_comments(review_id) + assert len(comments) > 0, ( + "Review has no comments despite successful submission" + ) + + file_level_found = False + for comment in comments: + comment_ids.append(comment["id"]) + # GitHub returns subject_type for file-level comments + if ( + comment.get("subject_type") == "file" + or ( + comment.get("path") == target_file + and comment.get("line") is None + ) + ): + file_level_found = True + logger.info( + "File-level comment confirmed: id=%s path=%s subject_type=%s", + comment["id"], + comment.get("path"), + comment.get("subject_type"), + ) + + assert file_level_found, ( + "No file-level comment found in the review. " + f"Comments returned: {json.dumps(comments, indent=2)[:500]}" + ) + + finally: + # Cleanup: dismiss review and delete comments + if review_id is not None: + _dismiss_review(review_id) + for cid in comment_ids: + _delete_review_comment(cid)