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
+9 -8
3 changed files with 9 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 5b2fa0b9af - Show all commits
github/client.go
+3 -3
View File
79
 @@ -199,7 +199,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
timer := time.NewTimer(delay)
sonnet-review-bot commented 2026-05-13 02:30:09 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The doRequest method has accept string as the last parameter. Per Go idiom (style.md receiver naming / api-conventions), named parameters for Accept headers are fine, but the parameter name accept conflicts with the common Go pattern of using accept only in request context. Not a correctness issue — purely style.

**[NIT]** The `doRequest` method has `accept string` as the last parameter. Per Go idiom (style.md receiver naming / api-conventions), named parameters for Accept headers are fine, but the parameter name `accept` conflicts with the common Go pattern of using `accept` only in request context. Not a correctness issue — purely style.
select {
case <-timer.C:
// Timer already fired; Stop() is a no-op here.
// Backoff elapsed, proceed with retry.
case <-ctx.Done():
timer.Stop()
return nil, ctx.Err()
15
 @@ -259,8 +259,8 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
if attempt < len(backoff) {
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.
backoff[attempt] = delay
}
} else if t, err := http.ParseTime(ra); err == nil {
delay := time.Until(t)
} else if retryAt, err := http.ParseTime(ra); err == nil {
delay := time.Until(retryAt)
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.
if delay < 0 {
delay = 0
}
github/files.go
+2
View File
12
 @@ -48,6 +48,8 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]
if err2 := json.Unmarshal(body, &single); err2 != nil {
return nil, fmt.Errorf("parse contents JSON: %w", err2)
}
security-review-bot marked this conversation as resolved Outdated
security-review-bot commented 2026-05-12 22:22:13 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] escapePath preserves dot-segments ("." and "..") in path components. Some servers normalize dot-segments, which could let an untrusted path escape the intended "contents" endpoint (e.g., "/contents/../../pulls"). While this stays on the same host, it could lead to unexpected behavior. Consider rejecting or encoding dot-segments to prevent path traversal in URL construction.

**[MINOR]** escapePath preserves dot-segments ("." and "..") in path components. Some servers normalize dot-segments, which could let an untrusted path escape the intended "contents" endpoint (e.g., "/contents/../../pulls"). While this stays on the same host, it could lead to unexpected behavior. Consider rejecting or encoding dot-segments to prevent path traversal in URL construction.
// Guard against empty objects ({}) or unexpected shapes that
sonnet-review-bot commented 2026-05-13 01:17:39 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Silent empty-array handling: when the GitHub Contents API returns an empty JSON array [], json.Unmarshal into []entry succeeds with len(entries) == 0 and the function returns an empty slice without error. This is probably the correct behavior for an empty directory, but it's undocumented and could silently mask an unexpected server response. A comment clarifying this intent would be valuable.

**[MINOR]** Silent empty-array handling: when the GitHub Contents API returns an empty JSON array `[]`, `json.Unmarshal` into `[]entry` succeeds with `len(entries) == 0` and the function returns an empty slice without error. This is probably the correct behavior for an empty directory, but it's undocumented and could silently mask an unexpected server response. A comment clarifying this intent would be valuable.
// unmarshal successfully but carry no useful data.
if single.Name == "" && single.Path == "" && single.Type == "" {
return nil, fmt.Errorf("parse contents JSON: unexpected response format")
}
12
github/pr.go
+4 -5
View File
23
 @@ -194,7 +194,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
for _, cr := range checkResp.CheckRuns {
result = append(result, vcs.CommitStatus{
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.
Context: cr.Name,
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.
Status: mapCheckRunStatus(cr.Conclusion, cr.Status),
Status: mapCheckRunStatus(cr.Conclusion),
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.
Description: derefString(cr.Conclusion),
TargetURL: cr.HTMLURL,
})
3
 @@ -208,10 +208,9 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
}
// mapCheckRunStatus maps a check run conclusion to a vcs.CommitStatus status string.
sonnet-review-bot commented 2026-05-12 22:17:41 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] In mapCheckRunStatus, the cases "in_progress" and "queued" are in the switch *conclusion block, but conclusion is non-nil when this switch is reached. GitHub's API sets conclusion to null for in-progress/queued check runs, not to the strings "in_progress" or "queued" — those are status field values. So these cases are dead code and will never be reached. The nil-conclusion check at the top already handles the pending case correctly.

**[MINOR]** In `mapCheckRunStatus`, the cases `"in_progress"` and `"queued"` are in the `switch *conclusion` block, but `conclusion` is non-nil when this switch is reached. GitHub's API sets `conclusion` to null for in-progress/queued check runs, not to the strings `"in_progress"` or `"queued"` — those are `status` field values. So these cases are dead code and will never be reached. The nil-conclusion check at the top already handles the pending case correctly.
// The second parameter (check run status field, e.g. "completed", "in_progress") is
// unused because conclusion alone determines the mapped state: nil conclusion means
// the run is still in progress (pending), regardless of the status field value.
func mapCheckRunStatus(conclusion *string, _ string) string {
// Conclusion alone determines the mapped state: nil conclusion means the run is
// still in progress (pending), regardless of the status field value.
func mapCheckRunStatus(conclusion *string) string {
if conclusion == nil {
// Still running or queued
return "pending"