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

feat(github): implement PRReader + FileReader client (#80) #93

Closed
rodin wants to merge 16 commits from review-bot-issue-80 into feature/github-support
pull from: review-bot-issue-80
merge into: rodin:feature/github-support
Conversation 78 Commits 16 Files Changed 8
+28 -18
1 changed files with 28 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit b380e7fcae - Show all commits
github/client.go
+28 -18
View File
95
 @@ -255,25 +255,11 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
return nil, fmt.Errorf("do request: %w", err) return nil, fmt.Errorf("do request: %w", err)
} }
if resp.StatusCode >= 200 && resp.StatusCode < 300 { body, done, err := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes)
body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBytes+1)) if done {
sonnet-review-bot commented 2026-05-13 00:14:07 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The int64 cast in if int64(len(body)) >= maxResponseBytes is unnecessary since maxResponseBytes is an untyped constant and len() returns int. Both sides of the comparison are int-typed. The cast is harmless but adds noise.

**[MINOR]** The `int64` cast in `if int64(len(body)) >= maxResponseBytes` is unnecessary since `maxResponseBytes` is an untyped constant and `len()` returns `int`. Both sides of the comparison are `int`-typed. The cast is harmless but adds noise.
resp.Body.Close() return body, err
if err != nil {
return nil, fmt.Errorf("read response body: %w", err)
}
if len(body) > maxResponseBytes {
return nil, fmt.Errorf("response body exceeded %d bytes (truncated)", maxResponseBytes)
}
return body, nil
} }
lastErr = err
errBody, readErr := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes))
if readErr != nil && len(errBody) == 0 {
errBody = []byte(fmt.Sprintf("[error reading response body: %v]", readErr))
}
resp.Body.Close()
lastErr = &APIError{StatusCode: resp.StatusCode, Body: string(errBody)}
sonnet-review-bot commented 2026-05-13 03:48:59 +00:00
Review
Copy Link
Copy Source

[NIT] In doRequest, after the handleResponse call, there's a check if resp.StatusCode == http.StatusTooManyRequests but resp could theoretically have been closed by handleResponse. The body is closed, but the resp.StatusCode field is still accessible on the struct. This is correct and safe in Go's net/http — just worth being aware of.

**[NIT]** In `doRequest`, after the `handleResponse` call, there's a check `if resp.StatusCode == http.StatusTooManyRequests` but `resp` could theoretically have been closed by `handleResponse`. The body is closed, but the `resp.StatusCode` field is still accessible on the struct. This is correct and safe in Go's net/http — just worth being aware of.
// Retry on 429 rate limit // Retry on 429 rate limit
if resp.StatusCode == http.StatusTooManyRequests && attempt < maxAttempts-1 { if resp.StatusCode == http.StatusTooManyRequests && attempt < maxAttempts-1 {
@@ -311,6 +297,30 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
return nil, lastErr return nil, lastErr
} }
// handleResponse reads and closes the response body, returning the result.
// 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.
func (c *Client) handleResponse(resp *http.Response, maxRespBytes int, maxErrBytes int) ([]byte, bool, error) {
defer resp.Body.Close()
if resp.StatusCode >= 200 && resp.StatusCode < 300 {
body, err := io.ReadAll(io.LimitReader(resp.Body, int64(maxRespBytes)+1))
if err != nil {
return nil, true, fmt.Errorf("read response body: %w", err)
}
if len(body) > maxRespBytes {
return nil, true, fmt.Errorf("response body exceeded %d bytes (truncated)", maxRespBytes)
}
return body, true, nil
}
errBody, readErr := io.ReadAll(io.LimitReader(resp.Body, int64(maxErrBytes)))
if readErr != nil && len(errBody) == 0 {
errBody = []byte(fmt.Sprintf("[error reading response body: %v]", readErr))
}
return nil, false, &APIError{StatusCode: resp.StatusCode, Body: string(errBody)}
}
// doGet is a convenience wrapper for GET requests with the default Accept header. // doGet is a convenience wrapper for GET requests with the default Accept header.
func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) { func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
return c.doRequest(ctx, http.MethodGet, reqURL, "") return c.doRequest(ctx, http.MethodGet, reqURL, "")