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
+6 -3
3 changed files with 6 additions and 3 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 30798ff023 - Show all commits
github/client.go
+1 -1
View File
86
 @@ -225,7 +225,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
timer := time.NewTimer(delay)
select {
sonnet-review-bot commented 2026-05-13 00:14:07 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The timer leak on the happy path is benign but slightly untidy. When <-timer.C fires, timer.Stop() returns false and the channel is already drained, so it's a no-op — the comment acknowledges this. Consider using timer.Reset pattern or noting it more explicitly, but this is purely cosmetic.

**[MINOR]** The timer leak on the happy path is benign but slightly untidy. When `<-timer.C` fires, `timer.Stop()` returns false and the channel is already drained, so it's a no-op — the comment acknowledges this. Consider using `timer.Reset` pattern or noting it more explicitly, but this is purely cosmetic.
case <-timer.C:
timer.Stop() // no-op after fire, releases runtime resources promptly
timer.Stop() // no-op after fire; kept for symmetry with the ctx.Done case
sonnet-review-bot commented 2026-05-13 01:42:53 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The response body limit check uses >= instead of >. io.LimitReader reads at most maxResponseBytes bytes, so len(body) == maxResponseBytes could mean the response is exactly 10 MiB (not truncated) or it was truncated at the limit. Using > is impossible here since LimitReader caps at the limit. The check should be == maxResponseBytes to be semantically correct, or the limit should be maxResponseBytes+1 to distinguish exactly-limit from truncated. The current code will error on exactly 10 MiB legitimate responses.

**[MINOR]** The response body limit check uses `>=` instead of `>`. `io.LimitReader` reads at most `maxResponseBytes` bytes, so `len(body) == maxResponseBytes` could mean the response is exactly 10 MiB (not truncated) or it was truncated at the limit. Using `>` is impossible here since `LimitReader` caps at the limit. The check should be `== maxResponseBytes` to be semantically correct, or the limit should be `maxResponseBytes+1` to distinguish exactly-limit from truncated. The current code will error on exactly 10 MiB legitimate responses.
case <-ctx.Done():
timer.Stop()
sonnet-review-bot commented 2026-05-13 02:30:09 +00:00
Review
Copy Link
Copy Source

[MINOR] After resp.Body.Close() is called for a successful response, if io.ReadAll returns an error, the body is already closed which is correct, but the success path reads up to maxResponseBytes+1 bytes via io.LimitReader. This is the intentional over-read trick to detect exceeding the limit, which is correct. However, on the error path for non-2xx responses (line 245), errBody, _ := io.ReadAll(...) silently discards the read error. While acceptable in practice since it's for error body capture, the convention in this codebase is to check all error returns. This is minor since the worst case is an empty error body.

**[MINOR]** After `resp.Body.Close()` is called for a successful response, if `io.ReadAll` returns an error, the body is already closed which is correct, but the success path reads up to `maxResponseBytes+1` bytes via `io.LimitReader`. This is the intentional over-read trick to detect exceeding the limit, which is correct. However, on the error path for non-2xx responses (line 245), `errBody, _ := io.ReadAll(...)` silently discards the read error. While acceptable in practice since it's for error body capture, the convention in this codebase is to check all error returns. This is minor since the worst case is an empty error body.
return nil, ctx.Err()
8
github/files.go
+1 -1
View File
25
 @@ -81,7 +81,7 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]
if err := json.Unmarshal(body, &entries); err != nil {
var single entry
if err2 := json.Unmarshal(body, &single); err2 != nil {
return nil, fmt.Errorf("parse contents JSON: %w", err2)
return nil, fmt.Errorf("parse contents JSON: as array: %v; as object: %w", err, err2)
}
// Guard against empty objects ({}) or unexpected shapes that
// unmarshal successfully but carry no useful data.
1
github/pr.go
+4 -1
View File
10
 @@ -127,6 +127,9 @@ func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, nu
// Returns nil (not an empty slice) when there are no statuses or check runs.
sonnet-review-bot commented 2026-05-13 02:30:09 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The GetFileContentAtRef method is placed in pr.go but it's also the implementation delegate for GetFileContent in files.go. Conceptually it belongs in files.go alongside the other file-related methods. The split works because Go doesn't care which file a method is in, but for navigability (per the style.md pattern of organizing by concept) moving it to files.go would be cleaner.

**[NIT]** The `GetFileContentAtRef` method is placed in `pr.go` but it's also the implementation delegate for `GetFileContent` in `files.go`. Conceptually it belongs in `files.go` alongside the other file-related methods. The split works because Go doesn't care which file a method is in, but for navigability (per the `style.md` pattern of organizing by concept) moving it to `files.go` would be cleaner.
// If the commit statuses endpoint fails (e.g. 404 for an unknown SHA), the
// function returns immediately without attempting the check-runs endpoint.
// If the check-runs endpoint fails after statuses were fetched successfully,
// the function returns an error (not a partial result) so callers always get
sonnet-review-bot commented 2026-05-13 03:48:59 +00:00
Review
Copy Link
Copy Source

[NIT] The GetPullRequestFiles comment says 'Returns nil (not an empty slice) when the PR has no changed files' but if the first page returns an empty array, allFiles remains nil — this is correct. However the GitHub API actually returns an empty array for PRs with 0 changed files (valid edge case), so the nil vs empty distinction in the doc comment is accurate but subtle. No code change needed.

**[NIT]** The `GetPullRequestFiles` comment says 'Returns nil (not an empty slice) when the PR has no changed files' but if the first page returns an empty array, `allFiles` remains nil — this is correct. However the GitHub API actually returns an empty array for PRs with 0 changed files (valid edge case), so the nil vs empty distinction in the doc comment is accurate but subtle. No code change needed.
// either a complete view or a clear error signal.
func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcs.CommitStatus, error) {
var result []vcs.CommitStatus
11
 @@ -192,7 +195,7 @@ func mapCheckRunStatus(conclusion *string) string {
case "failure", "action_required", "timed_out":
sonnet-review-bot commented 2026-05-13 02:41:43 +00:00
Review
Copy Link
Copy Source

[MINOR] The mapCheckRunStatus mapping treats cancelled, skipped, and neutral as "success" (non-blocking). This is a policy decision that should be documented more prominently — cancelled in particular could reasonably be mapped to failure in some contexts. The comment // non-blocking is brief; a note explaining the design rationale (e.g. 'these do not indicate a blocking failure per GitHub's check suite semantics') would help future maintainers.

**[MINOR]** The `mapCheckRunStatus` mapping treats `cancelled`, `skipped`, and `neutral` as `"success"` (non-blocking). This is a policy decision that should be documented more prominently — `cancelled` in particular could reasonably be mapped to `failure` in some contexts. The comment `// non-blocking` is brief; a note explaining the design rationale (e.g. 'these do not indicate a blocking failure per GitHub's check suite semantics') would help future maintainers.
return "failure"
sonnet-review-bot commented 2026-05-12 23:26:36 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The mapCheckRunStatus function signature takes an ignored second parameter _ string. The comment explains why, which is good. A minor alternative would be to not accept the parameter at all and have the caller omit it, but given this is a private helper called from one place with both values available, the current approach is defensible. The doc comment adequately explains the design choice.

**[NIT]** The `mapCheckRunStatus` function signature takes an ignored second parameter `_ string`. The comment explains why, which is good. A minor alternative would be to not accept the parameter at all and have the caller omit it, but given this is a private helper called from one place with both values available, the current approach is defensible. The doc comment adequately explains the design choice.
sonnet-review-bot commented 2026-05-12 23:49:43 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The mapCheckRunStatus function signature func mapCheckRunStatus(conclusion *string, _ string) string uses a blank identifier for the status parameter. While the comment explains why, a cleaner approach given the parameter is truly unused would be to just not include it and update the call site — or if the signature must stay for extensibility, at minimum name it (e.g., checkStatus string) so readers can understand what's being ignored. The blank _ with no name makes the call site mapCheckRunStatus(cr.Conclusion, cr.Status) harder to understand without reading the comment.

**[MINOR]** The `mapCheckRunStatus` function signature `func mapCheckRunStatus(conclusion *string, _ string) string` uses a blank identifier for the `status` parameter. While the comment explains why, a cleaner approach given the parameter is truly unused would be to just not include it and update the call site — or if the signature must stay for extensibility, at minimum name it (e.g., `checkStatus string`) so readers can understand what's being ignored. The blank `_` with no name makes the call site `mapCheckRunStatus(cr.Conclusion, cr.Status)` harder to understand without reading the comment.
case "cancelled", "skipped", "neutral":
sonnet-review-bot commented 2026-05-12 23:01:09 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] mapCheckRunStatus ignores the second parameter (status string) entirely — it's named _. The function signature accepts the status for potential future use, but the unused parameter with a blank name could be confusing. If the status field (e.g. 'in_progress', 'queued') is never needed, the parameter should be removed from the signature; if it may be needed, a comment explaining why it's currently unused would help. The current behavior maps nil conclusion to 'pending' regardless of status, which may be correct, but 'stale' and 'waiting' conclusions (which GitHub also uses) would silently fall through to 'pending' rather than being explicit.

**[MINOR]** mapCheckRunStatus ignores the second parameter (status string) entirely — it's named `_`. The function signature accepts the status for potential future use, but the unused parameter with a blank name could be confusing. If the `status` field (e.g. 'in_progress', 'queued') is never needed, the parameter should be removed from the signature; if it may be needed, a comment explaining why it's currently unused would help. The current behavior maps nil conclusion to 'pending' regardless of status, which may be correct, but 'stale' and 'waiting' conclusions (which GitHub also uses) would silently fall through to 'pending' rather than being explicit.
return "success" // non-blocking
return "success" // non-blocking: these do not indicate a blocking failure per GitHub check suite semantics
case "stale", "waiting":
return "pending"
default:
4