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

feat(cmd): wire --provider and --base-url flags into CLI (Phase 5) #106

Merged
aweiker merged 17 commits from review-bot-issue-82 into feature/github-support 2026-05-13 17:16:28 +00:00
Conversation 98 Commits 17 Files Changed 14
+4 -8
4 changed files with 4 additions and 8 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit e70b54f238 - Show all commits
cmd/review-bot/main.go
-4
View File
29
 @@ -179,9 +179,6 @@ func main() {
ghBaseURL = "https://api.github.com"
gpt-review-bot commented 2026-05-13 15:52:47 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] When provider == github, base URL fallback logic duplicates the default already handled by github.NewClient("", ""). You can simplify by passing the possibly-empty baseURL and letting NewClient use its default.

**[NIT]** When provider == github, base URL fallback logic duplicates the default already handled by github.NewClient("", ""). You can simplify by passing the possibly-empty baseURL and letting NewClient use its default.
}
client = github.NewClient(*reviewerToken, ghBaseURL)
default:
fmt.Fprintf(os.Stderr, "Error: unhandled provider %q\n", *provider)
os.Exit(1)
}
slog.Info("VCS client initialized", "provider", *provider)
sonnet-review-bot commented 2026-05-13 15:51:02 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The switch for VCS client initialization has no default branch (the provider validation switch above guarantees valid values, so this is technically safe), but adding a default: panic(...) or default: slog.Error(...); os.Exit(1) would make the invariant explicit and guard against future providers being added to the validation switch without a corresponding factory branch.

**[NIT]** The switch for VCS client initialization has no default branch (the provider validation switch above guarantees valid values, so this is technically safe), but adding a default: panic(...) or default: slog.Error(...); os.Exit(1) would make the invariant explicit and guard against future providers being added to the validation switch without a corresponding factory branch.
51
 @@ -899,7 +896,6 @@ func extractSentinelName(body string) string {
return name
}
// findAllOwnReviews returns all non-superseded reviews matching the sentinel.
rodin marked this conversation as resolved Outdated
sonnet-review-bot commented 2026-05-13 11:15:28 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] There is a stray blank line before // findAllOwnReviews returns all non-superseded reviews matching the sentinel. after the extractSentinelName function body closing brace. Minor cosmetic issue.

**[NIT]** There is a stray blank line before `// findAllOwnReviews returns all non-superseded reviews matching the sentinel.` after the `extractSentinelName` function body closing brace. Minor cosmetic issue.
func findAllOwnReviews(reviews []vcs.Review, sentinel string) []vcs.Review {
var result []vcs.Review
cmd/review-bot/main_test.go
+2 -3
View File
@@ -210,7 +210,6 @@ func TestBuildSupersededBodyShortSHA(t *testing.T) {
}
}
func TestHasSharedToken(t *testing.T) {
tests := []struct {
name string
2
 @@ -646,8 +645,8 @@ func TestExtractSentinelName_EdgeCases(t *testing.T) {
{"<!-- review-bot:sonnet --> rest", "sonnet"},
{"<!-- review-bot:gpt-review --> rest", "gpt-review"},
{"no sentinel here", "unknown"},
{"<!-- review-bot:", "unknown"}, // prefix but no suffix
{"prefix <!-- review-bot:abc --> end", "abc"}, // embedded in text
{"<!-- review-bot:", "unknown"}, // prefix but no suffix
{"prefix <!-- review-bot:abc --> end", "abc"}, // embedded in text
}
for _, tc := range tests {
github/client.go
+1
View File
2
 @@ -386,6 +386,7 @@ func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, r
return c.doRequestCore(ctx, method, reqURL, opts)
gpt-review-bot commented 2026-05-13 13:51:45 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] Minor formatting: there is an extra blank line between the closing brace of doRequestWithBody and the start of doJSONRequest that could be removed for consistency.

**[NIT]** Minor formatting: there is an extra blank line between the closing brace of doRequestWithBody and the start of doJSONRequest that could be removed for consistency.
sonnet-review-bot commented 2026-05-13 14:15:32 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] There is a spurious blank line before the closing brace of doRequestWithBody (after return c.doRequestCore(...)). Minor formatting issue that gofmt would not flag since it's inside a function body, but it's visually inconsistent.

**[NIT]** There is a spurious blank line before the closing brace of `doRequestWithBody` (after `return c.doRequestCore(...)`). Minor formatting issue that gofmt would not flag since it's inside a function body, but it's visually inconsistent.
}
sonnet-review-bot commented 2026-05-13 15:51:02 +00:00
Review
Copy Link
Copy Source

[NIT] doJSONRequest is added but not yet called by any public methods in the diff (the github package has no PostReview/DismissReview/etc. visible in the diff). This is fine for an incremental PR, but it means the new helper is untested at the integration level. The unit tests for doJSONRequest itself are present and cover the retry path.

**[NIT]** doJSONRequest is added but not yet called by any public methods in the diff (the github package has no PostReview/DismissReview/etc. visible in the diff). This is fine for an incremental PR, but it means the new helper is untested at the integration level. The unit tests for doJSONRequest itself are present and cover the retry path.
// doJSONRequest performs an HTTP request with a JSON body and returns the response body.
// It delegates retry/backoff/429 handling to doRequestWithBody.
sonnet-review-bot commented 2026-05-13 10:30:42 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] doJSONRequest does not retry on HTTP 429 like doRequest does. Since PostReview and DismissReview are write operations that can be rate-limited by GitHub, missing retry logic here could cause failures in high-traffic repos. Consider either adding retry logic or documenting that write operations are not retried.

**[MINOR]** `doJSONRequest` does not retry on HTTP 429 like `doRequest` does. Since `PostReview` and `DismissReview` are write operations that can be rate-limited by GitHub, missing retry logic here could cause failures in high-traffic repos. Consider either adding retry logic or documenting that write operations are not retried.
// This is a general-purpose helper used by any method that needs to send JSON payloads
2
review/persona_test.go
+1 -1
View File
@@ -355,7 +355,7 @@ func TestCapitalizeFirst(t *testing.T) {
{"HELLO", "HELLO"},
{"a", "A"},
{"", ""},
{"日本語", "日本語"}, // Non-ASCII: Japanese doesn't have case
{"日本語", "日本語"}, // Non-ASCII: Japanese doesn't have case
{"über", "Über"}, // German umlaut
{"élève", "Élève"}, // French accent
}