feat(cmd): wire --provider and --base-url flags into CLI (Phase 5) #106
@@ -169,6 +169,8 @@ func main() {
|
||||
ghBaseURL = "https://api.github.com"
|
||||
}
|
||||
|
|
||||
client = github.NewClient(*reviewerToken, ghBaseURL)
|
||||
sonnet-review-bot
commented
[MINOR] The switch for initializing **[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] 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)
|
||||
|
sonnet-review-bot
commented
[NIT] The **[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.
|
||||
}
|
||||
slog.Info("VCS client initialized", "provider", *provider)
|
||||
|
[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
[MINOR] The **[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 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.
|
||||
@@ -498,7 +500,10 @@ func main() {
|
||||
|
||||
// Supersede all old reviews
|
||||
if len(oldReviews) > 0 {
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `supersedeOldReviews` function uses a type assertion `client.(*gitea.Adapter)` in what is supposed to be a provider-dispatch function. This leaks the concrete Gitea type into logic that should be abstracted by the `vcs.Client` interface. The comment acknowledges this is 'guaranteed to succeed', but it's a coupling smell: `cmd/main.go` now imports and depends on `gitea.Adapter` internals for a Gitea-specific code path. This is acceptable for now given the Gitea-specific `EditComment`/`ResolveComment`/`GetTimelineReviewCommentIDForReview` calls have no VCS-agnostic equivalent, but it should be noted as technical debt if additional providers are added.
|
||||
supersedeOldReviews(ctx, client, *provider, *vcsURL, owner, repoName, prNumber, oldReviews, posted.ID, sentinel)
|
||||
if err := supersedeOldReviews(ctx, client, *provider, *vcsURL, owner, repoName, prNumber, oldReviews, posted.ID, sentinel); err != nil {
|
||||
slog.Error("failed to supersede old reviews", "error", err)
|
||||
os.Exit(1)
|
||||
|
sonnet-review-bot
commented
[MINOR] In **[MINOR]** In `supersedeOldReviews`, the GitHub case iterates over `oldReviews` and logs a warning on `DismissReview` failure but continues without returning an error. The function still returns `nil` after the loop even if all dismissals failed. This is intentional (best-effort dismissal) but the caller in `main()` calls `os.Exit(1)` on any error returned — so failures here are silently swallowed. Consider either returning a joined error for all failures or at minimum documenting the best-effort semantics in the function doc comment.
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -517,7 +522,7 @@ func verdictToEvent(verdict string) vcs.ReviewEvent {
|
||||
// supersedeOldReviews marks old reviews as superseded.
|
||||
// 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) {
|
||||
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" {
|
||||
for _, old := range oldReviews {
|
||||
if err := client.DismissReview(ctx, owner, repoName, prNumber, old.ID, "Superseded by new review"); err != nil {
|
||||
@@ -526,14 +531,13 @@ func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsUR
|
||||
slog.Info("dismissed old review", "review_id", old.ID, "new_review_id", newReviewID, "pr", prNumber)
|
||||
}
|
||||
}
|
||||
return
|
||||
return nil
|
||||
|
sonnet-review-bot
commented
[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.
|
||||
}
|
||||
|
||||
// Gitea: existing EditComment + ResolveComment flow
|
||||
|
sonnet-review-bot
commented
[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.
|
||||
giteaAdapter, ok := client.(*gitea.Adapter)
|
||||
if !ok {
|
||||
|
rodin marked this conversation as resolved
Outdated
sonnet-review-bot
commented
[MINOR] The **[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.
|
||||
slog.Error("expected gitea.Adapter for gitea provider")
|
||||
os.Exit(1)
|
||||
return fmt.Errorf("expected gitea.Adapter for gitea provider, got %T", client)
|
||||
}
|
||||
underlying := giteaAdapter.Underlying()
|
||||
|
||||
@@ -576,6 +580,7 @@ func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsUR
|
||||
slog.Warn("some inline comments could not be resolved", "review_id", oldReview.ID, "failed", failed, "pr", prNumber)
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// fetchFileContext fetches the full content of modified files from the PR branch.
|
||||
@@ -847,7 +852,21 @@ func extractSentinelName(body string) string {
|
||||
if end < 0 {
|
||||
return "unknown"
|
||||
}
|
||||
return rest[:end]
|
||||
name := rest[:end]
|
||||
// Sanitize: strip control characters to prevent log injection.
|
||||
name = strings.Map(func(r rune) rune {
|
||||
if r < 0x20 || r == 0x7f {
|
||||
return -1
|
||||
}
|
||||
return r
|
||||
}, name)
|
||||
if len(name) > 64 {
|
||||
name = name[:64]
|
||||
}
|
||||
if name == "" {
|
||||
return "unknown"
|
||||
}
|
||||
return name
|
||||
}
|
||||
|
||||
// findOwnReview locates the most recent non-superseded review matching the sentinel.
|
||||
|
||||
@@ -141,7 +141,9 @@ func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int
|
||||
return allReviews, nil
|
||||
}
|
||||
|
||||
// DeleteReview deletes a review from a pull request.
|
||||
// DeleteReview permanently deletes a review from a pull request.
|
||||
// Use DismissReview instead when the review should remain visible but marked as dismissed
|
||||
// (e.g., superseding an outdated review while preserving history).
|
||||
func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
|
||||
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d",
|
||||
|
rodin marked this conversation as resolved
Outdated
sonnet-review-bot
commented
[NIT] The **[NIT]** The `ListReviews` page-limit warning comment (`// NOTE: This warning only fires when...`) is placed after the `if page == maxReviewPages` block but before the loop's closing brace. The comment is correct but its placement after the action it describes is mildly confusing. Moving it above the `if` block would improve readability.
|
||||
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
|
||||
@@ -185,7 +187,7 @@ func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) {
|
||||
|
||||
// doJSONRequest performs an HTTP request with a JSON body and returns the response body.
|
||||
// It handles HTTPS validation, authentication, and response reading.
|
||||
func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, payload interface{}) ([]byte, error) {
|
||||
func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, payload any) ([]byte, error) {
|
||||
const maxErrorBodyBytes = 4 * 1024
|
||||
|
||||
jsonBody, err := json.Marshal(payload)
|
||||
|
||||
@@ -10,7 +10,6 @@ func FormatMarkdown(result *ReviewResult, reviewerName string) string {
|
||||
return FormatMarkdownWithDisplay(result, reviewerName, reviewerName)
|
||||
}
|
||||
|
||||
|
||||
// FormatMarkdownWithDisplay formats a ReviewResult with separate display name and sentinel name.
|
||||
// Note: displayName is not HTML-escaped as Gitea sanitizes rendered Markdown.
|
||||
// Persona display names are controlled by repo owners (trusted input).
|
||||
|
||||
@@ -98,7 +98,6 @@ func TestFormatMarkdown_SpecialChars(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
func TestFormatMarkdown_Sentinel(t *testing.T) {
|
||||
|
sonnet-review-bot
commented
[NIT] Similar to the formatter.go nit — a blank line artifact remains after removing **[NIT]** Similar to the formatter.go nit — a blank line artifact remains after removing `TestGiteaEvent`. Minor formatting issue.
|
||||
result := &ReviewResult{
|
||||
Verdict: "APPROVE",
|
||||
|
||||
[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.
[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 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.