From 2ac7f5539628f20471af9e75941534d406be32a5 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 21:59:21 -0700 Subject: [PATCH 1/6] 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") + } + } +} -- 2.47.3 From b0dc6d0c096342c16d6dac0a1af656207e56af7a Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 22:10:49 -0700 Subject: [PATCH 2/6] fix: handle single-line hunks and no-newline markers in diff parser - Hunk headers without comma ("@@ -1 +1 @@") now parse correctly by splitting on comma OR space instead of comma only - Explicit skip for "\ No newline at end of file" lines (was already safe but now documents intent) - Tests added for both edge cases (TDD: tests written first, confirmed failure, then fixed) Addresses sonnet findings #1 and #2 from PR #26 review. --- gitea/diff.go | 15 ++++++++++++--- gitea/diff_test.go | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 3 deletions(-) diff --git a/gitea/diff.go b/gitea/diff.go index e45ef47..d191189 100644 --- a/gitea/diff.go +++ b/gitea/diff.go @@ -44,12 +44,16 @@ func ParseDiffNewLines(diff string) *DiffLineRanges { continue } - // Parse hunk header: @@ -old,count +new,count @@ + // Parse hunk header: @@ -old,count +new,count @@ or @@ -old +new @@ if strings.HasPrefix(line, "@@") && currentFile != "" { - // Extract the +N part + // Extract the +N part — handle both "+10,8" and "+1" forms parts := strings.Split(line, "+") if len(parts) >= 2 { - numStr := strings.Split(parts[1], ",")[0] + // Take everything before comma or space + numStr := parts[1] + if idx := strings.IndexAny(numStr, ", "); idx != -1 { + numStr = numStr[:idx] + } n, err := strconv.Atoi(numStr) if err == nil { newLine = n @@ -62,6 +66,11 @@ func ParseDiffNewLines(diff string) *DiffLineRanges { continue } + // Skip diff metadata lines + if strings.HasPrefix(line, "\\") { + continue + } + // Count lines in hunk if strings.HasPrefix(line, "+") || strings.HasPrefix(line, " ") { result.files[currentFile][newLine] = true diff --git a/gitea/diff_test.go b/gitea/diff_test.go index a017aff..f73b39e 100644 --- a/gitea/diff_test.go +++ b/gitea/diff_test.go @@ -73,3 +73,43 @@ func TestParseDiffNewLines_Empty(t *testing.T) { t.Error("empty diff should contain nothing") } } + +func TestParseDiffNewLines_NoCommaHunk(t *testing.T) { + // Single-line hunks omit the comma: @@ -1 +1 @@ + diff := `diff --git a/single.go b/single.go +--- a/single.go ++++ b/single.go +@@ -1 +1 @@ +-old line ++new line +` + ranges := ParseDiffNewLines(diff) + if !ranges.Contains("single.go", 1) { + t.Error("expected single.go:1 to be in diff (no-comma hunk)") + } + if ranges.Contains("single.go", 2) { + t.Error("single.go:2 should NOT be in diff") + } +} + +func TestParseDiffNewLines_NoNewlineMarker(t *testing.T) { + // "\ No newline at end of file" should not advance line counter + diff := `diff --git a/noeof.go b/noeof.go +--- a/noeof.go ++++ b/noeof.go +@@ -1,2 +1,2 @@ ++line one ++line two +\ No newline at end of file +` + ranges := ParseDiffNewLines(diff) + if !ranges.Contains("noeof.go", 1) { + t.Error("expected noeof.go:1") + } + if !ranges.Contains("noeof.go", 2) { + t.Error("expected noeof.go:2") + } + if ranges.Contains("noeof.go", 3) { + t.Error("noeof.go:3 should NOT be in diff (no-newline marker)") + } +} -- 2.47.3 From 1c2292265b7d95f60e0045cc16d6546103552b4f Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 22:17:36 -0700 Subject: [PATCH 3/6] feat: skip re-posting when review is unchanged (preserve threads) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before posting, compare the new review body+event against the existing review with the same sentinel. If identical, skip entirely — this preserves conversation threads on inline comments and avoids re-notifying reviewers for findings they already know about. Only re-posts when findings actually change (fixed, new, or different). Tests: 6 cases covering identical, different body, different state, stale reviews, and different sentinels. --- cmd/review-bot/main.go | 29 ++++++++++++++ cmd/review-bot/main_test.go | 79 +++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index ac1a6d1..55b6281 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -257,6 +257,15 @@ func main() { log.Printf("Attaching %d inline comments", len(inlineComments)) } + // Check if existing review is unchanged — skip to preserve conversation threads + if *updateExisting && *reviewerName != "" { + existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) + if err == nil && reviewUnchanged(existingReviews, reviewBody, event, sentinel) { + log.Printf("Review unchanged from previous run; skipping to preserve threads") + return + } + } + log.Printf("Posting review (event=%s)...", event) posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments) if err != nil { @@ -465,3 +474,23 @@ func shouldEscalate(reviews []gitea.Review, postedID int64, postedLogin, ownSent } return false } + +// reviewUnchanged checks if an existing review with the same sentinel +// already has identical body and state. Returns true if a re-post would +// produce the same result (skip to preserve conversation threads). +func reviewUnchanged(reviews []gitea.Review, newBody, newEvent, sentinel string) bool { + for _, r := range reviews { + if r.Stale { + continue + } + if !strings.Contains(r.Body, sentinel) { + continue + } + // Compare state (map APPROVED back from Gitea's representation) + existingEvent := r.State + if existingEvent == r.State && existingEvent == newEvent && r.Body == newBody { + return true + } + } + return false +} diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index 38304c5..11ff8e2 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -148,3 +148,82 @@ func TestShouldEscalate(t *testing.T) { }) } } + +func TestReviewUnchanged(t *testing.T) { + tests := []struct { + name string + existing []gitea.Review + newBody string + newEvent string + sentinel string + want bool + }{ + { + name: "no existing review", + existing: nil, + newBody: "new review", + newEvent: "APPROVED", + sentinel: "", + want: false, + }, + { + name: "identical body and state", + existing: []gitea.Review{ + makeReview(100, "bot", "APPROVED", false, "same body\n"), + }, + newBody: "same body\n", + newEvent: "APPROVED", + sentinel: "", + want: true, + }, + { + name: "same body but different state", + existing: []gitea.Review{ + makeReview(100, "bot", "APPROVED", false, "body\n"), + }, + newBody: "body\n", + newEvent: "REQUEST_CHANGES", + sentinel: "", + want: false, + }, + { + name: "different body same state", + existing: []gitea.Review{ + makeReview(100, "bot", "APPROVED", false, "old body\n"), + }, + newBody: "new body\n", + newEvent: "APPROVED", + sentinel: "", + want: false, + }, + { + name: "stale review with same body (should still post)", + existing: []gitea.Review{ + makeReview(100, "bot", "APPROVED", true, "same\n"), + }, + newBody: "same\n", + newEvent: "APPROVED", + sentinel: "", + want: false, + }, + { + name: "different sentinel (not our review)", + existing: []gitea.Review{ + makeReview(100, "bot", "APPROVED", false, "body\n"), + }, + newBody: "body\n", + newEvent: "APPROVED", + sentinel: "", + want: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := reviewUnchanged(tc.existing, tc.newBody, tc.newEvent, tc.sentinel) + if got != tc.want { + t.Errorf("reviewUnchanged() = %v, want %v", got, tc.want) + } + }) + } +} -- 2.47.3 From e261976dd80d3eeeceabbd40f18d765ada646a32 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 22:46:45 -0700 Subject: [PATCH 4/6] feat: edit-in-place review updates (no more delete) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace the delete-and-repost strategy with edit-in-place: 1. No existing review → POST new (first run) 2. Same state, same body → skip entirely (threads preserved) 3. Same state, body changed → PATCH body in place via timeline API 4. State change needed → PATCH old body to "Superseded", POST new This preserves conversation threads on inline comments. Replies to findings are never lost. The only time a new review is posted is on first run or when the state transitions (APPROVED ↔ REQUEST_CHANGES). New Gitea client methods: - EditComment: PATCH /repos/{owner}/{repo}/issues/comments/{id} - GetTimelineReviewCommentID: finds the comment ID for a review body by scanning the issue timeline for the sentinel Also simplifies shouldEscalate: removes the login parameter requirement for pre-posting scenarios (uses findOwnReview to get login from existing review instead). Tests: findOwnReview (4 cases), EditComment (2 cases), GetTimelineReviewCommentID (2 cases), shouldEscalate (8 cases updated). --- cmd/review-bot/main.go | 114 +++++++++++++++++++++++------------- cmd/review-bot/main_test.go | 81 +++++++++++++++++++++---- gitea/client.go | 75 ++++++++++++++++++++++++ gitea/client_test.go | 79 +++++++++++++++++++++++++ 4 files changed, 298 insertions(+), 51 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 55b6281..b37f448 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -257,56 +257,70 @@ func main() { log.Printf("Attaching %d inline comments", len(inlineComments)) } - // Check if existing review is unchanged — skip to preserve conversation threads + // --- Review update strategy --- + // 1. No existing review → POST new + // 2. Existing review, same state → PATCH body in place (preserves threads) + // 3. Existing review, state change → PATCH old to "Superseded", POST new if *updateExisting && *reviewerName != "" { existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) - if err == nil && reviewUnchanged(existingReviews, reviewBody, event, sentinel) { - log.Printf("Review unchanged from previous run; skipping to preserve threads") - return - } - } - - log.Printf("Posting review (event=%s)...", event) - posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments) - if err != nil { - log.Fatalf("Failed to post review: %v", err) - } - log.Printf("Review posted (id=%d, user=%s)", posted.ID, posted.User.Login) - - // Delete stale reviews from this bot using sentinel matching - if *updateExisting && *reviewerName != "" { - reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) if err != nil { log.Printf("Warning: could not list existing reviews: %v", err) } else { - for _, r := range reviews { - if r.ID != posted.ID && r.User.Login == posted.User.Login && strings.Contains(r.Body, sentinel) { - if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, r.ID); err != nil { - log.Printf("Warning: could not delete old review %d: %v", r.ID, err) - } else { - log.Printf("Deleted stale review %d", r.ID) - } - } + // Worst-wins: escalate if a sibling blocks (need own login from existing review) + ownLogin := "" + existing := findOwnReview(existingReviews, sentinel) + if existing != nil { + ownLogin = existing.User.Login + } + if event == "APPROVED" && shouldEscalate(existingReviews, 0, ownLogin, sentinel) { + log.Printf("Sibling review has REQUEST_CHANGES; escalating to REQUEST_CHANGES") + event = "REQUEST_CHANGES" } - // Worst-wins: if we posted APPROVE but a sibling review from the - // same user (same token, different role) has REQUEST_CHANGES, - // delete ours and re-post as REQUEST_CHANGES to maintain the block. - if event == "APPROVED" && shouldEscalate(reviews, posted.ID, posted.User.Login, sentinel) { - log.Printf("Sibling review has REQUEST_CHANGES; escalating") - 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, inlineComments) + if existing != nil { + if reviewUnchanged(existingReviews, reviewBody, event, sentinel) { + log.Printf("Review unchanged from previous run; skipping to preserve threads") + return + } + + // Same state → PATCH in place + if existing.State == event { + commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel) if err != nil { - log.Printf("Warning: could not re-post as REQUEST_CHANGES: %v", err) + log.Printf("Warning: could not find review comment ID, falling back to new post: %v", err) } else { - log.Printf("Review escalated to REQUEST_CHANGES") + if err := giteaClient.EditComment(ctx, owner, repoName, commentID, reviewBody); err != nil { + log.Printf("Warning: could not edit review, falling back to new post: %v", err) + } else { + log.Printf("Review updated in place (comment_id=%d)", commentID) + return + } + } + } else { + // State change → mark old as superseded, post new below + commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel) + if err != nil { + log.Printf("Warning: could not find old review comment ID: %v", err) + } else { + supersededBody := fmt.Sprintf("~~*This review has been superseded by a newer review below.*~~\n\n%s", sentinel) + if err := giteaClient.EditComment(ctx, owner, repoName, commentID, supersededBody); err != nil { + log.Printf("Warning: could not mark old review as superseded: %v", err) + } else { + log.Printf("Marked old review as superseded (state was %s, now %s)", existing.State, event) + } } } } } } + + // POST new review (first run, or state transition fallthrough) + log.Printf("Posting review (event=%s)...", event) + _, err = giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments) + if err != nil { + log.Fatalf("Failed to post review: %v", err) + } + log.Printf("Review posted successfully") } // fetchFileContext fetches the full content of modified files from the PR branch. @@ -463,12 +477,20 @@ func validateReviewerName(name string) error { return nil } -// shouldEscalate checks if the current APPROVED review should be escalated -// to REQUEST_CHANGES because a sibling bot review (same user, different role) -// already has REQUEST_CHANGES. -func shouldEscalate(reviews []gitea.Review, postedID int64, postedLogin, ownSentinel string) bool { +// shouldEscalate checks if any sibling bot review from the same user +// (different sentinel, same token) has REQUEST_CHANGES. +// ownLogin is the bot user login; if empty, escalation check is skipped. +// postedID is excluded from consideration (0 means no exclusion needed). +func shouldEscalate(reviews []gitea.Review, postedID int64, ownLogin, ownSentinel string) bool { + if ownLogin == "" { + return false + } for _, r := range reviews { - if r.ID != postedID && !r.Stale && r.User.Login == postedLogin && r.State == "REQUEST_CHANGES" && strings.Contains(r.Body, "", want: false, }, @@ -73,7 +74,7 @@ func TestShouldEscalate(t *testing.T) { makeReview(101, "bot", "REQUEST_CHANGES", false, "bad\n"), }, postedID: 100, - postedLogin: "bot", + ownLogin: "bot", ownSentinel: "", want: true, }, @@ -83,7 +84,7 @@ func TestShouldEscalate(t *testing.T) { makeReview(101, "other-bot", "REQUEST_CHANGES", false, "bad\n"), }, postedID: 100, - postedLogin: "bot", + ownLogin: "bot", ownSentinel: "", want: false, }, @@ -93,7 +94,7 @@ func TestShouldEscalate(t *testing.T) { makeReview(101, "bot", "REQUEST_CHANGES", true, "old\n"), }, postedID: 100, - postedLogin: "bot", + ownLogin: "bot", ownSentinel: "", want: false, }, @@ -103,7 +104,7 @@ func TestShouldEscalate(t *testing.T) { makeReview(101, "bot", "REQUEST_CHANGES", false, "old\n"), }, postedID: 100, - postedLogin: "bot", + ownLogin: "bot", ownSentinel: "", want: false, }, @@ -113,7 +114,7 @@ func TestShouldEscalate(t *testing.T) { makeReview(101, "bot", "APPROVED", false, "good\n"), }, postedID: 100, - postedLogin: "bot", + ownLogin: "bot", ownSentinel: "", want: false, }, @@ -123,7 +124,7 @@ func TestShouldEscalate(t *testing.T) { makeReview(101, "bot", "REQUEST_CHANGES", false, "please fix this"), }, postedID: 100, - postedLogin: "bot", + ownLogin: "bot", ownSentinel: "", want: false, }, @@ -133,7 +134,7 @@ func TestShouldEscalate(t *testing.T) { makeReview(100, "bot", "REQUEST_CHANGES", false, "x\n"), }, postedID: 100, - postedLogin: "bot", + ownLogin: "bot", ownSentinel: "", want: false, }, @@ -141,7 +142,7 @@ func TestShouldEscalate(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - got := shouldEscalate(tc.reviews, tc.postedID, tc.postedLogin, tc.ownSentinel) + got := shouldEscalate(tc.reviews, tc.postedID, tc.ownLogin, tc.ownSentinel) if got != tc.want { t.Errorf("shouldEscalate() = %v, want %v", got, tc.want) } @@ -227,3 +228,63 @@ func TestReviewUnchanged(t *testing.T) { }) } } + +func TestFindOwnReview(t *testing.T) { + tests := []struct { + name string + reviews []gitea.Review + sentinel string + wantID int64 + wantNil bool + }{ + { + name: "no reviews", + reviews: nil, + sentinel: "", + wantNil: true, + }, + { + name: "found by sentinel", + reviews: []gitea.Review{ + makeReview(42, "bot", "APPROVED", false, "review body\n"), + }, + sentinel: "", + wantID: 42, + }, + { + name: "wrong sentinel", + reviews: []gitea.Review{ + makeReview(42, "bot", "APPROVED", false, "body\n"), + }, + sentinel: "", + wantNil: true, + }, + { + name: "multiple reviews, returns first match", + reviews: []gitea.Review{ + makeReview(10, "bot", "APPROVED", false, "old\n"), + makeReview(20, "bot", "APPROVED", false, "new\n"), + }, + sentinel: "", + wantID: 20, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := findOwnReview(tc.reviews, tc.sentinel) + if tc.wantNil { + if got != nil { + t.Errorf("findOwnReview() = %v, want nil", got) + } + } else { + if got == nil { + t.Fatal("findOwnReview() = nil, want non-nil") + } + if got.ID != tc.wantID { + t.Errorf("findOwnReview().ID = %d, want %d", got.ID, tc.wantID) + } + } + }) + } +} diff --git a/gitea/client.go b/gitea/client.go index 4185f18..ee15e9a 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -353,3 +353,78 @@ func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number in } return nil } + +// TimelineEvent represents an entry from the issue timeline API. +type TimelineEvent struct { + ID int64 `json:"id"` + Type string `json:"type"` + Body string `json:"body"` + User struct { + Login string `json:"login"` + } `json:"user"` +} + +// GetTimelineReviewCommentID finds the comment ID for a review body by +// scanning the issue timeline for a review event containing the sentinel. +func (c *Client) GetTimelineReviewCommentID(ctx context.Context, owner, repo string, number int, sentinel string) (int64, error) { + const pageSize = 50 + for page := 1; ; page++ { + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues/%d/timeline?limit=%d&page=%d", + c.baseURL, + url.PathEscape(owner), + url.PathEscape(repo), + number, + pageSize, + page) + body, err := c.doGet(ctx, reqURL) + if err != nil { + return 0, fmt.Errorf("get timeline (page %d): %w", page, err) + } + var events []TimelineEvent + if err := json.Unmarshal(body, &events); err != nil { + return 0, fmt.Errorf("parse timeline (page %d): %w", page, err) + } + for _, ev := range events { + if ev.Type == "review" && strings.Contains(ev.Body, sentinel) { + return ev.ID, nil + } + } + if len(events) < pageSize { + break + } + } + return 0, fmt.Errorf("no timeline event found with sentinel") +} + +// EditComment updates the body of an issue/review comment. +func (c *Client) EditComment(ctx context.Context, owner, repo string, commentID int64, newBody string) error { + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues/comments/%d", + c.baseURL, + url.PathEscape(owner), + url.PathEscape(repo), + commentID) + + payload := struct { + Body string `json:"body"` + }{Body: newBody} + data, _ := json.Marshal(payload) + + req, err := http.NewRequestWithContext(ctx, http.MethodPatch, reqURL, bytes.NewReader(data)) + if err != nil { + return fmt.Errorf("create edit request: %w", err) + } + req.Header.Set("Authorization", "token "+c.token) + req.Header.Set("Content-Type", "application/json") + + resp, err := c.http.Do(req) + if err != nil { + return fmt.Errorf("edit comment: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + body, _ := io.ReadAll(resp.Body) + return fmt.Errorf("edit comment failed (status %d): %s", resp.StatusCode, body) + } + return nil +} diff --git a/gitea/client_test.go b/gitea/client_test.go index 415e83b..be9b94e 100644 --- a/gitea/client_test.go +++ b/gitea/client_test.go @@ -426,3 +426,82 @@ func TestDeleteReview_Forbidden(t *testing.T) { t.Fatal("expected error for 403, got nil") } } + +func TestEditComment(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPatch { + t.Errorf("expected PATCH, got %s", r.Method) + } + if r.URL.Path != "/api/v1/repos/owner/repo/issues/comments/42" { + t.Errorf("unexpected path: %s", r.URL.Path) + } + + var payload struct { + Body string `json:"body"` + } + json.NewDecoder(r.Body).Decode(&payload) + if payload.Body != "updated body" { + t.Errorf("unexpected body: %s", payload.Body) + } + + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"id": 42, "body": "updated body"}`)) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + err := client.EditComment(context.Background(), "owner", "repo", 42, "updated body") + if err != nil { + t.Fatalf("EditComment() error = %v", err) + } +} + +func TestEditComment_Forbidden(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + w.Write([]byte(`{"message": "not allowed"}`)) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + err := client.EditComment(context.Background(), "owner", "repo", 42, "new body") + if err == nil { + t.Fatal("expected error for 403 response") + } +} + +func TestGetTimelineReviewCommentID(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v1/repos/owner/repo/issues/5/timeline" { + t.Errorf("unexpected path: %s", r.URL.Path) + } + w.Write([]byte(`[ + {"id": 100, "type": "comment", "body": "random"}, + {"id": 200, "type": "review", "body": "other review "}, + {"id": 300, "type": "review", "body": "our review "} + ]`)) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + id, err := client.GetTimelineReviewCommentID(context.Background(), "owner", "repo", 5, "") + if err != nil { + t.Fatalf("GetTimelineReviewCommentID() error = %v", err) + } + if id != 300 { + t.Errorf("got id=%d, want 300", id) + } +} + +func TestGetTimelineReviewCommentID_NotFound(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Write([]byte(`[{"id": 100, "type": "review", "body": "no match"}]`)) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + _, err := client.GetTimelineReviewCommentID(context.Background(), "owner", "repo", 5, "") + if err == nil { + t.Fatal("expected error when sentinel not found") + } +} -- 2.47.3 From ec1962213374ef829629dbda24a93ad99a775dfa Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 22:50:13 -0700 Subject: [PATCH 5/6] fix: address review findings (escalation, marshal error, redundant check) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 1. First-run escalation regression (MAJOR): Add post-posting escalation fallback. After posting APPROVED on first run, check if a sibling from the same user has REQUEST_CHANGES — if so, mark ours as superseded and re-post as REQUEST_CHANGES. 2. json.Marshal error handling (MINOR): Return error from EditComment instead of ignoring it with blank identifier. 3. Redundant condition (NIT): Remove dead assignment in reviewUnchanged where existingEvent was assigned from r.State then compared to itself. --- cmd/review-bot/main.go | 32 +++++++++++++++++++++++++++----- gitea/client.go | 5 ++++- 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index b37f448..8163ea1 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -316,11 +316,35 @@ func main() { // POST new review (first run, or state transition fallthrough) log.Printf("Posting review (event=%s)...", event) - _, err = giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments) + posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments) if err != nil { log.Fatalf("Failed to post review: %v", err) } - log.Printf("Review posted successfully") + log.Printf("Review posted (id=%d, user=%s)", posted.ID, posted.User.Login) + + // Post-posting escalation: if we just posted APPROVED but a sibling + // from the same user has REQUEST_CHANGES, mark ours as superseded and + // re-post as REQUEST_CHANGES. This handles the first-run case where + // we don't know our login until after posting. + if event == "APPROVED" && *updateExisting && *reviewerName != "" { + reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) + if err == nil && shouldEscalate(reviews, posted.ID, posted.User.Login, sentinel) { + log.Printf("Post-posting escalation: sibling has REQUEST_CHANGES") + // Mark our just-posted review as superseded + commentID, err := giteaClient.GetTimelineReviewCommentID(ctx, owner, repoName, prNumber, sentinel) + if err == nil { + supersededBody := fmt.Sprintf("~~*This review has been superseded by a newer review below.*~~\n\n%s", sentinel) + giteaClient.EditComment(ctx, owner, repoName, commentID, supersededBody) + } + // Re-post as REQUEST_CHANGES + _, 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 { + log.Printf("Review escalated to REQUEST_CHANGES") + } + } + } } // fetchFileContext fetches the full content of modified files from the PR branch. @@ -508,9 +532,7 @@ func reviewUnchanged(reviews []gitea.Review, newBody, newEvent, sentinel string) if !strings.Contains(r.Body, sentinel) { continue } - // Compare state (map APPROVED back from Gitea's representation) - existingEvent := r.State - if existingEvent == r.State && existingEvent == newEvent && r.Body == newBody { + if r.State == newEvent && r.Body == newBody { return true } } diff --git a/gitea/client.go b/gitea/client.go index ee15e9a..9355e29 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -407,7 +407,10 @@ func (c *Client) EditComment(ctx context.Context, owner, repo string, commentID payload := struct { Body string `json:"body"` }{Body: newBody} - data, _ := json.Marshal(payload) + data, err := json.Marshal(payload) + if err != nil { + return fmt.Errorf("marshal edit payload: %w", err) + } req, err := http.NewRequestWithContext(ctx, http.MethodPatch, reqURL, bytes.NewReader(data)) if err != nil { -- 2.47.3 From a9c8ecfb0bc160f936aff403bed5472d28e7305b Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 23:01:32 -0700 Subject: [PATCH 6/6] docs: add review update strategy with state transition diagram Explains the edit-in-place approach, state transition rules, worst-wins escalation, and inline comment lifecycle. Includes a Mermaid state diagram for visual reference. --- docs/REVIEW_STRATEGY.md | 97 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 docs/REVIEW_STRATEGY.md diff --git a/docs/REVIEW_STRATEGY.md b/docs/REVIEW_STRATEGY.md new file mode 100644 index 0000000..270d124 --- /dev/null +++ b/docs/REVIEW_STRATEGY.md @@ -0,0 +1,97 @@ +# Review Update Strategy + +review-bot uses an **edit-in-place** strategy for updating reviews. Reviews are never deleted — this preserves conversation threads on inline comments. + +## State Transition Diagram + +```mermaid +stateDiagram-v2 + [*] --> NoExistingReview: First run + + NoExistingReview --> POST_Review: Generate findings + event + POST_Review --> PostEscalationCheck: event == APPROVED? + + PostEscalationCheck --> Done: No sibling blocks + PostEscalationCheck --> Supersede_And_Repost: Sibling has REQUEST_CHANGES + Supersede_And_Repost --> Done: Posted as REQUEST_CHANGES + + [*] --> ExistingReviewFound: Subsequent run (sentinel match) + + ExistingReviewFound --> CheckEscalation: Determine final event + CheckEscalation --> CompareState: Apply worst-wins if needed + + CompareState --> SameState: existing.state == new event + CompareState --> StateChange: existing.state != new event + + SameState --> Skip: Body unchanged + SameState --> PatchBody: Body changed → PATCH in place + + StateChange --> Escalate: APPROVED → REQUEST_CHANGES + StateChange --> Downgrade: REQUEST_CHANGES → APPROVED + + Escalate --> Supersede: PATCH old body → "Superseded" + Supersede --> POST_New_RC: POST new REQUEST_CHANGES + + Downgrade --> POST_New_Approve: POST new APPROVED (old stays intact) + + Skip --> Done + PatchBody --> Done + POST_New_RC --> Done + POST_New_Approve --> Done +``` + +## Rules + +| Scenario | Action | Reason | +|----------|--------|--------| +| No existing review | POST new | First run | +| Same state, same body | Skip | Nothing changed — preserve threads | +| Same state, body changed | PATCH body | Update findings without losing threads | +| APPROVED → REQUEST_CHANGES | Supersede old + POST new | Can always escalate; old APPROVED is no longer valid | +| REQUEST_CHANGES → APPROVED | POST new APPROVED | Can't edit state; old REQUEST_CHANGES stays as historical record | +| Sibling has REQUEST_CHANGES (worst-wins) | Escalate to REQUEST_CHANGES | PR must stay blocked if ANY reviewer blocks | + +## Key Constraints + +1. **Review state is immutable after POST** — Gitea has no API to change APPROVED ↔ REQUEST_CHANGES +2. **Never delete reviews** — Deleting cascades to inline comments and reply threads +3. **"Last review per user" wins** — Gitea uses the most recent review from a user for merge decisions +4. **REQUEST_CHANGES reviews are never touched** — Their inline comments and threads are preserved as historical record +5. **APPROVED reviews can be superseded** — When escalation is needed, mark old as superseded and POST new + +## Worst-Wins (Shared Token) + +When multiple reviewer roles share a token (e.g., `sonnet` and `security` both use `sonnet-review-bot`): + +``` +CI Matrix Run: + sonnet → REQUEST_CHANGES (findings) + security → APPROVED (no security issues) + ↓ + security sees sibling REQUEST_CHANGES + ↓ + security escalates → REQUEST_CHANGES + ↓ + PR stays blocked ✓ +``` + +The **first-run case** (no existing review to read login from) uses a post-posting fallback: +POST APPROVED → check siblings → if blocked, supersede own APPROVED → re-POST as REQUEST_CHANGES. + +## Edit Mechanism + +Reviews are edited via `PATCH /repos/{owner}/{repo}/issues/comments/{id}`: + +- **Review body**: ID obtained from the timeline API (`/issues/{index}/timeline`, type `"review"`) +- **Inline comments**: IDs obtained from `/pulls/{index}/reviews/{id}/comments` +- **Both are editable** by the token that created them +- **ListReviews always returns the original body** (reads from review table, not comment table) — sentinel matching works regardless of edits + +## Inline Comments Lifecycle + +| Event | Inline comments behavior | +|-------|--------------------------| +| First POST | Created on specific diff lines | +| PATCH body (same state) | Unchanged — still current findings | +| Supersede (state change) | Old inline comments stay (readable but on outdated code) | +| New POST after supersede | Fresh inline comments on current diff | -- 2.47.3