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

feat(cmd): wire --provider and --base-url flags into CLI (Phase 5) #106

Merged
aweiker merged 17 commits from review-bot-issue-82 into feature/github-support 2026-05-13 17:16:28 +00:00
Conversation 98 Commits 17 Files Changed 14
+14 -212
4 changed files with 14 additions and 212 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit d40902771e - Show all commits
cmd/review-bot/main.go
-16
View File
81
 @@ -892,22 +892,6 @@ func extractSentinelName(body string) string {
return name
}
sonnet-review-bot commented 2026-05-13 13:49:34 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] Extra blank line between the closing brace of extractSentinelName and the comment for findAllOwnReviews — minor, but inconsistent with the rest of the file's style.

**[NIT]** Extra blank line between the closing brace of `extractSentinelName` and the comment for `findAllOwnReviews` — minor, but inconsistent with the rest of the file's style.
// findOwnReview locates the most recent non-superseded review matching the sentinel.
func findOwnReview(reviews []vcs.Review, sentinel string) *vcs.Review {
var best *vcs.Review
for i := range reviews {
if !strings.Contains(reviews[i].Body, sentinel) {
continue
}
if strings.Contains(reviews[i].Body, "~~Original review~~") {
continue
}
if best == nil || reviews[i].ID > best.ID {
best = &reviews[i]
}
}
return best
}
// findAllOwnReviews returns all non-superseded reviews matching the sentinel.
func findAllOwnReviews(reviews []vcs.Review, sentinel string) []vcs.Review {
1
cmd/review-bot/main_test.go
-85
View File
@@ -210,91 +210,6 @@ func TestBuildSupersededBodyShortSHA(t *testing.T) {
}
}
func TestFindOwnReview(t *testing.T) {
tests := []struct {
name string
reviews []vcs.Review
sentinel string
wantID int64
wantNil bool
}{
{
name: "no reviews",
reviews: nil,
sentinel: "<!-- review-bot:sonnet -->",
wantNil: true,
},
{
name: "found by sentinel",
reviews: []vcs.Review{
makeReview(42, "bot", "APPROVED", false, "review body\n<!-- review-bot:sonnet -->"),
},
sentinel: "<!-- review-bot:sonnet -->",
wantID: 42,
},
{
name: "wrong sentinel",
reviews: []vcs.Review{
makeReview(42, "bot", "APPROVED", false, "body\n<!-- review-bot:gpt -->"),
},
sentinel: "<!-- review-bot:sonnet -->",
wantNil: true,
},
{
name: "multiple reviews, returns first match",
reviews: []vcs.Review{
makeReview(10, "bot", "APPROVED", false, "old\n<!-- review-bot:gpt -->"),
makeReview(20, "bot", "APPROVED", false, "new\n<!-- review-bot:sonnet -->"),
},
sentinel: "<!-- review-bot:sonnet -->",
wantID: 20,
},
{
name: "skips superseded review",
reviews: []vcs.Review{
makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n**Superseded**\n<!-- review-bot:sonnet -->"),
makeReview(20, "bot", "APPROVED", false, "fresh review\n<!-- review-bot:sonnet -->"),
},
sentinel: "<!-- review-bot:sonnet -->",
wantID: 20,
},
{
name: "only superseded reviews exist",
reviews: []vcs.Review{
makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n<!-- review-bot:sonnet -->"),
},
sentinel: "<!-- review-bot:sonnet -->",
wantNil: true,
},
{
name: "picks highest ID among matches",
reviews: []vcs.Review{
makeReview(50, "bot", "APPROVED", false, "v1\n<!-- review-bot:sonnet -->"),
makeReview(30, "bot", "APPROVED", false, "v0\n<!-- review-bot:sonnet -->"),
},
sentinel: "<!-- review-bot:sonnet -->",
wantID: 50,
},
}
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)
}
}
})
}
}
func TestHasSharedToken(t *testing.T) {
tests := []struct {
2
github/client.go
+8 -109
View File
1
 @@ -5,8 +5,8 @@ package github
import (
"bytes"
"encoding/json"
"context"
"encoding/json"
"errors"
"fmt"
"io"
@@ -215,6 +215,11 @@ type requestOptions struct {
func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts requestOptions) ([]byte, error) {
const maxRetryAfter = 120 * time.Second
// maxErrorBodyBytes limits how much of an error response body is stored.
// Kept small (4 KiB) to reduce the risk of sensitive data leakage if callers
// log APIError.Body directly. Error() further truncates to 200 bytes.
const maxErrorBodyBytes = 4 * 1024
// backoff holds per-attempt delays: backoff[i] is the delay before attempt i+1.
// Length must be maxRetryAttempts-1 (one entry per retry gap).
// SetRetryBackoff validates at configuration time; the default is always valid.
@@ -228,11 +233,6 @@ func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts
copy(backoff, defaultBackoff)
}
// maxErrorBodyBytes limits how much of an error response body is stored.
// Kept small (4 KiB) to reduce the risk of sensitive data leakage if callers
// log APIError.Body directly. Error() further truncates to 200 bytes.
const maxErrorBodyBytes = 4 * 1024
// Reject non-HTTPS URLs early since the URL is immutable across retries.
if c.token != "" && !c.allowInsecureHTTP {
parsed, err := url.Parse(reqURL)
1
 @@ -387,114 +387,13 @@ func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, r
gpt-review-bot commented 2026-05-13 13:51:45 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] Minor formatting: there is an extra blank line between the closing brace of doRequestWithBody and the start of doJSONRequest that could be removed for consistency.

**[NIT]** Minor formatting: there is an extra blank line between the closing brace of doRequestWithBody and the start of doJSONRequest that could be removed for consistency.
sonnet-review-bot commented 2026-05-13 14:15:32 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] There is a spurious blank line before the closing brace of doRequestWithBody (after return c.doRequestCore(...)). Minor formatting issue that gofmt would not flag since it's inside a function body, but it's visually inconsistent.

**[NIT]** There is a spurious blank line before the closing brace of `doRequestWithBody` (after `return c.doRequestCore(...)`). Minor formatting issue that gofmt would not flag since it's inside a function body, but it's visually inconsistent.
}
sonnet-review-bot commented 2026-05-13 15:51:02 +00:00
Review
Copy Link
Copy Source

[NIT] doJSONRequest is added but not yet called by any public methods in the diff (the github package has no PostReview/DismissReview/etc. visible in the diff). This is fine for an incremental PR, but it means the new helper is untested at the integration level. The unit tests for doJSONRequest itself are present and cover the retry path.

**[NIT]** doJSONRequest is added but not yet called by any public methods in the diff (the github package has no PostReview/DismissReview/etc. visible in the diff). This is fine for an incremental PR, but it means the new helper is untested at the integration level. The unit tests for doJSONRequest itself are present and cover the retry path.
// doJSONRequest performs an HTTP request with a JSON body and returns the response body.
// It handles HTTPS validation, authentication, response reading, and retries on HTTP 429
// rate limit responses (matching the retry behavior of doRequest).
// It delegates retry/backoff/429 handling to doRequestWithBody.
// This is a general-purpose helper used by any method that needs to send JSON payloads
sonnet-review-bot commented 2026-05-13 10:30:42 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] doJSONRequest does not retry on HTTP 429 like doRequest does. Since PostReview and DismissReview are write operations that can be rate-limited by GitHub, missing retry logic here could cause failures in high-traffic repos. Consider either adding retry logic or documenting that write operations are not retried.

**[MINOR]** `doJSONRequest` does not retry on HTTP 429 like `doRequest` does. Since `PostReview` and `DismissReview` are write operations that can be rate-limited by GitHub, missing retry logic here could cause failures in high-traffic repos. Consider either adding retry logic or documenting that write operations are not retried.
// (e.g. PostReview, DismissReview).
func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, payload any) ([]byte, error) {
sonnet-review-bot commented 2026-05-13 10:40:59 +00:00
Outdated
Review
Copy Link
Copy Source

[MAJOR] doJSONRequest duplicates ~100 lines of retry logic that already exists in doRequest. Both functions implement identical 429-retry-with-backoff loops, HTTPS validation, and Retry-After header parsing. This violates DRY and doubles the maintenance burden — a future change to retry semantics must be applied in two places. The preferred approach would be to make doRequest accept an optional body (io.Reader) or to extract the shared retry scaffolding into a single private helper that both GET and write operations call.

**[MAJOR]** doJSONRequest duplicates ~100 lines of retry logic that already exists in doRequest. Both functions implement identical 429-retry-with-backoff loops, HTTPS validation, and Retry-After header parsing. This violates DRY and doubles the maintenance burden — a future change to retry semantics must be applied in two places. The preferred approach would be to make doRequest accept an optional body (io.Reader) or to extract the shared retry scaffolding into a single private helper that both GET and write operations call.
const (
maxErrorBodyBytes = 4 * 1024
maxRetryAfter = 120 * time.Second
)
jsonBody, err := json.Marshal(payload)
if err != nil {
sonnet-review-bot commented 2026-05-13 16:34:41 +00:00
Review
Copy Link
Copy Source

[MINOR] doJSONRequest is added to client.go but is only used internally by review.go methods. Placing transport-layer helpers (doRequestCore, doRequestWithBody) together with a higher-level JSON serialisation helper makes the file slightly inconsistent. This is a minor organisation concern — a comment grouping the helpers or moving doJSONRequest to review.go would improve cohesion, but the current placement is not wrong.

**[MINOR]** `doJSONRequest` is added to `client.go` but is only used internally by `review.go` methods. Placing transport-layer helpers (`doRequestCore`, `doRequestWithBody`) together with a higher-level JSON serialisation helper makes the file slightly inconsistent. This is a minor organisation concern — a comment grouping the helpers or moving `doJSONRequest` to `review.go` would improve cohesion, but the current placement is not wrong.
return nil, fmt.Errorf("marshal request body: %w", err)
}
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)
}
}
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)
}
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()
}
}
}
req, err := http.NewRequestWithContext(ctx, method, reqURL, bytes.NewReader(jsonBody))
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")
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")
body, done, handleErr := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
if done {
return body, handleErr
}
lastErr = handleErr
// Retry on 429 rate limit
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.doRequestWithBody(ctx, method, reqURL, jsonBody)
}
github/client_test.go
+6 -2
View File
@@ -609,7 +609,9 @@ func TestDoJSONRequest_429Retry(t *testing.T) {
defer ts.Close()
c := NewClient("token", ts.URL, AllowInsecureHTTP())
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
sonnet-review-bot commented 2026-05-13 10:40:59 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] c.SetRetryBackoff(...) return value is not checked in TestDoJSONRequest_429Retry. The call on line 613 ignores the error. All other tests in the file use if err := c.SetRetryBackoff(...); err != nil { t.Fatalf(...) }. Should be consistent.

**[MINOR]** `c.SetRetryBackoff(...)` return value is not checked in `TestDoJSONRequest_429Retry`. The call on line 613 ignores the error. All other tests in the file use `if err := c.SetRetryBackoff(...); err != nil { t.Fatalf(...) }`. Should be consistent.
}
body, err := c.doJSONRequest(context.Background(), http.MethodPost, ts.URL+"/test", map[string]string{"key": "val"})
if err != nil {
@@ -631,7 +633,9 @@ func TestDoJSONRequest_429ExhaustsRetries(t *testing.T) {
defer ts.Close()
c := NewClient("token", ts.URL, AllowInsecureHTTP())
c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond})
if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil {
t.Fatalf("SetRetryBackoff: %v", err)
}
_, err := c.doJSONRequest(context.Background(), http.MethodPost, ts.URL+"/test", map[string]string{"key": "val"})
if err == nil {