Compare commits

..

6 Commits

Author SHA1 Message Date
claw 8e26c26f5f fix(github): add pagination tests and fix truncation warning logic
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 20s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 24s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 56s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m31s
F1: Add comprehensive pagination tests for ListReviews covering:
- Multi-page behaviour (2 full + 1 partial page)
- Exact-multiple-of-pageSize (extra empty-page round-trip)
- maxReviewPages cutoff (cap hit, results still returned)
- Empty first page (PR with no reviews)

F2: Fix truncation warning logic by moving it outside the loop with
a 'truncated' flag. Previously, the warning fired inline at page==maxPages
which could miss the case where the short-page break fires first on the
cap page. Now it only fires when the loop exits because the cap was reached
and the last page was full (indicating more data likely exists).

Also adds SetReviewPagination to Client for test-time override of page
size and max pages, following the existing SetRetryBackoff pattern.
2026-05-13 17:22:51 -07:00
claw 22b3ce8fef fix(github): consolidate review.go and identity.go into reviews.go (#116)
Remove github/review.go and github/identity.go, replacing them with a
consolidated github/reviews.go that:

- Uses doJSONRequest for PostReview and DismissReview (cleaner than
  manual marshal + doRequestWithBody)
- Adds paginated ListReviews with per_page=100 and max 100 pages
- Consolidates GetAuthenticatedUser and userResponse type (previously
  duplicated in identity.go)
- Preserves all sentinel errors (ErrCannotDeleteSubmittedReview,
  ErrConflictingCommitIDs), state translation, commit ID validation,
  and SupersedeReviews

This prevents the redeclaration errors that occur when both review.go
and reviews.go exist in the same package, as described in issue #116.

Closes #116
2026-05-13 17:21:24 -07:00
aweiker b2c83c00bc Merge pull request 'fix(vcs): thread CommitID through abstraction layer (#114)' (#117) from review-bot-issue-114 into feature/github-support
Reviewed-on: #117
Reviewed-by: security-review-bot <10+security-review-bot@noreply.gitea.weiker.me>
Reviewed-by: Aaron Weiker <aaron@weiker.org>
2026-05-13 23:13:21 +00:00
claw 25cb55449e fix(nit): align CommitID field in vcs/types.go and document no-op in github/review.go
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 19s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 29s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 48s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m38s
2026-05-13 13:49:41 -07:00
claw 7e3b6ec8f1 fix(vcs): thread CommitID through abstraction layer (#114)
CI / test (pull_request) Successful in 20s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 46s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m24s
Add CommitID field to vcs.ReviewRequest so the commit anchor propagates
through the vcs.Client interface to platform adapters.

Changes:
- vcs/types.go: Add CommitID string field to ReviewRequest
- gitea/client.go: Add commitID parameter to PostReview, include in API payload
- gitea/adapter.go: Pass req.CommitID to underlying client
- github/review.go: Use req.CommitID as primary, fall back to comment-level
- cmd/review-bot/main.go: Set CommitID on ReviewRequest from pr.Head.SHA

Fixes #114
2026-05-13 13:30:48 -07:00
aweiker a32a5b694b Merge pull request 'feat(cmd): wire --provider and --base-url flags into CLI (Phase 5)' (#106) from review-bot-issue-82 into feature/github-support
Reviewed-on: #106
Reviewed-by: security-review-bot <10+security-review-bot@noreply.gitea.weiker.me>
Reviewed-by: Aaron Weiker <aaron@weiker.org>
2026-05-13 17:16:28 +00:00
11 changed files with 544 additions and 80 deletions
+1
View File
@@ -485,6 +485,7 @@ func main() {
reviewReq := vcs.ReviewRequest{
Body: reviewBody,
Event: event,
CommitID: pr.Head.SHA,
Comments: inlineComments,
}
posted, err := client.PostReview(ctx, owner, repoName, prNumber, reviewReq)
+4 -4
View File
@@ -170,9 +170,9 @@ func (a *Adapter) PostReview(ctx context.Context, owner, repo string, number int
if err != nil {
return nil, fmt.Errorf("translate position %d in %s: %w", c.Position, c.Path, err)
}
// CommitID from vcs.ReviewComment is intentionally not forwarded:
// Gitea review comments are pinned to the PR head SHA automatically,
// and the CreatePullReview API has no per-comment commit_id field.
// Per-comment CommitID is not forwarded to Gitea inline comments:
// Gitea's CreatePullReview API has no per-comment commit_id field.
// The review-level commit anchor is set via req.CommitID instead.
giteaComments = append(giteaComments, ReviewComment{
Path: c.Path,
NewPosition: int64(lineNum),
@@ -181,7 +181,7 @@ func (a *Adapter) PostReview(ctx context.Context, owner, repo string, number int
}
}
review, err := a.client.PostReview(ctx, owner, repo, number, event, req.Body, giteaComments)
review, err := a.client.PostReview(ctx, owner, repo, number, event, req.Body, req.CommitID, giteaComments)
if err != nil {
return nil, fmt.Errorf("post review: %w", err)
}
+70
View File
@@ -408,3 +408,73 @@ func TestAdapter_RequestReviewerSelf(t *testing.T) {
t.Fatalf("RequestReviewerSelf() error = %v", err)
}
}
func TestAdapter_PostReview_CommitID_Threading(t *testing.T) {
var gotPayload struct {
Body string `json:"body"`
Event string `json:"event"`
CommitID string `json:"commit_id"`
}
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewDecoder(r.Body).Decode(&gotPayload)
json.NewEncoder(w).Encode(map[string]any{
"id": 1,
"body": "test",
"user": map[string]any{"login": "bot"},
"commit_id": "abc123def456",
})
}))
defer server.Close()
client := gitea.NewClient(server.URL, "token")
adapter := gitea.NewAdapter(client)
review, err := adapter.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
Body: "LGTM",
Event: vcs.ReviewEventApprove,
CommitID: "abc123def456",
// No comments → no diff fetch needed
})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if gotPayload.CommitID != "abc123def456" {
t.Errorf("commit_id = %q, want %q", gotPayload.CommitID, "abc123def456")
}
if review.CommitID != "abc123def456" {
t.Errorf("review.CommitID = %q, want %q", review.CommitID, "abc123def456")
}
}
func TestAdapter_PostReview_EmptyCommitID_Omitted(t *testing.T) {
var gotRawPayload map[string]any
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewDecoder(r.Body).Decode(&gotRawPayload)
json.NewEncoder(w).Encode(map[string]any{
"id": 1,
"body": "test",
"user": map[string]any{"login": "bot"},
})
}))
defer server.Close()
client := gitea.NewClient(server.URL, "token")
adapter := gitea.NewAdapter(client)
_, err := adapter.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
Body: "looks good",
Event: vcs.ReviewEventComment,
// CommitID intentionally empty
})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
// With empty CommitID and omitempty tag, the field should not appear in JSON
if _, exists := gotRawPayload["commit_id"]; exists {
t.Errorf("commit_id should be omitted when empty, but was present: %v", gotRawPayload["commit_id"])
}
}
+6 -2
View File
@@ -186,18 +186,22 @@ 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".
// event should be one of "APPROVED", "REQUEST_CHANGES", or "COMMENT".
// commitID anchors the review to a specific commit SHA. If empty, Gitea
// defaults to the current PR head.
// 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) {
func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []ReviewComment) (*Review, error) {
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
payload := struct {
Body string `json:"body"`
Event string `json:"event"`
CommitID string `json:"commit_id,omitempty"`
Comments []ReviewComment `json:"comments,omitempty"`
}{
Body: body,
Event: event,
CommitID: commitID,
Comments: comments,
}
+61 -2
View File
@@ -135,7 +135,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", nil)
review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", "", nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
@@ -182,7 +182,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", nil)
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", "", nil)
if err == nil {
t.Fatal("expected error for 403, got nil")
}
@@ -1144,3 +1144,62 @@ func TestSanitizeErrorForLog(t *testing.T) {
})
}
}
func TestPostReview_CommitID_InPayload(t *testing.T) {
var gotPayload struct {
Body string `json:"body"`
Event string `json:"event"`
CommitID string `json:"commit_id"`
}
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": 200,
"body": "LGTM",
"user": map[string]any{"login": "bot"},
"state": "APPROVED",
"commit_id": "deadbeef1234",
})
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
review, err := client.PostReview(context.Background(), "owner", "repo", 5, "APPROVED", "LGTM", "deadbeef1234", nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if gotPayload.CommitID != "deadbeef1234" {
t.Errorf("sent commit_id = %q, want %q", gotPayload.CommitID, "deadbeef1234")
}
if review.CommitID != "deadbeef1234" {
t.Errorf("response commit_id = %q, want %q", review.CommitID, "deadbeef1234")
}
}
func TestPostReview_EmptyCommitID_OmittedFromPayload(t *testing.T) {
var gotRaw map[string]any
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
json.NewDecoder(r.Body).Decode(&gotRaw)
w.Header().Set("Content-Type", "application/json")
w.WriteHeader(200)
json.NewEncoder(w).Encode(map[string]any{
"id": 201,
"body": "ok",
"user": map[string]any{"login": "bot"},
})
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
_, err := client.PostReview(context.Background(), "owner", "repo", 5, "COMMENT", "ok", "", nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if _, exists := gotRaw["commit_id"]; exists {
t.Errorf("commit_id should be omitted when empty, but was present: %v", gotRaw["commit_id"])
}
}
+2 -2
View File
@@ -37,7 +37,7 @@ func TestPostReview_WithComments(t *testing.T) {
{Path: "util.go", NewPosition: 10, Body: "[MINOR] Style issue"},
}
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "REQUEST_CHANGES", "summary", comments)
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "REQUEST_CHANGES", "summary", "", comments)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
@@ -72,7 +72,7 @@ func TestPostReview_NilComments(t *testing.T) {
defer server.Close()
client := NewClient(server.URL, "test-token")
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "all good", nil)
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "all good", "", nil)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
+12
View File
@@ -110,6 +110,11 @@ type Client struct {
// retryBackoff[i] is the delay before attempt i+1 (after attempt i fails).
// If nil, defaults to {1s, 2s}. Set to shorter durations in tests via SetRetryBackoff.
retryBackoff []time.Duration
// reviewPageSize overrides reviewsPerPage for testing. Zero means use default.
reviewPageSize int
// reviewMaxPages overrides maxReviewPages for testing. Zero means use default.
reviewMaxPages int
}
// defaultCheckRedirect is the redirect policy used by NewClient and SetHTTPClient(nil).
@@ -194,6 +199,13 @@ func (c *Client) SetRetryBackoff(d []time.Duration) error {
return nil
}
// SetReviewPagination overrides the page size and max pages for ListReviews.
// Intended for testing only; must be called before any goroutines issue requests.
func (c *Client) SetReviewPagination(pageSize, maxPages int) {
c.reviewPageSize = pageSize
c.reviewMaxPages = maxPages
}
// requestOptions holds per-request configuration for doRequestCore.
type requestOptions struct {
// bodyFn returns a fresh io.Reader for the request body on each attempt.
-29
View File
@@ -1,29 +0,0 @@
package github
import (
"context"
"encoding/json"
"fmt"
)
// userResponse is the GitHub API response for the authenticated user.
type userResponse struct {
Login string `json:"login"`
}
// GetAuthenticatedUser returns the login of the currently authenticated user.
func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) {
reqURL := fmt.Sprintf("%s/user", c.baseURL)
body, err := c.doGet(ctx, reqURL)
if err != nil {
return "", fmt.Errorf("get authenticated user: %w", err)
}
var resp userResponse
if err := json.Unmarshal(body, &resp); err != nil {
return "", fmt.Errorf("parse user response: %w", err)
}
return resp.Login, nil
}
+286
View File
@@ -2,6 +2,7 @@ package github
import (
"context"
"fmt"
"encoding/json"
"errors"
"io"
@@ -389,3 +390,288 @@ func TestPostReview_ConflictingCommitIDs(t *testing.T) {
t.Errorf("expected ErrConflictingCommitIDs, got: %v", err)
}
}
func TestPostReview_RequestCommitID_TakesPriority(t *testing.T) {
var gotPayload struct {
CommitID string `json:"commit_id"`
Body string `json:"body"`
}
c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
json.NewDecoder(r.Body).Decode(&gotPayload)
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]any{
"id": 42,
"body": "LGTM",
"state": "APPROVED",
"commit_id": "req-level-sha",
"user": map[string]any{"login": "bot"},
})
})
review, err := c.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
Body: "LGTM",
Event: vcs.ReviewEventApprove,
CommitID: "req-level-sha",
Comments: []vcs.ReviewComment{
{Path: "a.go", Position: 1, CommitID: "req-level-sha", Body: "looks good"},
},
})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if gotPayload.CommitID != "req-level-sha" {
t.Errorf("sent commit_id = %q, want %q", gotPayload.CommitID, "req-level-sha")
}
if review.CommitID != "req-level-sha" {
t.Errorf("review.CommitID = %q, want %q", review.CommitID, "req-level-sha")
}
}
func TestPostReview_RequestCommitID_ConflictsWithComment(t *testing.T) {
c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
t.Fatal("request should not be sent when commit IDs conflict")
})
// req.CommitID is set, and a comment has a different CommitID → conflict
_, err := c.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
Body: "Review",
Event: vcs.ReviewEventComment,
CommitID: "req-sha",
Comments: []vcs.ReviewComment{
{Path: "a.go", Position: 1, CommitID: "different-sha", Body: "nit"},
},
})
if err == nil {
t.Fatal("expected error for conflicting commit IDs")
}
if !errors.Is(err, ErrConflictingCommitIDs) {
t.Errorf("expected ErrConflictingCommitIDs, got: %v", err)
}
}
func TestPostReview_RequestCommitID_FallbackToComment(t *testing.T) {
var gotPayload struct {
CommitID string `json:"commit_id"`
}
c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
json.NewDecoder(r.Body).Decode(&gotPayload)
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]any{
"id": 43,
"body": "ok",
"state": "COMMENTED",
"commit_id": "comment-sha",
"user": map[string]any{"login": "bot"},
})
})
// req.CommitID is empty, so it falls back to the comment's CommitID
_, err := c.PostReview(context.Background(), "owner", "repo", 1, vcs.ReviewRequest{
Body: "ok",
Event: vcs.ReviewEventComment,
// CommitID intentionally empty
Comments: []vcs.ReviewComment{
{Path: "a.go", Position: 1, CommitID: "comment-sha", Body: "note"},
},
})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if gotPayload.CommitID != "comment-sha" {
t.Errorf("sent commit_id = %q, want %q (fallback from comment)", gotPayload.CommitID, "comment-sha")
}
}
// --- ListReviews pagination tests ---
func TestListReviews_MultiPage(t *testing.T) {
// Test multi-page pagination: 2 full pages + 1 partial page.
// pageSize=3, so pages return [3, 3, 2] reviews = 8 total.
const pageSize = 3
callCount := 0
c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
if r.Method != "GET" {
t.Fatalf("expected GET, got %s", r.Method)
}
callCount++
page := r.URL.Query().Get("page")
var reviews []map[string]interface{}
switch page {
case "1":
for i := 1; i <= pageSize; i++ {
reviews = append(reviews, map[string]interface{}{
"id": i, "body": fmt.Sprintf("review %d", i),
"state": "APPROVED", "commit_id": "sha1",
"user": map[string]string{"login": "user1"},
})
}
case "2":
for i := pageSize + 1; i <= pageSize*2; i++ {
reviews = append(reviews, map[string]interface{}{
"id": i, "body": fmt.Sprintf("review %d", i),
"state": "COMMENTED", "commit_id": "sha1",
"user": map[string]string{"login": "user2"},
})
}
case "3":
// Partial page: only 2 reviews (less than pageSize)
for i := pageSize*2 + 1; i <= pageSize*2+2; i++ {
reviews = append(reviews, map[string]interface{}{
"id": i, "body": fmt.Sprintf("review %d", i),
"state": "CHANGES_REQUESTED", "commit_id": "sha1",
"user": map[string]string{"login": "user3"},
})
}
default:
t.Fatalf("unexpected page: %s", page)
}
json.NewEncoder(w).Encode(reviews)
})
c.SetReviewPagination(pageSize, 10)
reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(reviews) != 8 {
t.Fatalf("expected 8 reviews, got %d", len(reviews))
}
if callCount != 3 {
t.Errorf("expected 3 API calls, got %d", callCount)
}
// Verify reviews are correctly concatenated in order
for i, r := range reviews {
expectedID := int64(i + 1)
if r.ID != expectedID {
t.Errorf("review[%d]: expected ID %d, got %d", i, expectedID, r.ID)
}
}
}
func TestListReviews_ExactMultipleOfPageSize(t *testing.T) {
// When total reviews is an exact multiple of pageSize, an extra request
// returning 0 results terminates the loop. No truncation warning.
const pageSize = 2
callCount := 0
c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
callCount++
page := r.URL.Query().Get("page")
var reviews []map[string]interface{}
switch page {
case "1":
reviews = []map[string]interface{}{
{"id": 1, "body": "r1", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u1"}},
{"id": 2, "body": "r2", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u2"}},
}
case "2":
reviews = []map[string]interface{}{
{"id": 3, "body": "r3", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u3"}},
{"id": 4, "body": "r4", "state": "APPROVED", "commit_id": "s1", "user": map[string]string{"login": "u4"}},
}
case "3":
// Empty page — signals end of data
reviews = []map[string]interface{}{}
default:
t.Fatalf("unexpected page: %s", page)
}
json.NewEncoder(w).Encode(reviews)
})
c.SetReviewPagination(pageSize, 10)
reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(reviews) != 4 {
t.Fatalf("expected 4 reviews, got %d", len(reviews))
}
// 3 calls: page 1 (full), page 2 (full), page 3 (empty)
if callCount != 3 {
t.Errorf("expected 3 API calls, got %d", callCount)
}
}
func TestListReviews_MaxPagesCutoff(t *testing.T) {
// When maxPages is hit and the last page is full, results are truncated
// and a warning would fire (we verify the reviews are still returned).
const pageSize = 2
const maxPages = 2
callCount := 0
c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
callCount++
page := r.URL.Query().Get("page")
// Always return a full page (simulating more data exists)
var reviews []map[string]interface{}
var baseID int
switch page {
case "1":
baseID = 0
case "2":
baseID = pageSize
default:
t.Fatalf("unexpected page %s (should not exceed maxPages)", page)
}
for i := 1; i <= pageSize; i++ {
reviews = append(reviews, map[string]interface{}{
"id": baseID + i, "body": fmt.Sprintf("r%d", baseID+i),
"state": "APPROVED", "commit_id": "sha1",
"user": map[string]string{"login": "user"},
})
}
json.NewEncoder(w).Encode(reviews)
})
c.SetReviewPagination(pageSize, maxPages)
reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
// Should return all reviews fetched within the cap
expectedCount := pageSize * maxPages
if len(reviews) != expectedCount {
t.Fatalf("expected %d reviews, got %d", expectedCount, len(reviews))
}
if callCount != maxPages {
t.Errorf("expected %d API calls, got %d", maxPages, callCount)
}
// Verify concatenation order
for i, r := range reviews {
if r.ID != int64(i+1) {
t.Errorf("review[%d]: expected ID %d, got %d", i, i+1, r.ID)
}
}
}
func TestListReviews_EmptyFirstPage(t *testing.T) {
// PR with no reviews: first page returns empty array.
callCount := 0
c := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
callCount++
json.NewEncoder(w).Encode([]map[string]interface{}{})
})
c.SetReviewPagination(10, 5)
reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(reviews) != 0 {
t.Fatalf("expected 0 reviews, got %d", len(reviews))
}
if callCount != 1 {
t.Errorf("expected 1 API call, got %d", callCount)
}
}
+96 -40
View File
@@ -5,12 +5,21 @@ import (
"encoding/json"
"errors"
"fmt"
"log/slog"
"net/http"
"net/url"
"gitea.weiker.me/rodin/review-bot/vcs"
)
const (
// reviewsPerPage is the number of reviews to fetch per API page.
reviewsPerPage = 100
// maxReviewPages is the maximum number of pages to paginate through
// when listing reviews. Acts as a safeguard against infinite pagination.
maxReviewPages = 100
)
// ErrCannotDeleteSubmittedReview is returned when DeleteReview is called on
// a review that has already been submitted (APPROVED, REQUEST_CHANGES, COMMENT).
// GitHub only allows deletion of PENDING reviews. Callers that need to replace
@@ -54,6 +63,11 @@ type dismissReviewRequest struct {
Event string `json:"event"`
}
// userResponse is the GitHub API response for the authenticated user.
type userResponse struct {
Login string `json:"login"`
}
// translateGitHubReviewState translates a GitHub API review state to the
// canonical vcs.Review.State value.
func translateGitHubReviewState(state string) string {
@@ -82,21 +96,25 @@ func translateGitHubReviewState(state string) string {
// (via the omitempty tag on postReviewRequest.Comments).
//
// The GitHub API accepts a single commit_id per review submission. PostReview
// extracts it from the first comment with a non-empty CommitID. If any subsequent
// comment specifies a different CommitID, PostReview returns ErrConflictingCommitIDs.
// Comments with an empty CommitID are allowed and inherit the review-level value.
// uses req.CommitID as the primary commit anchor. If req.CommitID is empty,
// it falls back to extracting from the first comment with a non-empty CommitID.
// If any subsequent comment specifies a different CommitID, PostReview returns
// ErrConflictingCommitIDs. Comments with an empty CommitID are allowed and
// inherit the review-level value.
func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, req vcs.ReviewRequest) (*vcs.Review, error) {
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
payload := postReviewRequest{
Body: req.Body,
Event: string(req.Event),
Body: req.Body,
Event: string(req.Event),
CommitID: req.CommitID,
}
// Build the payload in one pass. The GitHub API accepts a single commit_id
// per review; we extract it from the first comment that supplies one and
// reject the request if any other comment disagrees.
// per review. req.CommitID is the primary source; if empty, we extract from
// the first comment that supplies one. Reject if any comment disagrees with
// the resolved commit_id.
for _, comment := range req.Comments {
if comment.CommitID != "" {
if payload.CommitID == "" {
@@ -104,6 +122,7 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
} else if payload.CommitID != comment.CommitID {
return nil, ErrConflictingCommitIDs
}
// else: matching SHA is a no-op by design
}
payload.Comments = append(payload.Comments, reviewCommentEntry{
Path: comment.Path,
@@ -112,12 +131,7 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
})
}
data, err := json.Marshal(payload)
if err != nil {
return nil, fmt.Errorf("marshal review request: %w", err)
}
body, err := c.doRequestWithBody(ctx, http.MethodPost, reqURL, data)
body, err := c.doJSONRequest(ctx, http.MethodPost, reqURL, payload)
if err != nil {
return nil, fmt.Errorf("post review: %w", err)
}
@@ -136,33 +150,67 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
}, nil
}
// ListReviews retrieves all reviews for a pull request.
// ListReviews retrieves all reviews for a pull request with pagination.
// GitHub review states are translated to canonical vcs values.
func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcs.Review, error) {
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
body, err := c.doGet(ctx, reqURL)
if err != nil {
return nil, fmt.Errorf("list reviews: %w", err)
perPage := reviewsPerPage
if c.reviewPageSize > 0 {
perPage = c.reviewPageSize
}
maxPages := maxReviewPages
if c.reviewMaxPages > 0 {
maxPages = c.reviewMaxPages
}
var responses []reviewResponse
if err := json.Unmarshal(body, &responses); err != nil {
return nil, fmt.Errorf("parse reviews response: %w", err)
}
var allReviews []vcs.Review
truncated := false
reviews := make([]vcs.Review, len(responses))
for i, r := range responses {
reviews[i] = vcs.Review{
ID: r.ID,
Body: r.Body,
User: vcs.UserInfo{Login: r.User.Login},
State: translateGitHubReviewState(r.State),
CommitID: r.CommitID,
for page := 1; page <= maxPages; page++ {
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews?per_page=%d&page=%d",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, perPage, page)
body, err := c.doGet(ctx, reqURL)
if err != nil {
return nil, fmt.Errorf("list reviews page %d: %w", page, err)
}
var responses []reviewResponse
if err := json.Unmarshal(body, &responses); err != nil {
return nil, fmt.Errorf("parse reviews response: %w", err)
}
if len(responses) == 0 {
break
}
for _, r := range responses {
allReviews = append(allReviews, vcs.Review{
ID: r.ID,
Body: r.Body,
User: vcs.UserInfo{Login: r.User.Login},
State: translateGitHubReviewState(r.State),
CommitID: r.CommitID,
})
}
if len(responses) < perPage {
break
}
// If we just fetched page maxPages and it was full (the short-page break
// above didn't fire), more reviews likely exist beyond our limit.
if page == maxPages {
truncated = true
}
}
return reviews, nil
if truncated {
slog.Warn("ListReviews hit page limit; results may be truncated",
"owner", owner, "repo", repo, "pr", number,
"maxPages", maxPages, "reviewsFetched", len(allReviews))
}
return allReviews, nil
}
// DeleteReview deletes a pull request review.
@@ -173,7 +221,6 @@ func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number in
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
// nil body: the GitHub DELETE endpoint for reviews requires no request body.
_, err := c.doRequestWithBody(ctx, http.MethodDelete, reqURL, nil)
if err != nil {
var apiErr *APIError
@@ -199,12 +246,7 @@ func (c *Client) DismissReview(ctx context.Context, owner, repo string, number i
Event: "DISMISS",
}
data, err := json.Marshal(payload)
if err != nil {
return fmt.Errorf("marshal dismiss request: %w", err)
}
_, err = c.doRequestWithBody(ctx, http.MethodPut, reqURL, data)
_, err := c.doJSONRequest(ctx, http.MethodPut, reqURL, payload)
if err != nil {
return fmt.Errorf("dismiss review: %w", err)
}
@@ -223,3 +265,17 @@ func (c *Client) SupersedeReviews(ctx context.Context, owner, repo string, prNum
}
return errors.Join(errs...)
}
// GetAuthenticatedUser returns the login name of the authenticated user.
func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) {
reqURL := fmt.Sprintf("%s/user", c.baseURL)
body, err := c.doGet(ctx, reqURL)
if err != nil {
return "", fmt.Errorf("get authenticated user: %w", err)
}
var resp userResponse
if err := json.Unmarshal(body, &resp); err != nil {
return "", fmt.Errorf("parse user response: %w", err)
}
return resp.Login, nil
}
+6 -1
View File
@@ -93,6 +93,11 @@ type ReviewRequest struct {
// Body is the top-level review comment.
Body string `json:"body"`
// Event is the review action (approve, request changes, or comment).
Event ReviewEvent `json:"event"`
Event ReviewEvent `json:"event"`
// CommitID anchors the review to a specific commit SHA.
// If empty, the platform defaults to the current PR head.
// Adapters use this as the primary commit anchor for the review submission.
CommitID string `json:"commit_id,omitempty"`
Comments []ReviewComment `json:"comments,omitempty"`
}