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

feat(github): implement PRReader interface #102

Merged
aweiker merged 5 commits from issue-80-b-pr-reader into feature/github-support 2026-05-13 05:30:37 +00:00
Conversation 20 Commits 5 Files Changed 5
+64 -8
2 changed files with 64 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit eaccc96073 - Show all commits
github/pr.go
+25 -8
View File
@@ -51,7 +51,10 @@ type checkRunsResponse struct {
} `json:"check_runs"`
}
// GetPullRequest fetches PR metadata.
// GetPullRequest fetches PR metadata from the GitHub API.
sonnet-review-bot commented 2026-05-13 04:22:04 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The doc comment for GetPullRequest is minimal: // GetPullRequest fetches PR metadata. Per the documentation patterns, it should start with the function name and provide slightly more context, e.g. what errors it may return (wraps APIError, IsNotFound applicable).

**[NIT]** The doc comment for `GetPullRequest` is minimal: `// GetPullRequest fetches PR metadata.` Per the documentation patterns, it should start with the function name and provide slightly more context, e.g. what errors it may return (wraps APIError, IsNotFound applicable).
// Returns an *APIError wrapping the HTTP status on non-2xx responses (e.g.
// IsNotFound for 404, IsUnauthorized for 401). Network and context errors
// are wrapped but not typed as *APIError.
func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*vcs.PullRequest, error) {
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
body, err := c.doGet(ctx, reqURL)
@@ -82,9 +85,15 @@ func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, num
return string(body), nil
}
// maxPages is the upper bound on pagination loops to prevent unbounded iteration
// in case the server returns a full page indefinitely.
const maxPages = 100
const (
// maxFilesPages is the upper bound on pagination loops for PR file listing,
// preventing unbounded iteration if the server always returns a full page.
maxFilesPages = 100
// maxCheckRunPages is the upper bound on pagination loops for check-run listing,
// preventing unbounded iteration if the server always returns a full page.
maxCheckRunPages = 100
)
gpt-review-bot commented 2026-05-13 04:23:04 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Pagination in GetPullRequestFiles relies solely on len(files) < 100 to detect the last page. Parsing the Link header (rel="next") would be more robust and avoid extra requests or edge cases where a final page might still contain 100 items.

**[MINOR]** Pagination in GetPullRequestFiles relies solely on len(files) < 100 to detect the last page. Parsing the Link header (rel="next") would be more robust and avoid extra requests or edge cases where a final page might still contain 100 items.
// GetPullRequestFiles fetches the list of files changed in a PR.
// Paginates through all pages (100 per page) to collect all files.
1
 @@ -93,7 +102,7 @@ const maxPages = 100
func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcs.ChangedFile, error) {
var allFiles []vcs.ChangedFile
for page := 1; page <= maxPages; page++ {
for page := 1; page <= maxFilesPages; page++ {
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/files?per_page=100&page=%d",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, page)
sonnet-review-bot commented 2026-05-13 04:48:15 +00:00
Review
Copy Link
Copy Source

[NIT] The maxFilesPages and maxCheckRunPages constants are both 100, meaning the loop can fetch up to 100 pages × 100 items = 10,000 files/check-runs. GitHub's API silently returns at most 3,000 files for large PRs (it caps the list). The constant value is fine, but a comment noting the effective GitHub limit (3,000 files) would help future readers understand why 10,000 is not actually reachable.

**[NIT]** The `maxFilesPages` and `maxCheckRunPages` constants are both 100, meaning the loop can fetch up to 100 pages × 100 items = 10,000 files/check-runs. GitHub's API silently returns at most 3,000 files for large PRs (it caps the list). The constant value is fine, but a comment noting the effective GitHub limit (3,000 files) would help future readers understand why 10,000 is not actually reachable.
body, err := c.doGet(ctx, reqURL)
7
 @@ -154,7 +163,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
}
// Fetch check runs (paginated)
for checkPage := 1; checkPage <= maxPages; checkPage++ {
for checkPage := 1; checkPage <= maxCheckRunPages; checkPage++ {
checkURL := fmt.Sprintf("%s/repos/%s/%s/commits/%s/check-runs?per_page=100&page=%d",
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha), checkPage)
checkBody, err := c.doGet(ctx, checkURL)
@@ -169,7 +178,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
result = append(result, vcs.CommitStatus{
sonnet-review-bot commented 2026-05-13 04:37:47 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The Description field for check run statuses is populated with derefString(cr.Conclusion) — the raw conclusion value (e.g. "success", "failure", "skipped"). This is the same value that's being mapped to Status via mapCheckRunStatus. The doc comment says 'raw conclusion value (e.g. "success", "failure", "skipped")' but for commit statuses, Description is the human-readable description string (e.g. 'Build passed'). Using the conclusion as description is a semantic mismatch: for commit statuses Description carries a narrative message, while for check runs it carries a machine-readable conclusion. Consumers expecting a human-readable description field will get a raw enum string instead. Consider using cr.Status (e.g. 'completed', 'in_progress') or leaving it empty rather than duplicating the conclusion.

**[MINOR]** The `Description` field for check run statuses is populated with `derefString(cr.Conclusion)` — the raw conclusion value (e.g. "success", "failure", "skipped"). This is the same value that's being mapped to `Status` via `mapCheckRunStatus`. The doc comment says 'raw conclusion value (e.g. "success", "failure", "skipped")' but for commit statuses, `Description` is the human-readable description string (e.g. 'Build passed'). Using the conclusion as description is a semantic mismatch: for commit statuses `Description` carries a narrative message, while for check runs it carries a machine-readable conclusion. Consumers expecting a human-readable description field will get a raw enum string instead. Consider using `cr.Status` (e.g. 'completed', 'in_progress') or leaving it empty rather than duplicating the conclusion.
sonnet-review-bot commented 2026-05-13 04:48:15 +00:00
Review
Copy Link
Copy Source

[MINOR] The Description field for check runs is set to derefString(cr.Conclusion) (raw conclusion value like 'success', 'failure'). This means the Description field has a different semantic for check runs vs commit statuses — for statuses it's a human-readable description ('Build passed'), while for check runs it's a machine value ('success'). This is documented in the comment but may surprise callers. Consider using cr.Name or leaving it empty if no textual description is available from the check runs API (the output.summary field would require schema expansion).

**[MINOR]** The `Description` field for check runs is set to `derefString(cr.Conclusion)` (raw conclusion value like 'success', 'failure'). This means the `Description` field has a different semantic for check runs vs commit statuses — for statuses it's a human-readable description ('Build passed'), while for check runs it's a machine value ('success'). This is documented in the comment but may surprise callers. Consider using `cr.Name` or leaving it empty if no textual description is available from the check runs API (the `output.summary` field would require schema expansion).
Context: cr.Name,
Status: mapCheckRunStatus(cr.Conclusion),
Description: derefString(cr.Conclusion),
Description: derefString(cr.Conclusion), // raw conclusion value (e.g. "success", "failure", "skipped")
TargetURL: cr.HTMLURL,
security-review-bot commented 2026-05-13 05:19:53 +00:00
Review
Copy Link
Copy Source

[MINOR] mapCheckRunStatus maps "cancelled", "skipped", and "neutral" to "success". If this status is later used to gate security-critical actions (e.g., merges), a cancelled required check could be interpreted as success and potentially weaken enforcement. Consider mapping these to a non-success state (e.g., pending) or exposing them distinctly so callers can enforce policies accurately.

**[MINOR]** mapCheckRunStatus maps "cancelled", "skipped", and "neutral" to "success". If this status is later used to gate security-critical actions (e.g., merges), a cancelled required check could be interpreted as success and potentially weaken enforcement. Consider mapping these to a non-success state (e.g., pending) or exposing them distinctly so callers can enforce policies accurately.
})
}
1
 @@ -181,9 +190,17 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
return result, nil
}
sonnet-review-bot commented 2026-05-13 04:22:04 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The mapCheckRunStatus function maps cancelled, skipped, and neutral to success. The comment says 'non-blocking' but this could be surprising to callers who depend on CommitStatus.Status for gating decisions — a cancelled check that maps to 'success' might unblock a review when it shouldn't. This is a semantic/policy decision that should be documented more prominently in the function's doc comment (the current comment only appears inline in the switch case, not in the function doc).

**[MINOR]** The `mapCheckRunStatus` function maps `cancelled`, `skipped`, and `neutral` to `success`. The comment says 'non-blocking' but this could be surprising to callers who depend on CommitStatus.Status for gating decisions — a cancelled check that maps to 'success' might unblock a review when it shouldn't. This is a semantic/policy decision that should be documented more prominently in the function's doc comment (the current comment only appears inline in the switch case, not in the function doc).
// mapCheckRunStatus maps a check run conclusion to a vcs.CommitStatus status string.
// mapCheckRunStatus maps a GitHub check run conclusion to a vcs.CommitStatus status string.
// Conclusion alone determines the mapped state: nil conclusion means the run is
// still in progress (pending), regardless of the status field value.
//
// Mapping rules:
sonnet-review-bot commented 2026-05-13 05:18:42 +00:00
Review
Copy Link
Copy Source

[MINOR] Mapping cancelled and skipped check run conclusions to "success" is semantically questionable. These conclusions mean the check was not run, not that it passed. Callers using vcs.CommitStatus.Status for gate-checking (e.g. blocking review until all checks pass) will treat skipped/cancelled checks as green, potentially masking required checks that were cancelled. A dedicated "skipped" or "neutral" status value — or at least "neutral" mapped to a neutral/unknown status — would be more honest. This is a design decision with real correctness implications; the comment acknowledges it but the downstream impact depends on how callers interpret the status.

**[MINOR]** Mapping `cancelled` and `skipped` check run conclusions to `"success"` is semantically questionable. These conclusions mean the check was not run, not that it passed. Callers using `vcs.CommitStatus.Status` for gate-checking (e.g. blocking review until all checks pass) will treat skipped/cancelled checks as green, potentially masking required checks that were cancelled. A dedicated `"skipped"` or `"neutral"` status value — or at least `"neutral"` mapped to a neutral/unknown status — would be more honest. This is a design decision with real correctness implications; the comment acknowledges it but the downstream impact depends on how callers interpret the status.
// - nil → "pending" (run still in progress or queued)
// - "success" → "success"
// - "failure", "action_required", "timed_out" → "failure"
// - "cancelled", "skipped", "neutral" → "success" (non-blocking per GitHub check suite semantics)
// - "stale", "waiting" → "pending"
// - unknown values → "pending" (conservative: treat unrecognized conclusions as incomplete)
func mapCheckRunStatus(conclusion *string) string {
if conclusion == nil {
// Still running or queued
github/pr_test.go
+39
View File
7
 @@ -545,6 +545,7 @@ func TestGetCommitStatuses_CheckRunConclusions(t *testing.T) {
name = *tt.conclusion
}
t.Run(name, func(t *testing.T) {
t.Parallel()
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if strings.Contains(r.URL.Path, "/status") {
json.NewEncoder(w).Encode(map[string]interface{}{
1
 @@ -632,6 +633,44 @@ func TestGetCommitStatuses_MalformedJSON(t *testing.T) {
}
}
func TestGetCommitStatuses_CheckRunsErrorAfterStatusesSucceed(t *testing.T) {
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
switch {
case strings.Contains(r.URL.Path, "/status"):
// Statuses succeed
json.NewEncoder(w).Encode(map[string]interface{}{
"state": "success",
"statuses": []map[string]string{
{
"context": "ci/build",
"state": "success",
"description": "Build passed",
"target_url": "https://ci.example.com/1",
},
},
})
case strings.Contains(r.URL.Path, "/check-runs"):
// Check runs fail with 500
w.WriteHeader(500)
w.Write([]byte(`{"message":"Internal Server Error"}`))
default:
w.WriteHeader(404)
}
}))
defer srv.Close()
c := NewClient("token", srv.URL, AllowInsecureHTTP())
c.SetHTTPClient(srv.Client())
_, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "abc123")
if err == nil {
t.Fatal("expected error when check-runs endpoint fails after statuses succeed")
}
if !strings.Contains(err.Error(), "fetch check runs") {
t.Errorf("expected check runs error, got: %v", err)
}
}
func stringPtr(s string) *string {
return &s
}