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

feat(github): implement Reviewer and Identity interfaces (#81) #105

Merged
aweiker merged 7 commits from review-bot-issue-81 into feature/github-support 2026-05-13 13:39:14 +00:00
Conversation 40 Commits 7 Files Changed 7
+7 -11
3 changed files with 7 additions and 11 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 293296b50c - Show all commits
github/client.go
+2 -1
View File
@@ -196,7 +196,8 @@ func (c *Client) SetRetryBackoff(d []time.Duration) error {
// requestOptions holds per-request configuration for doRequestCore.
type requestOptions struct {
gpt-review-bot commented 2026-05-13 07:38:51 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The requestOptions.bodyFn comment lists POST, PUT, PATCH as body-carrying methods but omits DELETE-with-body. Update the comment to reflect that it must be non-nil for any request that carries a body (including DELETE when applicable) to avoid confusion.

**[NIT]** The requestOptions.bodyFn comment lists POST, PUT, PATCH as body-carrying methods but omits DELETE-with-body. Update the comment to reflect that it must be non-nil for any request that carries a body (including DELETE when applicable) to avoid confusion.
// bodyFn returns a fresh io.Reader for the request body on each attempt.
// Must be non-nil for requests that carry a body (POST, PUT, PATCH).
// Must be non-nil for any request that carries a body (POST, PUT, PATCH,
// or DELETE when a body is required by the API).
// Returning a fresh reader on each call allows retries to re-send the body.
bodyFn func() io.Reader
4
github/review.go
+5 -8
View File
2
 @@ -59,7 +59,8 @@ func translateGitHubReviewState(state string) string {
return "COMMENT"
default:
// States like APPROVED, DISMISSED, and PENDING pass through unchanged
// as they already match the canonical vcs representation.
// as they already match the canonical vcs representation. PENDING appears
sonnet-review-bot commented 2026-05-13 07:26:59 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] translateGitHubReviewState maps "APPROVED" → "APPROVED" and "DISMISSED" → "DISMISSED" explicitly, which is identical to the default passthrough case. These two cases can be removed, leaving only the two non-trivial translations (CHANGES_REQUESTED and COMMENTED). This simplifies the function and makes the intent clearer: only translate states that differ from their canonical representation.

**[MINOR]** translateGitHubReviewState maps `"APPROVED"` → `"APPROVED"` and `"DISMISSED"` → `"DISMISSED"` explicitly, which is identical to the default passthrough case. These two cases can be removed, leaving only the two non-trivial translations (CHANGES_REQUESTED and COMMENTED). This simplifies the function and makes the intent clearer: only translate states that differ from their canonical representation.
// on draft reviews that have not yet been submitted via the GitHub UI or API.
return state
}
}
@@ -77,16 +78,12 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
Event: string(req.Event),
}
sonnet-review-bot commented 2026-05-13 07:55:21 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] PostReview doc comment says 'ReviewRequest.Event values (APPROVE, REQUEST_CHANGES, COMMENT) are sent directly — they match GitHub's canonical event strings.' This is misleading: GitHub's canonical event strings are actually APPROVE, REQUEST_CHANGES, and COMMENT, but the vcs type uses ReviewEventApprove etc. which the test confirms maps to 'APPROVE'. The comment is correct about the wire format but conflates the vcs abstraction with the GitHub API. More importantly, when a review has no comments, payload.Comments will be nil (not assigned), which is fine since it's omitempty, but this is an implicit assumption worth noting.

**[MINOR]** PostReview doc comment says 'ReviewRequest.Event values (APPROVE, REQUEST_CHANGES, COMMENT) are sent directly — they match GitHub's canonical event strings.' This is misleading: GitHub's canonical event strings are actually APPROVE, REQUEST_CHANGES, and COMMENT, but the vcs type uses ReviewEventApprove etc. which the test confirms maps to 'APPROVE'. The comment is correct about the wire format but conflates the vcs abstraction with the GitHub API. More importantly, when a review has no comments, `payload.Comments` will be nil (not assigned), which is fine since it's `omitempty`, but this is an implicit assumption worth noting.
// Populate CommitID from the first comment if available.
// Populate CommitID from the first comment and build the payload in one pass.
// All comments in a single review share the same commit_id.
for _, comment := range req.Comments {
sonnet-review-bot commented 2026-05-13 07:37:01 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] PostReview iterates over req.Comments twice: once to find the CommitID and once to build the payload.Comments slice. This could be combined into a single loop. Minor readability/efficiency issue on the common path.

**[NIT]** PostReview iterates over req.Comments twice: once to find the CommitID and once to build the payload.Comments slice. This could be combined into a single loop. Minor readability/efficiency issue on the common path.
if comment.CommitID != "" {
if payload.CommitID == "" && comment.CommitID != "" {
payload.CommitID = comment.CommitID
break
}
}
for _, comment := range req.Comments {
payload.Comments = append(payload.Comments, reviewCommentEntry{
Path: comment.Path,
Position: comment.Position,
5
 @@ -159,7 +156,7 @@ func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number in
if err != nil {
var apiErr *APIError
if errors.As(err, &apiErr) && apiErr.StatusCode == 422 {
return ErrCannotDeleteSubmittedReview
return fmt.Errorf("delete review: %w", ErrCannotDeleteSubmittedReview)
}
return fmt.Errorf("delete review: %w", err)
}
sonnet-review-bot commented 2026-05-13 09:05:11 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The DeleteReview method passes nil to doRequestWithBody, which is handled correctly by the nil check in that function. However, for a DELETE with no body it would be slightly more idiomatic (and self-documenting at the call site) to call doRequest or doGet with http.MethodDelete directly. This is a minor style point; the current approach works correctly and the inline comment explains the intent.

**[NIT]** The `DeleteReview` method passes `nil` to `doRequestWithBody`, which is handled correctly by the nil check in that function. However, for a DELETE with no body it would be slightly more idiomatic (and self-documenting at the call site) to call `doRequest` or `doGet` with `http.MethodDelete` directly. This is a minor style point; the current approach works correctly and the inline comment explains the intent.
sonnet-review-bot commented 2026-05-13 10:07:07 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] DeleteReview calls doRequestWithBody with a nil body. The existing doGet wrapper (which calls doRequest → doRequestCore) would work just as well for a bodyless DELETE, but since DELETE with a body is unusual and doRequestWithBody(ctx, DELETE, url, nil) correctly produces a bodyless request (the nil-body branch in doRequestWithBody), this is functionally fine. Consider adding a brief comment clarifying that the nil is intentional and not an oversight (the existing inline comment already does this, so this is truly minor).

**[NIT]** DeleteReview calls `doRequestWithBody` with a nil body. The existing `doGet` wrapper (which calls `doRequest` → `doRequestCore`) would work just as well for a bodyless DELETE, but since DELETE with a body is unusual and `doRequestWithBody(ctx, DELETE, url, nil)` correctly produces a bodyless request (the nil-body branch in doRequestWithBody), this is functionally fine. Consider adding a brief comment clarifying that the nil is intentional and not an oversight (the existing inline comment already does this, so this is truly minor).
rodin commented 2026-05-13 10:07:38 +00:00
Outdated
Review
Copy Link
Copy Source

Pushing back on this one. The current approach works correctly, is tested, and the nil-body contract is already documented via the inline comment added in a prior revision (// nil body: the GitHub DELETE endpoint for reviews requires no request body.). Additionally, doGet does not accept a method parameter — it always uses GET — so refactoring to use it for DELETE is not trivial without introducing a new helper. Leaving as-is.

Pushing back on this one. The current approach works correctly, is tested, and the nil-body contract is already documented via the inline comment added in a prior revision (`// nil body: the GitHub DELETE endpoint for reviews requires no request body.`). Additionally, `doGet` does not accept a method parameter — it always uses GET — so refactoring to use it for DELETE is not trivial without introducing a new helper. Leaving as-is.
2
github/review_test.go
-2
View File
9
 @@ -379,5 +379,3 @@ func TestTranslateGitHubReviewState(t *testing.T) {
}
}
sonnet-review-bot commented 2026-05-13 07:26:59 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] containsStr and containsSubstring reimplement strings.Contains from the standard library. The repository conventions do not restrict use of the standard library; strings.Contains(s, substr) should be used directly. The current implementation is also slightly incorrect: containsStr returns true when s == substr even without going through containsSubstring, but the logic mixing len checks and || chains is harder to read and maintain than a direct strings.Contains call.

**[MINOR]** containsStr and containsSubstring reimplement strings.Contains from the standard library. The repository conventions do not restrict use of the standard library; `strings.Contains(s, substr)` should be used directly. The current implementation is also slightly incorrect: `containsStr` returns true when `s == substr` even without going through `containsSubstring`, but the logic mixing `len` checks and `||` chains is harder to read and maintain than a direct `strings.Contains` call.
}
sonnet-review-bot commented 2026-05-13 07:37:01 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] There are two trailing blank lines at the end of review_test.go. gofmt typically produces a single trailing newline; this is minor but inconsistent with the style pattern.

**[NIT]** There are two trailing blank lines at the end of review_test.go. gofmt typically produces a single trailing newline; this is minor but inconsistent with the style pattern.
11