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
+45 -53
5 changed files with 45 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 34f7393892 - Show all commits
cmd/review-bot/main.go
+8 -12
View File
38
 @@ -429,7 +429,7 @@ func main() {
currentSHA = currentPR.Head.SHA
}
if shouldSkipStaleReview(evaluatedSHA, currentSHA) {
slog.Warn("HEAD moved during review — skipping stale review",
slog.Warn("HEAD moved during review -- skipping stale review",
"evaluated", evaluatedSHA,
"current", currentSHA,
"pr", prNumber)
4
 @@ -477,12 +477,12 @@ func main() {
}
// Self-request as reviewer (Gitea-specific; ensures we appear in required-reviewer checks)
sonnet-review-bot commented 2026-05-13 10:40:59 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews takes provider and vcsURL as plain strings rather than deriving them from the client value. This means the provider switch inside the function can diverge from the actual client type at runtime — e.g., passing provider="gitea" with a *github.Client would fall through to the type-assertion that expects *gitea.Adapter and return a confusing error. A type switch on client directly (or a provider-specific supersede method on the interface) would be more robust and remove the stringly-typed dispatch.

**[MINOR]** `supersedeOldReviews` takes `provider` and `vcsURL` as plain strings rather than deriving them from the `client` value. This means the provider switch inside the function can diverge from the actual client type at runtime — e.g., passing `provider="gitea"` with a `*github.Client` would fall through to the type-assertion that expects `*gitea.Adapter` and return a confusing error. A type switch on `client` directly (or a provider-specific supersede method on the interface) would be more robust and remove the stringly-typed dispatch.
sonnet-review-bot commented 2026-05-13 10:55:52 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The type assertion client.(*gitea.Adapter) in supersedeOldReviews is fragile. If the gitea provider is ever initialized differently (e.g. a wrapper or decorator), this assertion silently falls through to the error. The function already receives provider string — this is a valid guard, but the assertion is load-bearing for the gitea path and there's no test covering the !ok failure branch. A doc comment noting that the assertion is expected to always succeed given the prior provider switch (enforced by the caller) would make this safer to maintain.

**[MINOR]** The type assertion `client.(*gitea.Adapter)` in `supersedeOldReviews` is fragile. If the gitea provider is ever initialized differently (e.g. a wrapper or decorator), this assertion silently falls through to the error. The function already receives `provider string` — this is a valid guard, but the assertion is load-bearing for the gitea path and there's no test covering the `!ok` failure branch. A doc comment noting that the assertion is expected to always succeed given the prior provider switch (enforced by the caller) would make this safer to maintain.
if giteaAdapter, ok := client.(*gitea.Adapter); ok {
if selfReq, ok := client.(vcs.ReviewerSelfRequester); ok {
sonnet-review-bot commented 2026-05-13 14:30:36 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The Gitea-specific RequestReviewer self-request uses a type assertion client.(*gitea.Adapter) to detect provider rather than checking *provider == "gitea". Both approaches work, but using the string flag is more consistent with how the rest of the code (e.g., supersedeOldReviews) already branches on provider. The type-assertion approach is also slightly fragile — if gitea.Adapter is ever wrapped in a decorator (as the comment in supersedeOldReviews also notes), this silently stops working.

**[MINOR]** The Gitea-specific `RequestReviewer` self-request uses a type assertion `client.(*gitea.Adapter)` to detect provider rather than checking `*provider == "gitea"`. Both approaches work, but using the string flag is more consistent with how the rest of the code (e.g., `supersedeOldReviews`) already branches on provider. The type-assertion approach is also slightly fragile — if `gitea.Adapter` is ever wrapped in a decorator (as the comment in `supersedeOldReviews` also notes), this silently stops working.
sonnet-review-bot commented 2026-05-13 16:34:41 +00:00
Review
Copy Link
Copy Source

[MINOR] When len(oldReviews) > 0 and the client does not implement vcs.ReviewSuperseder, the code logs an error (slog.Error) but does NOT call os.Exit(1). This silently continues after reporting a failure, which is inconsistent with every other slog.Error call in main() that is followed by os.Exit(1). The new review has already been posted at this point, so the old reviews will remain un-superseded without any caller-visible error. Either add os.Exit(1) or downgrade to slog.Warn if non-fatal is intentional.

**[MINOR]** When `len(oldReviews) > 0` and the client does not implement `vcs.ReviewSuperseder`, the code logs an error (`slog.Error`) but does NOT call `os.Exit(1)`. This silently continues after reporting a failure, which is inconsistent with every other `slog.Error` call in `main()` that is followed by `os.Exit(1)`. The new review has already been posted at this point, so the old reviews will remain un-superseded without any caller-visible error. Either add `os.Exit(1)` or downgrade to `slog.Warn` if non-fatal is intentional.
authUser, err := client.GetAuthenticatedUser(ctx)
if err != nil {
slog.Warn("could not determine authenticated user for reviewer self-request", "error", err)
sonnet-review-bot commented 2026-05-13 08:22:47 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] In supersedeOldReviews, for the Gitea path, os.Exit(1) is called inside a non-main function (supersedeOldReviews). This violates the convention of returning errors rather than panicking/exiting from library-style functions. The caller (main) should receive an error and call os.Exit. While this function is in main.go and is unexported, calling os.Exit in nested functions makes testing harder and is an anti-pattern per the repo conventions ('Return errors; never panic').

**[MINOR]** In `supersedeOldReviews`, for the Gitea path, `os.Exit(1)` is called inside a non-main function (`supersedeOldReviews`). This violates the convention of returning errors rather than panicking/exiting from library-style functions. The caller (main) should receive an error and call os.Exit. While this function is in main.go and is unexported, calling os.Exit in nested functions makes testing harder and is an anti-pattern per the repo conventions ('Return errors; never panic').
} else if authUser != "" {
sonnet-review-bot commented 2026-05-13 08:52:25 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The Gitea-specific self-request reviewer block uses a type assertion (client.(*gitea.Adapter)) which leaks provider-specific behavior into main(). This is the established pattern here since Gitea's RequestReviewer isn't part of the vcs.Client interface, but worth documenting why it's intentional (it is partially documented with the comment). The pattern is acceptable but if more Gitea-specific operations accumulate, it may be worth a GiteaExtension optional interface.

**[MINOR]** The Gitea-specific self-request reviewer block uses a type assertion (client.(*gitea.Adapter)) which leaks provider-specific behavior into main(). This is the established pattern here since Gitea's RequestReviewer isn't part of the vcs.Client interface, but worth documenting why it's intentional (it is partially documented with the comment). The pattern is acceptable but if more Gitea-specific operations accumulate, it may be worth a GiteaExtension optional interface.
if err := giteaAdapter.Underlying().RequestReviewer(ctx, owner, repoName, prNumber, authUser); err != nil {
if err := selfReq.RequestReviewerSelf(ctx, owner, repoName, prNumber, authUser); err != nil {
slog.Warn("could not self-request as reviewer", "user", authUser, "error", err)
sonnet-review-bot commented 2026-05-13 10:55:52 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The supersedeOldReviews function accepts vcsURL string but for the GitHub case it is never used. For the default case in the outer switch it returns an error, so the parameter is effectively only used in the gitea path. This is fine functionally, but could cause confusion — a comment explaining the parameter is gitea-only would help.

**[MINOR]** The `supersedeOldReviews` function accepts `vcsURL string` but for the GitHub case it is never used. For the `default` case in the outer switch it returns an error, so the parameter is effectively only used in the gitea path. This is fine functionally, but could cause confusion — a comment explaining the parameter is gitea-only would help.
sonnet-review-bot commented 2026-05-13 13:49:34 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The type assertion client.(*gitea.Adapter) in the self-request block (Gitea-specific behavior) imports a concrete provider type into main.go, creating a coupling that the vcs.Client abstraction was designed to avoid. The same pattern recurs in supersedeOldReviews. Consider adding an optional interface (e.g., type SelfRequester interface { SelfRequestReview(...) error }) checked at runtime — following the Optional Interfaces pattern used in net/http — so main.go doesn't need to import gitea for this path. That said, given the Gitea-only nature of the feature and the explicit // Gitea-specific comment, this is a pragmatic trade-off rather than a bug.

**[MINOR]** The type assertion `client.(*gitea.Adapter)` in the self-request block (Gitea-specific behavior) imports a concrete provider type into `main.go`, creating a coupling that the `vcs.Client` abstraction was designed to avoid. The same pattern recurs in `supersedeOldReviews`. Consider adding an optional interface (e.g., `type SelfRequester interface { SelfRequestReview(...) error }`) checked at runtime — following the Optional Interfaces pattern used in `net/http` — so `main.go` doesn't need to import `gitea` for this path. That said, given the Gitea-only nature of the feature and the explicit `// Gitea-specific` comment, this is a pragmatic trade-off rather than a bug.
} else {
slog.Debug("self-requested as reviewer", "user", authUser, "pr", prNumber)
sonnet-review-bot commented 2026-05-13 10:30:42 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The supersedeOldReviews function accepts provider as a plain string and then type-asserts client.(*gitea.Adapter) for the gitea case. This couples the supersede logic to the concrete Gitea type rather than going through the interface. The type assertion failure path returns an error (good), but the design creates a hidden coupling: adding a new provider that also needs Gitea-style supersede would require modifying this function. Consider whether DismissReview-style behavior should be part of vcs.Client or whether the Gitea-specific path should be documented as intentionally exceptional.

**[MINOR]** The `supersedeOldReviews` function accepts `provider` as a plain string and then type-asserts `client.(*gitea.Adapter)` for the gitea case. This couples the supersede logic to the concrete Gitea type rather than going through the interface. The type assertion failure path returns an error (good), but the design creates a hidden coupling: adding a new provider that also needs Gitea-style supersede would require modifying this function. Consider whether `DismissReview`-style behavior should be part of `vcs.Client` or whether the Gitea-specific path should be documented as intentionally exceptional.
sonnet-review-bot commented 2026-05-13 14:15:32 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews uses os.Exit(1) on error from the caller, but the function itself returns an error — this is correct and the caller handles it. However, the function signature accepts vcsURL unconditionally even for the GitHub path where it is unused. The comment explains this is intentional for API uniformity, which is acceptable, but it could be a source of confusion in future refactors. A more idiomatic approach might be to embed the URL into a Gitea-specific struct or pass it only to the Gitea path. This is a design nit, not a bug.

**[MINOR]** supersedeOldReviews uses os.Exit(1) on error from the caller, but the function itself returns an error — this is correct and the caller handles it. However, the function signature accepts `vcsURL` unconditionally even for the GitHub path where it is unused. The comment explains this is intentional for API uniformity, which is acceptable, but it could be a source of confusion in future refactors. A more idiomatic approach might be to embed the URL into a Gitea-specific struct or pass it only to the Gitea path. This is a design nit, not a bug.
21
 @@ -563,6 +563,10 @@ func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsUR
}
underlying := giteaAdapter.Underlying()
// Validate vcsURL scheme before embedding in Markdown link (defense-in-depth).
if !strings.HasPrefix(vcsURL, "http://") && !strings.HasPrefix(vcsURL, "https://") {
return fmt.Errorf("supersedeOldReviews: vcsURL must have http or https scheme, got %q", vcsURL)
}
newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(vcsURL, "/"), owner, repoName, prNumber, newReviewID)
for _, oldReview := range oldReviews {
cid, err := underlying.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID)
7
 @@ -757,14 +761,6 @@ func envOrDefaultInt(key string, defaultVal int) int {
return defaultVal
}
func envOrDefaultBool(key string, defaultVal bool) bool {
v := strings.TrimSpace(strings.ToLower(os.Getenv(key)))
if v == "" {
return defaultVal
}
return v == "true" || v == "1" || v == "yes"
}
// validateReviewerName checks that the name contains only safe characters
// for embedding in an HTML comment sentinel ([a-zA-Z0-9_-]).
func validateReviewerName(name string) error {
1
 @@ -858,7 +854,7 @@ func hasSharedToken(reviews []vcs.Review, ownSentinel string) bool {
}
for _, r := range reviews {
if r.User.Login == ownLogin && strings.Contains(r.Body, "<!-- review-bot:") && !strings.Contains(r.Body, ownSentinel) {
slog.Warn("shared token detected — another review-bot role is using the same VCS user",
slog.Warn("shared token detected -- another review-bot role is using the same VCS user",
"sibling_role", extractSentinelName(r.Body), "user", ownLogin)
return true
}
2
cmd/review-bot/main_test.go
-41
View File
2
 @@ -596,47 +596,6 @@ func TestEnvOrDefaultInt(t *testing.T) {
}
}
func TestEnvOrDefaultBool(t *testing.T) {
tests := []struct {
name string
envVal string
setEnv bool
defaultVal bool
want bool
}{
{"unset returns default true", "", false, true, true},
{"unset returns default false", "", false, false, false},
{"true", "true", true, false, true},
{"TRUE", "TRUE", true, false, true},
{"True", "True", true, false, true},
{"1", "1", true, false, true},
{"yes", "yes", true, false, true},
{"YES", "YES", true, false, true},
{"false", "false", true, true, false},
{"0", "0", true, true, false},
{"no", "no", true, true, false},
{"random string", "random", true, true, false},
{"empty string returns default", "", true, true, true},
{"whitespace true", " true ", true, false, true},
}
for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
envKey := "TEST_ENV_BOOL_" + strings.ReplaceAll(tc.name, " ", "_")
if tc.setEnv {
os.Setenv(envKey, tc.envVal)
defer os.Unsetenv(envKey)
} else {
os.Unsetenv(envKey)
}
got := envOrDefaultBool(envKey, tc.defaultVal)
if got != tc.want {
t.Errorf("envOrDefaultBool(%q, %v) = %v, want %v", tc.envVal, tc.defaultVal, got, tc.want)
}
})
}
}
func TestExtractSentinelName_EdgeCases(t *testing.T) {
tests := []struct {
body string
gitea/adapter.go
+7
View File
@@ -16,6 +16,7 @@ type Adapter struct {
// Compile-time interface conformance assertion.
var _ vcs.Client = (*Adapter)(nil)
var _ vcs.ReviewerSelfRequester = (*Adapter)(nil)
// NewAdapter creates a new Adapter wrapping the given gitea Client.
func NewAdapter(client *Client) *Adapter {
1
 @@ -230,3 +231,9 @@ func (a *Adapter) DismissReview(ctx context.Context, owner, repo string, number
func (a *Adapter) GetAuthenticatedUser(ctx context.Context) (string, error) {
return a.client.GetAuthenticatedUser(ctx)
}
sonnet-review-bot commented 2026-05-13 16:34:41 +00:00
Review
Copy Link
Copy Source

[NIT] The comment block and both var _ vcs.ReviewerSelfRequester = (*Adapter)(nil) and var _ vcs.ReviewSuperseder = (*Adapter)(nil) are declared in the same file but separated by the SupersedeReviews implementation. The compile-time assertion for ReviewerSelfRequester is near line 19, while the one for ReviewSuperseder is mid-file (line 233). Convention (per patterns/style.md and stdlib) is to place all var _ Interface = ... assertions together near the top of the file for discoverability.

**[NIT]** The comment block and both `var _ vcs.ReviewerSelfRequester = (*Adapter)(nil)` and `var _ vcs.ReviewSuperseder = (*Adapter)(nil)` are declared in the same file but separated by the `SupersedeReviews` implementation. The compile-time assertion for `ReviewerSelfRequester` is near line 19, while the one for `ReviewSuperseder` is mid-file (line 233). Convention (per patterns/style.md and stdlib) is to place all `var _ Interface = ...` assertions together near the top of the file for discoverability.
// RequestReviewerSelf adds the given user as a requested reviewer on a pull request.
// This implements vcs.ReviewerSelfRequester for the Gitea adapter.
func (a *Adapter) RequestReviewerSelf(ctx context.Context, owner, repo string, number int, user string) error {
return a.client.RequestReviewer(ctx, owner, repo, number, user)
}
gitea/adapter_test.go
+22
View File
@@ -386,3 +386,25 @@ func TestAdapter_GetFileContent_RefRouting(t *testing.T) {
t.Errorf("GetFileContent(ref=\"abc123\") = %q, want %q", got, "content-at-ref")
}
}
func TestAdapter_RequestReviewerSelf(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.Method != http.MethodPost {
t.Errorf("expected POST, got %s", r.Method)
}
expected := "/api/v1/repos/owner/repo/pulls/5/requested_reviewers"
if r.URL.Path != expected {
t.Errorf("path = %q, want %q", r.URL.Path, expected)
}
w.WriteHeader(http.StatusCreated)
}))
defer server.Close()
client := gitea.NewClient(server.URL, "token")
adapter := gitea.NewAdapter(client)
err := adapter.RequestReviewerSelf(context.Background(), "owner", "repo", 5, "bot-user")
if err != nil {
t.Fatalf("RequestReviewerSelf() error = %v", err)
}
}
vcs/interfaces.go
+8
View File
@@ -41,3 +41,11 @@ type Client interface {
Reviewer
Identity
}
// ReviewerSelfRequester is an optional interface implemented by adapters that support
// requesting the authenticated user as a reviewer on a pull request. This is used for
// Gitea-specific behavior (ensuring the bot appears in required-reviewer checks).
// Consumers should use interface assertion: if sr, ok := client.(ReviewerSelfRequester); ok { ... }
type ReviewerSelfRequester interface {
RequestReviewerSelf(ctx context.Context, owner, repo string, number int, user string) error
}