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
-2
1 changed files with 0 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit c5bc807d2c - Show all commits
cmd/review-bot/main.go
-2
View File
62
 @@ -530,7 +530,6 @@ func verdictToEvent(verdict string) vcs.ReviewEvent {
}
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.
}
// supersedeOldReviews marks old reviews as superseded.
// supersedeOldReviews marks prior reviews as superseded so only the latest review is visible.
// For GitHub: dismisses old reviews (vcsURL is unused in this path).
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.
// For Gitea: edits the review body with a link to the new review and resolves inline comments.
5
 @@ -557,7 +556,6 @@ func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsUR
return fmt.Errorf("supersedeOldReviews: unsupported provider %q", provider)
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 type assertion client.(*gitea.Adapter) inside supersedeOldReviews couples this package-level function to the gitea package's internal concrete type. The comment acknowledges this is intentional and guarded, but if a decorator/wrapper is ever added around the adapter, this silently breaks. A cleaner long-term design would be to add a Gitea() GiteaUnderlying interface method or pass underlying directly rather than asserting. This is acceptable for now given the comment explains the rationale, but worth tracking.

**[MINOR]** The type assertion `client.(*gitea.Adapter)` inside `supersedeOldReviews` couples this package-level function to the `gitea` package's internal concrete type. The comment acknowledges this is intentional and guarded, but if a decorator/wrapper is ever added around the adapter, this silently breaks. A cleaner long-term design would be to add a `Gitea() GiteaUnderlying` interface method or pass `underlying` directly rather than asserting. This is acceptable for now given the comment explains the rationale, but worth tracking.
}
sonnet-review-bot commented 2026-05-13 15:02:31 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] supersedeOldReviews uses a provider string parameter and a switch that ultimately does a type assertion (client.(*gitea.Adapter)) for the Gitea path. This leaks a concrete type from the gitea package into what is otherwise a provider-agnostic function, and creates a tight coupling that will silently break if the adapter is ever wrapped (as the comment acknowledges). A cleaner design would be an optional interface (e.g., vcs.OldReviewSuperseder) that the Gitea adapter implements, similar to how ReviewerSelfRequester was handled, eliminating both the string switch and the type assertion.

**[MINOR]** `supersedeOldReviews` uses a provider string parameter and a `switch` that ultimately does a type assertion (`client.(*gitea.Adapter)`) for the Gitea path. This leaks a concrete type from the `gitea` package into what is otherwise a provider-agnostic function, and creates a tight coupling that will silently break if the adapter is ever wrapped (as the comment acknowledges). A cleaner design would be an optional interface (e.g., `vcs.OldReviewSuperseder`) that the Gitea adapter implements, similar to how `ReviewerSelfRequester` was handled, eliminating both the string switch and the type assertion.
sonnet-review-bot commented 2026-05-13 15:02:31 +00:00
Outdated
Review
Copy Link
Copy Source

[MINOR] The vcsURL parameter is accepted by supersedeOldReviews even for the GitHub path (where it is unused) with the stated goal of keeping the signature uniform. However this means the GitHub path silently ignores a potentially user-visible parameter with no validation. If vcsURL is an empty string and the provider is github, the Gitea URL-scheme validation block is never reached, which is fine — but a caller could accidentally pass a bad vcsURL for GitHub and receive no diagnostic. A doc comment clarifying this intentional skip would help future readers.

**[MINOR]** The `vcsURL` parameter is accepted by `supersedeOldReviews` even for the GitHub path (where it is unused) with the stated goal of keeping the signature uniform. However this means the GitHub path silently ignores a potentially user-visible parameter with no validation. If `vcsURL` is an empty string and the provider is `github`, the Gitea URL-scheme validation block is never reached, which is fine — but a caller could accidentally pass a bad vcsURL for GitHub and receive no diagnostic. A doc comment clarifying this intentional skip would help future readers.
// The type assertion below is guaranteed to succeed: the caller's provider switch
// ensures we only reach this point when provider == "gitea", and the gitea provider
// always constructs a *gitea.Adapter. The !ok branch guards against future refactors
10