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
+9 -6
2 changed files with 9 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit c4af35cd78 - Show all commits
cmd/review-bot/main.go
+8 -3
View File
20
 @@ -170,7 +170,8 @@ func main() {
}
sonnet-review-bot commented 2026-05-13 08:52:25 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The panic on the default branch of the VCS provider switch (panic("unreachable: unhandled provider " + *provider)) is technically correct since the provider is validated above, but per the repository's convention (Return errors; never panic), this should arguably be an error return or os.Exit. In main() context an os.Exit with an error message would be more consistent with the rest of the validation error handling.

**[NIT]** The panic on the default branch of the VCS provider switch (panic("unreachable: unhandled provider " + *provider)) is technically correct since the provider is validated above, but per the repository's convention (Return errors; never panic), this should arguably be an error return or os.Exit. In main() context an os.Exit with an error message would be more consistent with the rest of the validation error handling.
gpt-review-bot commented 2026-05-13 08:53:50 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Default branch of the provider switch panics on an unreachable path. Repository convention states 'Return errors; never panic.' Prefer logging an error and exiting with a non-zero status instead of panic, even for unreachable cases.

**[MINOR]** Default branch of the provider switch panics on an unreachable path. Repository convention states 'Return errors; never panic.' Prefer logging an error and exiting with a non-zero status instead of panic, even for unreachable cases.
gpt-review-bot commented 2026-05-13 16:36:00 +00:00
Review
Copy Link
Copy Source

[MINOR] Default case in the VCS provider switch uses panic("unreachable..."). Repository conventions state "never panic"; prefer logging and exiting with a non-zero status to enforce unreachable conditions.

**[MINOR]** Default case in the VCS provider switch uses panic("unreachable..."). Repository conventions state "never panic"; prefer logging and exiting with a non-zero status to enforce unreachable conditions.
client = github.NewClient(*reviewerToken, ghBaseURL)
sonnet-review-bot commented 2026-05-13 08:22:47 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The switch for initializing var client vcs.Client has no default case. Since the provider is validated earlier with an explicit switch+os.Exit(1), client will always be set before use. However, the Go compiler cannot prove this, and if the validation switch ever diverges from the initialization switch (e.g., a new provider is added to validation but not initialization), client would remain nil and panic at first use. A default: panic(...) or a default: os.Exit(1) would make the code more robust.

**[MINOR]** The switch for initializing `var client vcs.Client` has no `default` case. Since the provider is validated earlier with an explicit switch+os.Exit(1), `client` will always be set before use. However, the Go compiler cannot prove this, and if the validation switch ever diverges from the initialization switch (e.g., a new provider is added to validation but not initialization), `client` would remain nil and panic at first use. A `default: panic(...)` or a `default: os.Exit(1)` would make the code more robust.
security-review-bot commented 2026-05-13 11:16:22 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] When initializing the Gitea client, there is no enforcement or warning against using an insecure (HTTP) VCS URL with a token. Unlike the GitHub client which refuses to send credentials over HTTP by default, this path may allow plaintext token transmission depending on gitea.Client behavior. Recommend warning or rejecting non-HTTPS --vcs-url unless explicitly allowed for trusted/local use.

**[MINOR]** When initializing the Gitea client, there is no enforcement or warning against using an insecure (HTTP) VCS URL with a token. Unlike the GitHub client which refuses to send credentials over HTTP by default, this path may allow plaintext token transmission depending on gitea.Client behavior. Recommend warning or rejecting non-HTTPS --vcs-url unless explicitly allowed for trusted/local use.
default:
panic("unreachable: unhandled provider " + *provider)
fmt.Fprintf(os.Stderr, "Error: unhandled provider %q\n", *provider)
sonnet-review-bot commented 2026-05-13 10:55:52 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The default case in the VCS client factory switch (lines ~188-191) sets fmt.Fprintf(os.Stderr, ...) and os.Exit(1). This is unreachable dead code since the same provider values are validated in the earlier switch at line 109-115. It's defensive coding which is fine, but it's worth noting in a comment that this case cannot be reached given the prior validation.

**[NIT]** The `default` case in the VCS client factory switch (lines ~188-191) sets `fmt.Fprintf(os.Stderr, ...)` and `os.Exit(1)`. This is unreachable dead code since the same provider values are validated in the earlier switch at line 109-115. It's defensive coding which is fine, but it's worth noting in a comment that this case cannot be reached given the prior validation.
os.Exit(1)
}
security-review-bot commented 2026-05-13 10:31:43 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The --base-url (VCS_BASE_URL) for the GitHub API is fully user-controlled and used with the reviewer token; while HTTPS is enforced and cross-host redirects strip Authorization, an attacker who can influence this configuration could direct requests (and thus tokens) to an arbitrary HTTPS endpoint. Consider allowlisting known GitHub Enterprise hosts or warning when the base URL differs from the default to reduce misconfiguration/exfiltration risk.

**[MINOR]** The --base-url (VCS_BASE_URL) for the GitHub API is fully user-controlled and used with the reviewer token; while HTTPS is enforced and cross-host redirects strip Authorization, an attacker who can influence this configuration could direct requests (and thus tokens) to an arbitrary HTTPS endpoint. Consider allowlisting known GitHub Enterprise hosts or warning when the base URL differs from the default to reduce misconfiguration/exfiltration risk.
sonnet-review-bot commented 2026-05-13 16:34:41 +00:00
Review
Copy Link
Copy Source

[MINOR] The panic("unreachable: ...") in the default case of the VCS client switch violates the project convention "Return errors; never panic" (from CONVENTIONS.md). Although the validation at line 104 does make this branch genuinely unreachable at runtime, the convention is unconditional. A cleaner alternative is slog.Error(...); os.Exit(1) — consistent with every other fatal error path in this file — or the validation could be made to work through a single code path. The panic is acceptable as a defensive measure (and correctly named 'unreachable'), but it deviates from stated conventions.

**[MINOR]** The `panic("unreachable: ...")` in the `default` case of the VCS client switch violates the project convention "Return errors; never panic" (from CONVENTIONS.md). Although the validation at line 104 does make this branch genuinely unreachable at runtime, the convention is unconditional. A cleaner alternative is `slog.Error(...); os.Exit(1)` — consistent with every other fatal error path in this file — or the validation could be made to work through a single code path. The `panic` is acceptable as a defensive measure (and correctly named 'unreachable'), but it deviates from stated conventions.
slog.Info("VCS client initialized", "provider", *provider)
security-review-bot commented 2026-05-13 11:35:31 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The GitHub client uses a user-supplied base URL (--base-url/VCS_BASE_URL). While HTTPS is enforced and redirects strip Authorization, a misconfigured or attacker-controlled base URL could still receive the token over HTTPS. Consider validating/allowlisting expected hosts for production deployments, or requiring an explicit opt-in when using non-default hosts.

**[MINOR]** The GitHub client uses a user-supplied base URL (--base-url/VCS_BASE_URL). While HTTPS is enforced and redirects strip Authorization, a misconfigured or attacker-controlled base URL could still receive the token over HTTPS. Consider validating/allowlisting expected hosts for production deployments, or requiring an explicit opt-in when using non-default hosts.
33
 @@ -523,7 +524,8 @@ func verdictToEvent(verdict string) vcs.ReviewEvent {
// For GitHub: dismisses old reviews.
// For Gitea: edits the review body and resolves inline comments.
func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsURL, owner, repoName string, prNumber int, oldReviews []vcs.Review, newReviewID int64, sentinel string) error {
if provider == "github" {
switch provider {
case "github":
for _, old := range oldReviews {
if err := client.DismissReview(ctx, owner, repoName, prNumber, old.ID, "Superseded by new review"); err != nil {
sonnet-review-bot commented 2026-05-13 09:37:04 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews uses a switch with a case "gitea": fall-through comment but doesn't actually use the Go fall-through mechanism — it just breaks out of the switch and continues below. This is correct behavior but the comment 'Fall through to Gitea-specific logic below the switch' is slightly misleading since Go switch cases don't fall through by default and no fallthrough keyword is used. The intent is clear but the comment could be reworded to 'continue to Gitea-specific logic below' to avoid confusion.

**[MINOR]** supersedeOldReviews uses a switch with a `case "gitea":` fall-through comment but doesn't actually use the Go fall-through mechanism — it just breaks out of the switch and continues below. This is correct behavior but the comment 'Fall through to Gitea-specific logic below the switch' is slightly misleading since Go switch cases don't fall through by default and no `fallthrough` keyword is used. The intent is clear but the comment could be reworded to 'continue to Gitea-specific logic below' to avoid confusion.
rodin commented 2026-05-13 10:10:20 +00:00
Outdated
Review
Copy Link
Copy Source

Good catch — reworded from "Fall through to Gitea-specific logic below the switch" to "Continue to Gitea-specific logic below the switch" in 7d6fe27. The term "fall through" is misleading in Go context since it implies the fallthrough keyword.

Good catch — reworded from "Fall through to Gitea-specific logic below the switch" to "Continue to Gitea-specific logic below the switch" in 7d6fe27. The term "fall through" is misleading in Go context since it implies the `fallthrough` keyword.
slog.Warn("failed to dismiss review", "id", old.ID, "error", err)
@@ -532,9 +534,12 @@ func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsUR
}
sonnet-review-bot commented 2026-05-13 15:51:02 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews performs a type assertion (client.(*gitea.Adapter)) that creates a layering violation: the cmd package now depends on the concrete gitea.Adapter type for provider-specific logic. The comment acknowledges the brittleness ('guards against future refactors') but a cleaner approach would be to add a Gitea-specific optional interface (e.g. vcs.GiteaSuperseder) similar to how ReviewerSelfRequester is handled, avoiding the concrete type assertion entirely.

**[MINOR]** supersedeOldReviews performs a type assertion (client.(*gitea.Adapter)) that creates a layering violation: the cmd package now depends on the concrete gitea.Adapter type for provider-specific logic. The comment acknowledges the brittleness ('guards against future refactors') but a cleaner approach would be to add a Gitea-specific optional interface (e.g. vcs.GiteaSuperseder) similar to how ReviewerSelfRequester is handled, avoiding the concrete type assertion entirely.
}
return nil
case "gitea":
sonnet-review-bot commented 2026-05-13 08:52:25 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews uses a string-based provider switch ('github' / else-Gitea) rather than a typed constant or enum. The provider value is passed as a plain string through multiple function calls. If a third provider is added later, it's easy to miss this function. Consider using the same switch structure as the client factory (case 'gitea': / case 'github': / default: return fmt.Errorf) to make exhaustiveness explicit.

**[MINOR]** supersedeOldReviews uses a string-based provider switch ('github' / else-Gitea) rather than a typed constant or enum. The provider value is passed as a plain string through multiple function calls. If a third provider is added later, it's easy to miss this function. Consider using the same switch structure as the client factory (case 'gitea': / case 'github': / default: return fmt.Errorf) to make exhaustiveness explicit.
// Gitea: EditComment + ResolveComment flow
default:
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

[MINOR] The supersedeOldReviews function has a duplicate doc comment: the first line says 'supersedeOldReviews marks old reviews as superseded.' and the second line repeats 'supersedeOldReviews marks prior reviews as superseded so only the latest review is visible.' This is a copy-paste artifact that should be cleaned up.

**[MINOR]** The `supersedeOldReviews` function has a duplicate doc comment: the first line says 'supersedeOldReviews marks old reviews as superseded.' and the second line repeats 'supersedeOldReviews marks prior reviews as superseded so only the latest review is visible.' This is a copy-paste artifact that should be cleaned up.
return fmt.Errorf("supersedeOldReviews: unsupported provider %q", provider)
}
// Gitea: existing EditComment + ResolveComment flow
giteaAdapter, ok := client.(*gitea.Adapter)
if !ok {
gpt-review-bot commented 2026-05-13 13:51:45 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] Provider-specific behavior is implemented via a concrete type assertion to *gitea.Adapter in supersedeOldReviews. This is acceptable, but consider an optional capability interface (e.g., an optional interface on vcs.Client) to avoid tight coupling and make future wrappers/decorators simpler.

**[MINOR]** Provider-specific behavior is implemented via a concrete type assertion to *gitea.Adapter in supersedeOldReviews. This is acceptable, but consider an optional capability interface (e.g., an optional interface on vcs.Client) to avoid tight coupling and make future wrappers/decorators simpler.
return fmt.Errorf("expected gitea.Adapter for gitea provider, got %T", client)
15
github/reviews.go
+1 -3
View File
1
 @@ -42,7 +42,6 @@ type reviewCommentCreate struct {
// dismissReviewRequest is the GitHub API request body for dismissing a review.
type dismissReviewRequest struct {
Message string `json:"message"`
Event string `json:"event"`
}
// userResponse is the GitHub API response for the authenticated user.
2
 @@ -60,7 +59,7 @@ func translateReviewEvent(event vcs.ReviewEvent) string {
case vcs.ReviewEventComment:
return "COMMENT"
sonnet-review-bot commented 2026-05-13 10:08:59 +00:00
Outdated
Review
Copy Link
Copy Source

[NIT] The translateReviewEvent function has a case vcs.ReviewEventComment: return "COMMENT" followed by default: return "COMMENT". The default case is unreachable given the current vcs.ReviewEvent type but is harmless. No change needed; just noting it's redundant.

**[NIT]** The `translateReviewEvent` function has a `case vcs.ReviewEventComment: return "COMMENT"` followed by `default: return "COMMENT"`. The default case is unreachable given the current `vcs.ReviewEvent` type but is harmless. No change needed; just noting it's redundant.
default:
return string(event)
return "COMMENT"
}
}
17
 @@ -161,7 +160,6 @@ func (c *Client) DismissReview(ctx context.Context, owner, repo string, number i
payload := dismissReviewRequest{
Message: message,
sonnet-review-bot commented 2026-05-13 09:37:04 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] doJSONRequest is defined on *Client in this file but conceptually duplicates HTTP infrastructure that likely already exists elsewhere in the github package (the file references c.doGet, c.doRequest, c.allowInsecureHTTP, c.httpClient, c.token, userAgent, maxResponseBytes — all of which must live in another file). This is fine architecturally, but the HTTPS-enforcement check inside doJSONRequest is inconsistent with doGet/doRequest which presumably handle their own security. If those methods don't enforce HTTPS, the security posture is uneven. If they do, the duplication is unnecessary. Worth ensuring the pattern is consistent across the package.

**[MINOR]** doJSONRequest is defined on *Client in this file but conceptually duplicates HTTP infrastructure that likely already exists elsewhere in the github package (the file references c.doGet, c.doRequest, c.allowInsecureHTTP, c.httpClient, c.token, userAgent, maxResponseBytes — all of which must live in another file). This is fine architecturally, but the HTTPS-enforcement check inside doJSONRequest is inconsistent with doGet/doRequest which presumably handle their own security. If those methods don't enforce HTTPS, the security posture is uneven. If they do, the duplication is unnecessary. Worth ensuring the pattern is consistent across the package.
rodin commented 2026-05-13 10:10:20 +00:00
Outdated
Review
Copy Link
Copy Source

Confirmed: doRequest (which doGet and DeleteReview use) already enforces the identical HTTPS-only check at lines 219-226 of client.go. doJSONRequest has its own copy because it handles the request lifecycle differently (JSON marshaling, bytes.NewReader, content headers) and doesn't route through doRequest. The security posture is consistent across all HTTP methods — no gap exists.

A future refactor could extract the URL-scheme check into a shared helper, but that's cosmetic. No changes needed here.

Confirmed: `doRequest` (which `doGet` and `DeleteReview` use) already enforces the identical HTTPS-only check at lines 219-226 of `client.go`. `doJSONRequest` has its own copy because it handles the request lifecycle differently (JSON marshaling, `bytes.NewReader`, content headers) and doesn't route through `doRequest`. The security posture is consistent across all HTTP methods — no gap exists. A future refactor could extract the URL-scheme check into a shared helper, but that's cosmetic. No changes needed here.
Event: "DISMISS",
}
sonnet-review-bot commented 2026-05-13 09:24:34 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] doJSONRequest is defined in github/reviews.go but it is a general HTTP helper. If other files in the github package (e.g. the existing client file) also define HTTP helpers, there may be duplication or naming conflicts. This is not visible from the diff alone, but worth verifying that doRequest (used by DeleteReview) and doGet (used by ListReviews, GetAuthenticatedUser) are also defined in the github package and that doJSONRequest doesn't conflict with or duplicate them.

**[MINOR]** `doJSONRequest` is defined in `github/reviews.go` but it is a general HTTP helper. If other files in the `github` package (e.g. the existing client file) also define HTTP helpers, there may be duplication or naming conflicts. This is not visible from the diff alone, but worth verifying that `doRequest` (used by `DeleteReview`) and `doGet` (used by `ListReviews`, `GetAuthenticatedUser`) are also defined in the `github` package and that `doJSONRequest` doesn't conflict with or duplicate them.
sonnet-review-bot commented 2026-05-13 10:08:59 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] doJSONRequest is defined in github/reviews.go but is a general HTTP helper. If other files in the github package (e.g., the client file not shown in the diff) also define HTTP helpers, this may cause a naming conflict or duplication. Consider whether this belongs in a shared internal helper or the main client file. This is a structural concern rather than a correctness issue.

**[MINOR]** `doJSONRequest` is defined in `github/reviews.go` but is a general HTTP helper. If other files in the `github` package (e.g., the client file not shown in the diff) also define HTTP helpers, this may cause a naming conflict or duplication. Consider whether this belongs in a shared internal helper or the main client file. This is a structural concern rather than a correctness issue.
_, err := c.doJSONRequest(ctx, http.MethodPut, reqURL, payload)
4