Compare commits

..

7 Commits

Author SHA1 Message Date
claw 437e318240 nit: clarify truncation detection comment in ListReviews
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 19s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 39s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 39s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 59s
Expand the inline comment at the page==maxPages check to more
explicitly explain why a full final page implies truncation.
2026-05-13 18:01:57 -07:00
claw 2e2fcbabfc style: fix import ordering and restore nil-body comment
PR Ready Gate / clear-labels (pull_request) Successful in 2s
CI / test (pull_request) Successful in 19s
CI / review (gpt-5, security, ., rodin/security-patterns, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 38s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 40s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 56s
- Reorder stdlib imports in review_test.go to alphabetical (goimports convention)
- Restore explanatory comment for nil body in DeleteReview

Addresses review comments #20533, #20534 on PR #119
2026-05-13 17:53:20 -07:00
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
9 changed files with 261 additions and 21 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)
}
+94 -1
View File
@@ -2,9 +2,9 @@ package github
import (
"context"
"fmt"
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
"strings"
@@ -391,6 +391,99 @@ func TestPostReview_ConflictingCommitIDs(t *testing.T) {
}
}
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) {
+17 -9
View File
@@ -96,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 == "" {
@@ -118,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,
@@ -192,8 +197,10 @@ func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int
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.
// Truncation detection: this runs on the final allowed iteration
// (page == maxPages) only when the page was full (the len < perPage
// early-break above didn't fire). A full final page means additional
// reviews likely exist beyond our pagination limit.
if page == maxPages {
truncated = true
}
@@ -216,6 +223,7 @@ 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
+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"`
}