Merge pull request 'fix(github): consolidate review.go and identity.go into reviews.go (#116)' (#119) from review-bot-issue-116 into feature/github-support

Reviewed-on: #119
Reviewed-by: Aaron Weiker <aaron@weiker.org>
Reviewed-by: security-review-bot <10+security-review-bot@noreply.gitea.weiker.me>

cmd/review-bot/main.go

@@ -439,7 +439,7 @@ func main() {
 		inlineComments = append(inlineComments, vcs.ReviewComment{
 			Path:     f.File,
 			Position: pos,
-			CommitID: pr.Head.SHA,
+			CommitID: evaluatedSHA,
 			Body:     fmt.Sprintf("**[%s]** %s", f.Severity, f.Finding),
 		})
 	}
@@ -485,7 +485,7 @@ func main() {
 	reviewReq := vcs.ReviewRequest{
 		Body:     reviewBody,
 		Event:    event,
-		CommitID: pr.Head.SHA,
+		CommitID: evaluatedSHA,
 		Comments: inlineComments,
 	}
 	posted, err := client.PostReview(ctx, owner, repoName, prNumber, reviewReq)

gitea/adapter_test.go

@@ -409,7 +409,7 @@ func TestAdapter_RequestReviewerSelf(t *testing.T) {
 	}
 }
 
-func TestAdapter_PostReview_CommitID_Threading(t *testing.T) {
+func TestAdapter_PostReview_CommitID(t *testing.T) {
 	var gotPayload struct {
 		Body     string `json:"body"`
 		Event    string `json:"event"`

gitea/client_test.go

@@ -147,6 +147,46 @@ func TestPostReview(t *testing.T) {
 	}
 }
 
+func TestPostReview_CommitID(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.Method != "POST" {
+			t.Fatalf("expected POST, got %s", r.Method)
+		}
+
+		body, _ := io.ReadAll(r.Body)
+		var payload struct {
+			Body     string `json:"body"`
+			Event    string `json:"event"`
+			CommitID string `json:"commit_id"`
+		}
+		if err := json.Unmarshal(body, &payload); err != nil {
+			t.Fatalf("unmarshal payload: %v", err)
+		}
+		if payload.CommitID != "deadbeef123" {
+			t.Errorf("expected commit_id %q, got %q", "deadbeef123", payload.CommitID)
+		}
+		if payload.Event != "APPROVED" {
+			t.Errorf("expected event APPROVED, got %q", payload.Event)
+		}
+
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte(`{"id":101,"user":{"login":"review-bot"},"state":"APPROVED","stale":false,"commit_id":"deadbeef123"}`))
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", "deadbeef123", nil)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if review.ID != 101 {
+		t.Errorf("expected review ID 101, got %d", review.ID)
+	}
+	if review.CommitID != "deadbeef123" {
+		t.Errorf("expected commit_id %q, got %q", "deadbeef123", review.CommitID)
+	}
+}
+
 func TestGetPullRequest_Non200(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		w.WriteHeader(http.StatusNotFound)

github/review_test.go

@@ -2,9 +2,9 @@ package github
 
 import (
 	"context"
-	"fmt"
 	"encoding/json"
 	"errors"
+	"fmt"
 	"io"
 	"net/http"
 	"strings"

github/reviews.go

@@ -117,7 +117,7 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
 	// the resolved commit_id.
 	for _, comment := range req.Comments {
 		if comment.CommitID != "" {
-			if payload.CommitID == "" {
+			if payload.CommitID == "" { // only reachable when req.CommitID is empty
 				payload.CommitID = comment.CommitID
 			} else if payload.CommitID != comment.CommitID {
 				return nil, ErrConflictingCommitIDs
@@ -197,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
+		// Truncation detection: this runs on the final allowed iteration
-		// above didn't fire), more reviews likely exist beyond our limit.
+		// (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
 		}
@@ -221,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