rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

feat(github): implement Reviewer and Identity interfaces (#81) #105

Merged
aweiker merged 7 commits from review-bot-issue-81 into feature/github-support 2026-05-13 13:39:14 +00:00
Conversation 40 Commits 7 Files Changed 7
+54 -135
3 changed files with 54 additions and 135 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit eba97321ad - Show all commits
github/client.go
+44 -114
View File
@@ -193,10 +193,24 @@ func (c *Client) SetRetryBackoff(d []time.Duration) error {
return nil
}
// doRequest performs an HTTP request with retry on 429 rate limit responses.
// It respects the Retry-After header when present (capped at maxRetryAfter).
// Transport errors (network failures, context cancellation) are not retried.
func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
// requestOptions holds per-request configuration for doRequestCore.
type requestOptions struct {
gpt-review-bot commented 2026-05-13 07:38:51 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The requestOptions.bodyFn comment lists POST, PUT, PATCH as body-carrying methods but omits DELETE-with-body. Update the comment to reflect that it must be non-nil for any request that carries a body (including DELETE when applicable) to avoid confusion.

**[NIT]** The requestOptions.bodyFn comment lists POST, PUT, PATCH as body-carrying methods but omits DELETE-with-body. Update the comment to reflect that it must be non-nil for any request that carries a body (including DELETE when applicable) to avoid confusion.
// bodyFn returns a fresh io.Reader for the request body on each attempt.
// Must be non-nil for requests that carry a body (POST, PUT, PATCH).
// Returning a fresh reader on each call allows retries to re-send the body.
bodyFn func() io.Reader
// accept overrides the default Accept header. Empty means "application/vnd.github+json".
accept string
// extraHeaders are additional headers to set on each request attempt.
extraHeaders map[string]string
}
// doRequestCore is the shared implementation for all HTTP requests with retry
// on 429 rate limit responses. It respects the Retry-After header when present
// (capped at maxRetryAfter). Transport errors are not retried.
func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts requestOptions) ([]byte, error) {
const maxRetryAfter = 120 * time.Second
// backoff holds per-attempt delays: backoff[i] is the delay before attempt i+1.
@@ -247,7 +261,11 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
}
}
req, err := http.NewRequestWithContext(ctx, method, reqURL, nil)
var body io.Reader
if opts.bodyFn != nil {
body = opts.bodyFn()
}
req, err := http.NewRequestWithContext(ctx, method, reqURL, body)
if err != nil {
return nil, fmt.Errorf("create request: %w", err)
}
@@ -258,11 +276,14 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
req.Header.Set("Authorization", "Bearer "+c.token)
}
req.Header.Set("User-Agent", userAgent)
if accept != "" {
req.Header.Set("Accept", accept)
if opts.accept != "" {
req.Header.Set("Accept", opts.accept)
} else {
req.Header.Set("Accept", "application/vnd.github+json")
}
for k, v := range opts.extraHeaders {
req.Header.Set(k, v)
}
resp, err := c.httpClient.Do(req)
if err != nil {
@@ -273,11 +294,11 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
respStatus := resp.StatusCode
retryAfterHeader := resp.Header.Get("Retry-After")
body, done, err := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
respBody, done, handleErr := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
if done {
return body, err
return respBody, handleErr
}
lastErr = err
lastErr = handleErr
// Retry on 429 rate limit
if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 {
@@ -315,6 +336,13 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
return nil, lastErr
}
// doRequest performs an HTTP request with retry on 429 rate limit responses.
// It respects the Retry-After header when present (capped at maxRetryAfter).
// Transport errors (network failures, context cancellation) are not retried.
func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
return c.doRequestCore(ctx, method, reqURL, requestOptions{accept: accept})
}
// handleResponse reads and closes the response body, returning the result.
sonnet-review-bot commented 2026-05-13 07:26:59 +00:00
Outdated
Review
Copy Link
Copy Source

[MAJOR] doRequestWithBody duplicates ~90% of doRequest's implementation. Both functions share identical backoff setup, HTTPS validation, retry loop, timer/select pattern, Retry-After parsing, and handleResponse invocation. The only difference is that doRequestWithBody constructs a bytes.NewReader(body) and sets Content-Type: application/json. This violates DRY and makes the retry logic a two-place maintenance burden. The fix is to refactor doRequest to accept an optional body (e.g. add an io.Reader or []byte parameter, or introduce a private doRequestInternal that both exported helpers delegate to). The pattern to follow is the existing Layered API pattern: doGet calls doRequest, and doRequestWithBody should also call a single underlying implementation.

**[MAJOR]** doRequestWithBody duplicates ~90% of doRequest's implementation. Both functions share identical backoff setup, HTTPS validation, retry loop, timer/select pattern, Retry-After parsing, and handleResponse invocation. The only difference is that doRequestWithBody constructs a `bytes.NewReader(body)` and sets `Content-Type: application/json`. This violates DRY and makes the retry logic a two-place maintenance burden. The fix is to refactor doRequest to accept an optional body (e.g. add an `io.Reader` or `[]byte` parameter, or introduce a private `doRequestInternal` that both exported helpers delegate to). The pattern to follow is the existing Layered API pattern: `doGet` calls `doRequest`, and `doRequestWithBody` should also call a single underlying implementation.
gpt-review-bot commented 2026-05-13 07:28:05 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] doRequestWithBody largely duplicates doRequest logic (retry/backoff, HTTPS checks, headers). Consider refactoring to a single helper that accepts an optional body and optional Accept header to avoid drift and ease maintenance.

**[MINOR]** doRequestWithBody largely duplicates doRequest logic (retry/backoff, HTTPS checks, headers). Consider refactoring to a single helper that accepts an optional body and optional Accept header to avoid drift and ease maintenance.
// It uses defer to ensure the body is always closed regardless of code path.
// Returns (body, done, err) where done=true means the caller should return immediately.
@@ -347,109 +375,11 @@ func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
// doRequestWithBody is like doRequest but sends a request body.
// It accepts the raw body bytes and sets Content-Type to application/json.
// Retry semantics match doRequest (retries on 429 with Retry-After support).
func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, body []byte) ([]byte, error) {
const maxRetryAfter = 120 * time.Second
defaultBackoff := []time.Duration{1 * time.Second, 2 * time.Second}
var backoff []time.Duration
if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 {
backoff = make([]time.Duration, len(c.retryBackoff))
copy(backoff, c.retryBackoff)
} else {
backoff = make([]time.Duration, len(defaultBackoff))
copy(backoff, defaultBackoff)
func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, reqBody []byte) ([]byte, error) {
var opts requestOptions
if reqBody != nil {
opts.bodyFn = func() io.Reader { return bytes.NewReader(reqBody) }
opts.extraHeaders = map[string]string{"Content-Type": "application/json"}
}
sonnet-review-bot commented 2026-05-13 10:07:08 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The doRequestWithBody function only sets Content-Type: application/json when reqBody != nil. For the nil-body DELETE case (used by DeleteReview), no Content-Type is set, which is correct. However, the function's doc comment says "sets Content-Type to application/json" without qualifying the nil case. Minor doc improvement: "sets Content-Type to application/json when body is non-nil".

**[NIT]** The `doRequestWithBody` function only sets `Content-Type: application/json` when `reqBody != nil`. For the nil-body DELETE case (used by DeleteReview), no Content-Type is set, which is correct. However, the function's doc comment says "sets Content-Type to application/json" without qualifying the nil case. Minor doc improvement: "sets Content-Type to application/json when body is non-nil".
const maxErrorBodyBytes = 4 * 1024
if c.token != "" && !c.allowInsecureHTTP {
parsed, err := url.Parse(reqURL)
if err != nil {
return nil, fmt.Errorf("parse request URL: %w", err)
}
if !strings.EqualFold(parsed.Scheme, "https") {
return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL)
}
}
var lastErr error
for attempt := 0; attempt < maxRetryAttempts; attempt++ {
if attempt > 0 {
var delay time.Duration
if attempt-1 < len(backoff) {
delay = backoff[attempt-1]
}
if delay > 0 {
timer := time.NewTimer(delay)
select {
case <-timer.C:
timer.Stop()
case <-ctx.Done():
timer.Stop()
return nil, ctx.Err()
}
}
}
var bodyReader io.Reader
if body != nil {
bodyReader = bytes.NewReader(body)
}
req, err := http.NewRequestWithContext(ctx, method, reqURL, bodyReader)
if err != nil {
return nil, fmt.Errorf("create request: %w", err)
}
if c.token != "" {
req.Header.Set("Authorization", "Bearer "+c.token)
}
req.Header.Set("User-Agent", userAgent)
req.Header.Set("Accept", "application/vnd.github+json")
if body != nil {
req.Header.Set("Content-Type", "application/json")
}
resp, err := c.httpClient.Do(req)
if err != nil {
return nil, fmt.Errorf("do request: %w", err)
}
respStatus := resp.StatusCode
retryAfterHeader := resp.Header.Get("Retry-After")
respBody, done, handleErr := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
if done {
return respBody, handleErr
}
lastErr = handleErr
if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 {
if ra := retryAfterHeader; ra != "" {
if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 {
delay := time.Duration(seconds) * time.Second
if delay > maxRetryAfter {
delay = maxRetryAfter
}
if attempt < len(backoff) {
backoff[attempt] = delay
}
} else if retryAt, err := http.ParseTime(ra); err == nil {
delay := time.Until(retryAt)
if delay < 0 {
delay = 0
}
if delay > maxRetryAfter {
delay = maxRetryAfter
}
if attempt < len(backoff) {
backoff[attempt] = delay
}
}
}
continue
}
return nil, lastErr
}
return nil, lastErr
return c.doRequestCore(ctx, method, reqURL, opts)
}
1
github/review.go
+8 -8
View File
@@ -5,6 +5,7 @@ import (
"encoding/json"
"errors"
"fmt"
"net/http"
"net/url"
"gitea.weiker.me/rodin/review-bot/vcs"
@@ -52,15 +53,13 @@ type dismissReviewRequest struct {
// canonical vcs.Review.State value.
func translateGitHubReviewState(state string) string {
switch state {
case "APPROVED":
return "APPROVED"
case "CHANGES_REQUESTED":
sonnet-review-bot commented 2026-05-13 07:37:01 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The comment on translateGitHubReviewState says 'States like APPROVED, DISMISSED, and PENDING pass through unchanged as they already match the canonical vcs representation.' — but PENDING is also a GitHub state that maps to itself. Worth noting whether PENDING is expected to appear in responses from ListReviews, and if vcs.Review.State accepts it. This is a documentation clarity issue, not a bug.

**[NIT]** The comment on translateGitHubReviewState says 'States like APPROVED, DISMISSED, and PENDING pass through unchanged as they already match the canonical vcs representation.' — but PENDING is also a GitHub state that maps to itself. Worth noting whether PENDING is expected to appear in responses from ListReviews, and if vcs.Review.State accepts it. This is a documentation clarity issue, not a bug.
return "REQUEST_CHANGES"
sonnet-review-bot commented 2026-05-13 07:55:21 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The dismissReviewRequest.Event field is always hardcoded to "DISMISS" and never varies. Since it's an implementation detail of the GitHub API (not exposed to callers), the struct could be simplified to a single-field struct, or the Event field removed and the JSON marshaling handled inline. This is very minor — the current approach is clear and extensible.

**[NIT]** The `dismissReviewRequest.Event` field is always hardcoded to "DISMISS" and never varies. Since it's an implementation detail of the GitHub API (not exposed to callers), the struct could be simplified to a single-field struct, or the Event field removed and the JSON marshaling handled inline. This is very minor — the current approach is clear and extensible.
case "COMMENTED":
return "COMMENT"
case "DISMISSED":
return "DISMISSED"
default:
// States like APPROVED, DISMISSED, and PENDING pass through unchanged
// as they already match the canonical vcs representation.
sonnet-review-bot commented 2026-05-13 07:26:59 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] translateGitHubReviewState maps "APPROVED" → "APPROVED" and "DISMISSED" → "DISMISSED" explicitly, which is identical to the default passthrough case. These two cases can be removed, leaving only the two non-trivial translations (CHANGES_REQUESTED and COMMENTED). This simplifies the function and makes the intent clearer: only translate states that differ from their canonical representation.

**[MINOR]** translateGitHubReviewState maps `"APPROVED"` → `"APPROVED"` and `"DISMISSED"` → `"DISMISSED"` explicitly, which is identical to the default passthrough case. These two cases can be removed, leaving only the two non-trivial translations (CHANGES_REQUESTED and COMMENTED). This simplifies the function and makes the intent clearer: only translate states that differ from their canonical representation.
return state
}
}
3
 @@ -100,7 +99,7 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
return nil, fmt.Errorf("marshal review request: %w", err)
}
body, err := c.doRequestWithBody(ctx, "POST", reqURL, data)
body, err := c.doRequestWithBody(ctx, http.MethodPost, reqURL, data)
gpt-review-bot commented 2026-05-13 07:38:51 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Returning ErrCannotDeleteSubmittedReview directly drops operation context. Consider wrapping the sentinel with context (e.g., fmt.Errorf("delete review: %w", ErrCannotDeleteSubmittedReview)) so callers get both the sentinel identity and helpful context, aligning with the documented wrapping pattern.

**[MINOR]** Returning ErrCannotDeleteSubmittedReview directly drops operation context. Consider wrapping the sentinel with context (e.g., fmt.Errorf("delete review: %w", ErrCannotDeleteSubmittedReview)) so callers get both the sentinel identity and helpful context, aligning with the documented wrapping pattern.
if err != nil {
return nil, fmt.Errorf("post review: %w", err)
}
2
 @@ -150,12 +149,13 @@ func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int
// DeleteReview deletes a pull request review.
gpt-review-bot commented 2026-05-13 07:28:05 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] DismissReview payload includes Event: "DISMISS". GitHub's dismissals endpoint typically requires a message; verify that including the event field is supported by the API. If unnecessary, removing it reduces risk of server-side validation issues.

**[MINOR]** DismissReview payload includes Event: "DISMISS". GitHub's dismissals endpoint typically requires a message; verify that including the event field is supported by the API. If unnecessary, removing it reduces risk of server-side validation issues.
// Only PENDING reviews can be deleted; attempting to delete a submitted review
// (APPROVED, CHANGES_REQUESTED, COMMENTED) returns ErrCannotDeleteSubmittedReview.
// (APPROVED, CHANGES_REQUESTED, or COMMENTED per GitHub API naming) returns
// ErrCannotDeleteSubmittedReview.
func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
_, err := c.doRequestWithBody(ctx, "DELETE", reqURL, nil)
_, err := c.doRequestWithBody(ctx, http.MethodDelete, reqURL, nil)
if err != nil {
var apiErr *APIError
if errors.As(err, &apiErr) && apiErr.StatusCode == 422 {
5
 @@ -183,7 +183,7 @@ func (c *Client) DismissReview(ctx context.Context, owner, repo string, number i
return fmt.Errorf("marshal dismiss request: %w", err)
}
_, err = c.doRequestWithBody(ctx, "PUT", reqURL, data)
_, err = c.doRequestWithBody(ctx, http.MethodPut, reqURL, data)
if err != nil {
return fmt.Errorf("dismiss review: %w", err)
}
github/review_test.go
+2 -13
View File
1
 @@ -7,6 +7,7 @@ import (
"io"
"net/http"
"net/http/httptest"
"strings"
"testing"
"time"
sonnet-review-bot commented 2026-05-13 07:55:21 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The table-driven test for TestTranslateGitHubReviewState uses field name expected instead of the idiomatic want used by the rest of the test suite. Minor inconsistency with the project's established naming convention.

**[MINOR]** The table-driven test for TestTranslateGitHubReviewState uses field name `expected` instead of the idiomatic `want` used by the rest of the test suite. Minor inconsistency with the project's established naming convention.
5
 @@ -153,7 +154,7 @@ func TestPostReview_MalformedResponse(t *testing.T) {
if err == nil {
t.Fatal("expected error for malformed response")
}
if !containsStr(err.Error(), "parse review response") {
if !strings.Contains(err.Error(), "parse review response") {
t.Errorf("expected parse error, got: %v", err)
}
}
2
 @@ -379,16 +380,4 @@ func TestTranslateGitHubReviewState(t *testing.T) {
}
sonnet-review-bot commented 2026-05-13 07:26:59 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] containsStr and containsSubstring reimplement strings.Contains from the standard library. The repository conventions do not restrict use of the standard library; strings.Contains(s, substr) should be used directly. The current implementation is also slightly incorrect: containsStr returns true when s == substr even without going through containsSubstring, but the logic mixing len checks and || chains is harder to read and maintain than a direct strings.Contains call.

**[MINOR]** containsStr and containsSubstring reimplement strings.Contains from the standard library. The repository conventions do not restrict use of the standard library; `strings.Contains(s, substr)` should be used directly. The current implementation is also slightly incorrect: `containsStr` returns true when `s == substr` even without going through `containsSubstring`, but the logic mixing `len` checks and `||` chains is harder to read and maintain than a direct `strings.Contains` call.
}
sonnet-review-bot commented 2026-05-13 07:37:01 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] There are two trailing blank lines at the end of review_test.go. gofmt typically produces a single trailing newline; this is minor but inconsistent with the style pattern.

**[NIT]** There are two trailing blank lines at the end of review_test.go. gofmt typically produces a single trailing newline; this is minor but inconsistent with the style pattern.
// containsStr is a test helper for checking error messages.
func containsStr(s, substr string) bool {
return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsSubstring(s, substr))
}
func containsSubstring(s, sub string) bool {
for i := 0; i <= len(s)-len(sub); i++ {
if s[i:i+len(sub)] == sub {
return true
}
}
return false
}
11