diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index f31cae4..8163ea1 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -241,44 +241,107 @@ 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)) + } + + // --- 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 { + log.Printf("Warning: could not list existing reviews: %v", err) + } else { + // 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" + } + + 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 find review comment ID, falling back to new post: %v", err) + } else { + 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) - 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) } 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 != "" { + // 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 { - 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) - } - } + 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) } - - // 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) - if err != nil { - log.Printf("Warning: could not re-post as REQUEST_CHANGES: %v", err) - } else { - log.Printf("Review escalated to REQUEST_CHANGES") - } - } + // 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") } } } @@ -438,14 +501,50 @@ 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,10 +142,149 @@ 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) } }) } } + +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) + } + }) + } +} + +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/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 | diff --git a/gitea/client.go b/gitea/client.go index 1b4a472..9355e29 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) @@ -343,3 +353,81 @@ 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, 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 { + 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 eabdd9e..be9b94e 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") } @@ -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") + } +} diff --git a/gitea/diff.go b/gitea/diff.go new file mode 100644 index 0000000..d191189 --- /dev/null +++ b/gitea/diff.go @@ -0,0 +1,85 @@ +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 @@ or @@ -old +new @@ + if strings.HasPrefix(line, "@@") && currentFile != "" { + // Extract the +N part — handle both "+10,8" and "+1" forms + parts := strings.Split(line, "+") + if len(parts) >= 2 { + // 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 + } + } + continue + } + + if currentFile == "" { + 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 + 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..f73b39e --- /dev/null +++ b/gitea/diff_test.go @@ -0,0 +1,115 @@ +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") + } +} + +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)") + } +} 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") + } + } +}