From 2ac7f5539628f20471af9e75941534d406be32a5 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 21:59:21 -0700 Subject: [PATCH] feat: inline review comments on specific lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Findings that reference a file+line within the diff are now posted as inline comments directly on that line, in addition to appearing in the summary table. Findings outside the diff range stay in the body only. Implementation: - gitea/diff.go: ParseDiffNewLines extracts new-file line numbers from each hunk in the unified diff - gitea/client.go: PostReview accepts optional []ReviewComment with path + new_position + body (omitempty when nil) - cmd/review-bot/main.go: maps findings → inline comments when the line exists in the diff, passes them to PostReview Tests: - diff parser: multi-hunk, new files, empty diff, boundary lines - PostReview: with comments, nil comments (omitted from payload) --- cmd/review-bot/main.go | 20 ++++++- gitea/client.go | 20 +++++-- gitea/client_test.go | 4 +- gitea/diff.go | 76 ++++++++++++++++++++++++++ gitea/diff_test.go | 75 +++++++++++++++++++++++++ gitea/post_review_comments_test.go | 88 ++++++++++++++++++++++++++++++ 6 files changed, 274 insertions(+), 9 deletions(-) create mode 100644 gitea/diff.go create mode 100644 gitea/diff_test.go create mode 100644 gitea/post_review_comments_test.go diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index f31cae4..ac1a6d1 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -241,8 +241,24 @@ func main() { sentinel := fmt.Sprintf("", *reviewerName) + // Map findings to inline comments for lines present in the diff + diffRanges := gitea.ParseDiffNewLines(diff) + var inlineComments []gitea.ReviewComment + for _, f := range result.Findings { + if f.File != "" && f.Line > 0 && diffRanges.Contains(f.File, f.Line) { + inlineComments = append(inlineComments, gitea.ReviewComment{ + Path: f.File, + NewPosition: int64(f.Line), + Body: fmt.Sprintf("**[%s]** %s", f.Severity, f.Finding), + }) + } + } + if len(inlineComments) > 0 { + log.Printf("Attaching %d inline comments", len(inlineComments)) + } + log.Printf("Posting review (event=%s)...", event) - posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody) + posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments) if err != nil { log.Fatalf("Failed to post review: %v", err) } @@ -272,7 +288,7 @@ func main() { if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, posted.ID); err != nil { log.Printf("Warning: could not delete review for escalation: %v", err) } else { - _, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "REQUEST_CHANGES", reviewBody) + _, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "REQUEST_CHANGES", reviewBody, inlineComments) if err != nil { log.Printf("Warning: could not re-post as REQUEST_CHANGES: %v", err) } else { diff --git a/gitea/client.go b/gitea/client.go index 1b4a472..4185f18 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -57,6 +57,13 @@ type ChangedFile struct { Status string `json:"status"` } +// ReviewComment represents an inline comment to attach to a review. +type ReviewComment struct { + Path string `json:"path"` + NewPosition int64 `json:"new_position"` + Body string `json:"body"` +} + // GetPullRequest fetches PR metadata. func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) { reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d", c.baseURL, owner, repo, number) @@ -131,15 +138,18 @@ func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, r // PostReview submits a review to a PR and returns the created review. // event should be "APPROVED" or "REQUEST_CHANGES". -func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string) (*Review, error) { +// comments are optional inline comments attached to specific lines. +func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string, comments []ReviewComment) (*Review, error) { reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, owner, repo, number) payload := struct { - Body string `json:"body"` - Event string `json:"event"` + Body string `json:"body"` + Event string `json:"event"` + Comments []ReviewComment `json:"comments,omitempty"` }{ - Body: body, - Event: event, + Body: body, + Event: event, + Comments: comments, } data, err := json.Marshal(payload) diff --git a/gitea/client_test.go b/gitea/client_test.go index eabdd9e..415e83b 100644 --- a/gitea/client_test.go +++ b/gitea/client_test.go @@ -128,7 +128,7 @@ func TestPostReview(t *testing.T) { defer server.Close() client := NewClient(server.URL, "test-token") - review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM") + review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", nil) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -175,7 +175,7 @@ func TestPostReview_Non200(t *testing.T) { defer server.Close() client := NewClient(server.URL, "test-token") - _, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test") + _, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", nil) if err == nil { t.Fatal("expected error for 403, got nil") } diff --git a/gitea/diff.go b/gitea/diff.go new file mode 100644 index 0000000..e45ef47 --- /dev/null +++ b/gitea/diff.go @@ -0,0 +1,76 @@ +package gitea + +import ( + "strconv" + "strings" +) + +// DiffLineRanges maps filenames to the set of new-file line numbers present in the diff. +type DiffLineRanges struct { + files map[string]map[int]bool +} + +// Contains reports whether the given file+line is within the diff hunks. +func (d *DiffLineRanges) Contains(file string, line int) bool { + if d == nil || d.files == nil { + return false + } + lines, ok := d.files[file] + if !ok { + return false + } + return lines[line] +} + +// ParseDiffNewLines parses a unified diff and extracts the new-file line numbers +// that appear in each hunk (both added and context lines). +func ParseDiffNewLines(diff string) *DiffLineRanges { + result := &DiffLineRanges{files: make(map[string]map[int]bool)} + + var currentFile string + var newLine int + + for _, line := range strings.Split(diff, "\n") { + // Track current file from +++ header + if strings.HasPrefix(line, "+++ b/") { + currentFile = strings.TrimPrefix(line, "+++ b/") + if result.files[currentFile] == nil { + result.files[currentFile] = make(map[int]bool) + } + continue + } + if strings.HasPrefix(line, "+++ /dev/null") { + currentFile = "" + continue + } + + // Parse hunk header: @@ -old,count +new,count @@ + if strings.HasPrefix(line, "@@") && currentFile != "" { + // Extract the +N part + parts := strings.Split(line, "+") + if len(parts) >= 2 { + numStr := strings.Split(parts[1], ",")[0] + n, err := strconv.Atoi(numStr) + if err == nil { + newLine = n + } + } + continue + } + + if currentFile == "" { + continue + } + + // Count lines in hunk + if strings.HasPrefix(line, "+") || strings.HasPrefix(line, " ") { + result.files[currentFile][newLine] = true + newLine++ + } else if strings.HasPrefix(line, "-") { + // Removed lines don't advance new line counter + continue + } + } + + return result +} diff --git a/gitea/diff_test.go b/gitea/diff_test.go new file mode 100644 index 0000000..a017aff --- /dev/null +++ b/gitea/diff_test.go @@ -0,0 +1,75 @@ +package gitea + +import ( + "testing" +) + +func TestParseDiffLineRanges(t *testing.T) { + diff := `diff --git a/main.go b/main.go +index abc1234..def5678 100644 +--- a/main.go ++++ b/main.go +@@ -10,6 +10,8 @@ func main() { + fmt.Println("hello") ++ fmt.Println("new line 11") ++ fmt.Println("new line 12") + fmt.Println("existing") + } +@@ -30,4 +32,5 @@ func other() { + return nil ++ // added at line 33 + } +diff --git a/util.go b/util.go +new file mode 100644 +--- /dev/null ++++ b/util.go +@@ -0,0 +1,5 @@ ++package main ++ ++func helper() string { ++ return "hi" ++} +` + + ranges := ParseDiffNewLines(diff) + + // main.go should have lines 10-17 (first hunk) and 32-36 (second hunk) + if !ranges.Contains("main.go", 11) { + t.Error("expected main.go:11 to be in diff") + } + if !ranges.Contains("main.go", 12) { + t.Error("expected main.go:12 to be in diff") + } + if !ranges.Contains("main.go", 10) { + t.Error("expected main.go:10 to be in diff (context line)") + } + if !ranges.Contains("main.go", 33) { + t.Error("expected main.go:33 to be in diff") + } + if ranges.Contains("main.go", 25) { + t.Error("main.go:25 should NOT be in diff") + } + + // util.go is entirely new, lines 1-5 + if !ranges.Contains("util.go", 1) { + t.Error("expected util.go:1 to be in diff") + } + if !ranges.Contains("util.go", 5) { + t.Error("expected util.go:5 to be in diff") + } + if ranges.Contains("util.go", 6) { + t.Error("util.go:6 should NOT be in diff") + } + + // Unknown file + if ranges.Contains("unknown.go", 1) { + t.Error("unknown.go should not be in diff") + } +} + +func TestParseDiffNewLines_Empty(t *testing.T) { + ranges := ParseDiffNewLines("") + if ranges.Contains("any.go", 1) { + t.Error("empty diff should contain nothing") + } +} diff --git a/gitea/post_review_comments_test.go b/gitea/post_review_comments_test.go new file mode 100644 index 0000000..e58d05d --- /dev/null +++ b/gitea/post_review_comments_test.go @@ -0,0 +1,88 @@ +package gitea + +import ( + "context" + "encoding/json" + "net/http" + "net/http/httptest" + "testing" +) + +func TestPostReview_WithComments(t *testing.T) { + var gotPayload struct { + Body string `json:"body"` + Event string `json:"event"` + Comments []struct { + Path string `json:"path"` + NewPosition int64 `json:"new_position"` + Body string `json:"body"` + } `json:"comments"` + } + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + json.NewDecoder(r.Body).Decode(&gotPayload) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(200) + json.NewEncoder(w).Encode(map[string]any{ + "id": 99, + "body": gotPayload.Body, + "user": map[string]any{"login": "bot"}, + }) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + comments := []ReviewComment{ + {Path: "main.go", NewPosition: 42, Body: "[MAJOR] Something bad"}, + {Path: "util.go", NewPosition: 10, Body: "[MINOR] Style issue"}, + } + + _, err := client.PostReview(context.Background(), "owner", "repo", 1, "REQUEST_CHANGES", "summary", comments) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + if len(gotPayload.Comments) != 2 { + t.Fatalf("expected 2 comments, got %d", len(gotPayload.Comments)) + } + if gotPayload.Comments[0].Path != "main.go" { + t.Errorf("expected path main.go, got %s", gotPayload.Comments[0].Path) + } + if gotPayload.Comments[0].NewPosition != 42 { + t.Errorf("expected new_position 42, got %d", gotPayload.Comments[0].NewPosition) + } + if gotPayload.Comments[1].Body != "[MINOR] Style issue" { + t.Errorf("unexpected body: %s", gotPayload.Comments[1].Body) + } +} + +func TestPostReview_NilComments(t *testing.T) { + var gotPayload map[string]any + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + json.NewDecoder(r.Body).Decode(&gotPayload) + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(200) + json.NewEncoder(w).Encode(map[string]any{ + "id": 100, + "body": "test", + "user": map[string]any{"login": "bot"}, + }) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + _, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "all good", nil) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // With nil comments, the field should be omitted (omitempty) + comments, ok := gotPayload["comments"] + if ok && comments != nil { + arr, isArr := comments.([]any) + if isArr && len(arr) > 0 { + t.Error("expected no comments in payload when nil passed") + } + } +}