feat: inline review comments on specific lines #26
+18
-2
@@ -241,8 +241,24 @@ func main() {
|
||||
|
||||
sentinel := fmt.Sprintf("<!-- review-bot:%s -->", *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 {
|
||||
|
||||
+15
-5
@@ -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)
|
||||
|
||||
@@ -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")
|
||||
}
|
||||
|
||||
@@ -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
|
||||
}
|
||||
|
||||
|
sonnet-review-bot
commented
[NIT] Hunk header parsing uses **[NIT]** Hunk header parsing uses `strings.Split(line, "+")` and then trims at comma/space. While adequate for typical diffs, consider a more targeted parse (e.g., locating the first '+' between the '@@' markers) to be more robust against unusual hunk headers.
|
||||
// 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
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
@@ -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")
|
||||
}
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user
[MINOR] PostReview constructs the reviews API URL using owner and repo without url.PathEscape, which can lead to malformed paths if these contain unexpected characters or slashes. While inputs are typically controlled, escaping path segments (as done elsewhere in this client) is a defense-in-depth improvement.