feat(cmd): wire --provider and --base-url flags into CLI (Phase 5) #106
@@ -2,7 +2,6 @@ package main
|
||||
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"flag"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
@@ -85,16 +84,8 @@ func main() {
|
||||
aicoreAPIURL := flag.String("aicore-api-url", envOrDefault("AICORE_API_URL", ""), "SAP AI Core API URL (for provider=aicore)")
|
||||
|
|
||||
aicoreResourceGroup := flag.String("aicore-resource-group", envOrDefault("AICORE_RESOURCE_GROUP", "default"), "SAP AI Core resource group (for provider=aicore)")
|
||||
|
rodin marked this conversation as resolved
sonnet-review-bot
commented
[NIT] The **[NIT]** The `--gitea-url` alias comment block is long (12 lines) and explains a subtle flag.StringVar trick. Consider extracting the alias registration to a small helper or at minimum shortening the comment to the key invariant: `flag.StringVar shares the pointer, so whichever flag is set last wins.` The current comment is accurate but its length makes it easy to miss the ORDERING constraint.
|
||||
|
||||
// Register --gitea-url as a backward-compatible alias for --vcs-url.
|
||||
// StringVar shares the *string pointer with vcsURL, so whichever flag is
|
||||
// set last by flag.Parse wins — both point to the same underlying value.
|
||||
// NOTE: If a user passes both --vcs-url and --gitea-url, the last one on
|
||||
// the command line takes effect (standard flag package behavior). This is
|
||||
// acceptable since --gitea-url is deprecated and both serve the same purpose.
|
||||
//
|
||||
// ORDERING: This must remain AFTER vcsURL's flag.String declaration and BEFORE
|
||||
// flag.Parse(). The *vcsURL dereference captures the env-var-resolved default
|
||||
// at registration time; moving flag.Parse() above this line would break it.
|
||||
// Backward-compatible alias: --gitea-url shares vcsURL's pointer (last flag wins).
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `--gitea-url` alias registration reads `*vcsURL` at registration time (before `flag.Parse()`). At that point `*vcsURL` holds the default value from `envOrDefault(...)`, so the alias default is correct. However, if a user passes `--vcs-url foo` on the command line, the alias's default is stale — only the variable is shared, not a live default. This is acceptable because the flag package updates the pointed-to variable via `flag.StringVar`, so any value written by either `--vcs-url` or `--gitea-url` goes to the same `*string`. But the help text for `--gitea-url` will show the env-var default rather than reflecting `--vcs-url`'s current value, which is mildly confusing. Consider a brief comment explaining the aliasing semantics.
sonnet-review-bot
commented
[NIT] The comment on the **[NIT]** The comment on the `--gitea-url` alias registration says "The *vcsURL dereference captures the env-var-resolved default at registration time". This is accurate but the behaviour when both `--vcs-url` and `--gitea-url` are passed (last one wins) is subtle enough that a brief integration test would provide more confidence than a comment alone.
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `--gitea-url` backward-compatible alias is registered with `flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...)` where `*vcsURL` is the default value captured at the moment of the call — before `flag.Parse()`. This is correct as written because `*vcsURL` is still the default string at that point, but it creates a subtle ordering dependency: if `vcsURL` ever acquired a non-default value between its declaration and this `flag.StringVar` call (e.g., if any code ran between them), the alias would get a stale default. The comment acknowledges this (`Must stay after vcsURL declaration and before flag.Parse()`), but the fragility is worth noting. A cleaner pattern would be `flag.StringVar(vcsURL, "gitea-url", "", "...")` or extracting the default value to a named constant.
|
||||
// Must stay after vcsURL declaration and before flag.Parse().
|
||||
|
sonnet-review-bot
commented
[NIT] The **[NIT]** The `--gitea-url` alias registration uses `*vcsURL` as the default value at registration time. This works correctly because flag parsing hasn't happened yet, so `*vcsURL` holds the env-var-resolved default. However this is subtle — if the evaluation order ever changed (e.g., someone moves `flag.Parse()` above this line), the alias would silently use an empty string default instead. A brief comment noting this dependency on registration-before-parse order would help future readers.
|
||||
flag.StringVar(vcsURL, "gitea-url", *vcsURL, "Deprecated: use --vcs-url instead")
|
||||
|
||||
flag.Parse()
|
||||
@@ -110,10 +101,8 @@ func main() {
|
||||
slog.Info("review-bot starting", "version", version)
|
||||
gpt-review-bot
commented
[NIT] Flag description for --base-url could mention GitHub Enterprise explicitly (e.g., "GitHub API base URL for github.com or GitHub Enterprise"), though this is already implied by tests and usage. **[NIT]** Flag description for --base-url could mention GitHub Enterprise explicitly (e.g., "GitHub API base URL for github.com or GitHub Enterprise"), though this is already implied by tests and usage.
sonnet-review-bot
commented
[NIT] The **[NIT]** The `envOrDefaultBool` function is defined and tested but never called in main.go (no usages in the diff or full file). It may be used elsewhere or kept for future use, but if unused it could be removed to keep the package clean.
|
||||
|
||||
// Validate VCS provider
|
||||
switch *provider {
|
||||
case "gitea", "github":
|
||||
// valid
|
||||
default:
|
||||
vcsProvider := vcs.VCSProvider(*provider)
|
||||
if !vcsProvider.Valid() {
|
||||
fmt.Fprintf(os.Stderr, "Error: invalid --provider %q (valid: gitea, github)\n", *provider)
|
||||
os.Exit(1)
|
||||
}
|
||||
@@ -126,7 +115,7 @@ func main() {
|
||||
os.Exit(1)
|
||||
}
|
||||
// --vcs-url is required only for gitea provider
|
||||
if *provider == "gitea" && *vcsURL == "" {
|
||||
if vcsProvider == vcs.ProviderGitea && *vcsURL == "" {
|
||||
fmt.Fprintf(os.Stderr, "Error: --vcs-url (or --gitea-url) is required for provider=gitea\n")
|
||||
os.Exit(1)
|
||||
}
|
||||
@@ -169,18 +158,16 @@ func main() {
|
||||
|
||||
// Initialize VCS client
|
||||
var client vcs.Client
|
||||
switch *provider {
|
||||
case "gitea":
|
||||
switch vcsProvider {
|
||||
case vcs.ProviderGitea:
|
||||
giteaClient := gitea.NewClient(*vcsURL, *reviewerToken)
|
||||
client = gitea.NewAdapter(giteaClient)
|
||||
case "github":
|
||||
ghBaseURL := *baseURL
|
||||
if ghBaseURL == "" {
|
||||
ghBaseURL = "https://api.github.com"
|
||||
}
|
||||
client = github.NewClient(*reviewerToken, ghBaseURL)
|
||||
case vcs.ProviderGithub:
|
||||
client = github.NewClient(*reviewerToken, *baseURL)
|
||||
default:
|
||||
|
rodin marked this conversation as resolved
Outdated
sonnet-review-bot
commented
[MINOR] The default case in the VCS client initialization switch ( **[MINOR]** The default case in the VCS client initialization switch (`default: fmt.Fprintf(os.Stderr, ...) ; os.Exit(1)`) is unreachable dead code: the provider is already validated by the earlier switch at line 109-114 which also exits on invalid values. This is harmless but adds noise.
|
||||
panic("unreachable: provider validation should have caught " + vcsProvider.String())
|
||||
}
|
||||
slog.Info("VCS client initialized", "provider", *provider)
|
||||
slog.Info("VCS client initialized", "provider", vcsProvider)
|
||||
|
sonnet-review-bot
commented
[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
[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
[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.
|
||||
|
||||
|
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.
|
||||
// Initialize LLM client
|
||||
llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel)
|
||||
|
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.
|
||||
@@ -506,11 +493,15 @@ func main() {
|
||||
}
|
||||
slog.Info("review posted", "review_id", posted.ID, "user", posted.User.Login, "pr", prNumber)
|
||||
|
sonnet-review-bot
commented
[MINOR] supersedeOldReviews accepts provider as a plain string rather than a typed constant/enum. The valid provider values are scattered: validated at entry (switch *provider) and re-evaluated here. A typed VCSProvider string type would make the contract explicit and allow the compiler to help catch new provider additions that aren't handled in both switches. **[MINOR]** supersedeOldReviews accepts provider as a plain string rather than a typed constant/enum. The valid provider values are scattered: validated at entry (switch *provider) and re-evaluated here. A typed VCSProvider string type would make the contract explicit and allow the compiler to help catch new provider additions that aren't handled in both switches.
|
||||
|
||||
|
sonnet-review-bot
commented
[NIT] The type assertion **[NIT]** The type assertion `client.(*gitea.Adapter)` for the self-reviewer path is necessary but creates a direct dependency on a concrete type inside main. The comment explaining this is good. Consider whether `RequestReviewer` belongs on a provider-specific optional interface (similar to the `http.Flusher` optional interface pattern) for cleaner future extensibility — e.g., `type SelfReviewer interface { RequestReviewerSelf(ctx, owner, repo string, pr int, user string) error }`. Not blocking, just worth noting for the next phase.
|
||||
// Supersede all old reviews
|
||||
// Supersede all old reviews via optional interface
|
||||
if len(oldReviews) > 0 {
|
||||
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)
|
||||
if superseder, ok := client.(vcs.ReviewSuperseder); ok {
|
||||
if err := superseder.SupersedeReviews(ctx, owner, repoName, prNumber, oldReviews, posted.ID, *vcsURL, sentinel); err != nil {
|
||||
|
sonnet-review-bot
commented
[MINOR] When **[MINOR]** When `client.(vcs.ReviewSuperseder)` returns false and `len(oldReviews) > 0`, the code logs a warning and silently skips superseding. For the GitHub provider, `github.Client` implements `ReviewSuperseder`, so this path is unreachable. For Gitea, `gitea.Adapter` also implements it. The warning will therefore never fire for any configured provider, making it dead code in practice. It's harmless, but the `slog.Warn` may mislead operators if a future provider is wired without implementing the interface — they'd see a warning but no error. Consider whether this should be an error for unknown cases.
|
||||
slog.Error("failed to supersede old reviews", "error", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
|
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.
|
||||
} else {
|
||||
slog.Warn("provider does not support review superseding", "provider", vcsProvider)
|
||||
}
|
||||
|
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.
|
||||
}
|
||||
}
|
||||
@@ -527,88 +518,6 @@ func verdictToEvent(verdict string) vcs.ReviewEvent {
|
||||
}
|
||||
}
|
||||
|
||||
|
gpt-review-bot
commented
[MINOR] supersedeOldReviews requires a concrete type assertion to *gitea.Adapter for Gitea-specific operations. This is acceptable but couples main to a concrete type; consider extending the vcs.Client (or a sub-interface) with provider-agnostic capabilities for superseding/resolve flows to avoid type assertions. **[MINOR]** supersedeOldReviews requires a concrete type assertion to *gitea.Adapter for Gitea-specific operations. This is acceptable but couples main to a concrete type; consider extending the vcs.Client (or a sub-interface) with provider-agnostic capabilities for superseding/resolve flows to avoid type assertions.
|
||||
// supersedeOldReviews marks prior reviews as superseded so only the latest review is visible.
|
||||
// For GitHub: dismisses old reviews (vcsURL is unused in this path).
|
||||
// For Gitea: edits the review body with a link to the new review and resolves inline comments.
|
||||
//
|
||||
// The vcsURL parameter is only used in the Gitea path to construct review permalink URLs;
|
||||
// it is accepted unconditionally to keep the function signature uniform across providers.
|
||||
func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsURL, owner, repoName string, prNumber int, oldReviews []vcs.Review, newReviewID int64, sentinel string) error {
|
||||
switch provider {
|
||||
case "github":
|
||||
// Best-effort dismissal: attempt all reviews, join any errors.
|
||||
var errs []error
|
||||
for _, old := range oldReviews {
|
||||
if err := client.DismissReview(ctx, owner, repoName, prNumber, old.ID, "Superseded by new review"); err != nil {
|
||||
slog.Warn("failed to dismiss review", "id", old.ID, "error", err)
|
||||
errs = append(errs, fmt.Errorf("dismiss review %d: %w", old.ID, err))
|
||||
} else {
|
||||
slog.Info("dismissed old review", "review_id", old.ID, "new_review_id", newReviewID, "pr", prNumber)
|
||||
}
|
||||
}
|
||||
return errors.Join(errs...)
|
||||
case "gitea":
|
||||
// Continue to Gitea-specific logic below the switch.
|
||||
default:
|
||||
return fmt.Errorf("supersedeOldReviews: unsupported provider %q", provider)
|
||||
}
|
||||
|
||||
// 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
|
||||
// (e.g. wrapping the adapter in a decorator) that would silently break this path.
|
||||
giteaAdapter, ok := client.(*gitea.Adapter)
|
||||
if !ok {
|
||||
return fmt.Errorf("expected gitea.Adapter for gitea provider, got %T", client)
|
||||
}
|
||||
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)
|
||||
if err != nil {
|
||||
slog.Warn("could not find comment ID for old review", "review_id", oldReview.ID, "error", err)
|
||||
continue
|
||||
}
|
||||
supersededBody := buildSupersededBody(oldReview.Body, oldReview.CommitID, newReviewURL, sentinel)
|
||||
if err := underlying.EditComment(ctx, owner, repoName, cid, supersededBody); err != nil {
|
||||
slog.Warn("could not mark old review as superseded", "review_id", oldReview.ID, "comment_id", cid, "error", err)
|
||||
continue
|
||||
}
|
||||
slog.Info("marked old review as superseded", "review_id", oldReview.ID, "new_review_id", newReviewID, "pr", prNumber)
|
||||
|
||||
// Resolve old review's inline comments
|
||||
oldComments, err := underlying.ListReviewComments(ctx, owner, repoName, prNumber, oldReview.ID)
|
||||
if err != nil {
|
||||
slog.Warn("could not list old review comments for resolution", "review_id", oldReview.ID, "error", err)
|
||||
continue
|
||||
}
|
||||
resolved, failed := 0, 0
|
||||
for _, c := range oldComments {
|
||||
if c.ID == 0 {
|
||||
continue
|
||||
}
|
||||
if err := underlying.ResolveComment(ctx, owner, repoName, c.ID); err != nil {
|
||||
slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err)
|
||||
failed++
|
||||
} else {
|
||||
resolved++
|
||||
}
|
||||
}
|
||||
if resolved > 0 {
|
||||
slog.Info("resolved old inline comments", "review_id", oldReview.ID, "count", resolved, "pr", prNumber)
|
||||
}
|
||||
if failed > 0 {
|
||||
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.
|
||||
func fetchFileContext(ctx context.Context, client vcs.PRReader, owner, repo, ref string, files []vcs.ChangedFile) string {
|
||||
var sb strings.Builder
|
||||
@@ -636,7 +545,7 @@ func fetchFileContext(ctx context.Context, client vcs.PRReader, owner, repo, ref
|
||||
// patternsRepo is comma-separated list of owner/name repos.
|
||||
// patternsFiles is comma-separated list of file paths or directories.
|
||||
// If a path ends with / or is a directory, all files within it are fetched recursively.
|
||||
// If patternsFiles is empty, all files from the repo root are fetched.
|
||||
// Empty entries in patternsFiles are skipped (no implicit repo-root fetch).
|
||||
|
gpt-review-bot
commented
[MINOR] supersedeOldReviews asserts client.(*gitea.Adapter), coupling main to the gitea package. Consider an optional interface (e.g., a GiteaSuperseder) to avoid a concrete type assertion and keep main fully provider-agnostic. **[MINOR]** supersedeOldReviews asserts client.(*gitea.Adapter), coupling main to the gitea package. Consider an optional interface (e.g., a GiteaSuperseder) to avoid a concrete type assertion and keep main fully provider-agnostic.
|
||||
func fetchPatterns(ctx context.Context, client vcs.FileReader, patternsRepo, patternsFiles string) string {
|
||||
var sb strings.Builder
|
||||
|
||||
@@ -812,30 +721,7 @@ func validateWorkspacePath(path, pathName string) (string, error) {
|
||||
return resolvedPath, nil
|
||||
|
sonnet-review-bot
commented
[NIT] There is a dangling comment fragment: **[NIT]** There is a dangling comment fragment: `// with collapsed original content and the commit it was evaluated against.` — this is the second half of the `buildSupersededBody` doc comment that was removed when the function moved to `gitea/adapter.go`. The first line of the doc comment (`// buildSupersededBody creates the body for a superseded review: struck-through banner`) was deleted but this trailing line was left orphaned. It should be removed.
|
||||
}
|
||||
|
||||
// buildSupersededBody creates the body for a superseded review: struck-through banner
|
||||
// with collapsed original content and the commit it was evaluated against.
|
||||
func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string) string {
|
||||
shortSHA := commitSHA
|
||||
if len(shortSHA) > 8 {
|
||||
shortSHA = shortSHA[:8]
|
||||
}
|
||||
var sb strings.Builder
|
||||
sb.WriteString("~~Original review~~\n\n")
|
||||
sb.WriteString("**Superseded** \u2014 [see current review](")
|
||||
sb.WriteString(newReviewURL)
|
||||
sb.WriteString(") for up-to-date findings.\n\n")
|
||||
if shortSHA != "" {
|
||||
sb.WriteString("<details><summary>Previous findings (commit ")
|
||||
sb.WriteString(shortSHA)
|
||||
sb.WriteString(")</summary>\n\n")
|
||||
} else {
|
||||
sb.WriteString("<details><summary>Previous findings</summary>\n\n")
|
||||
}
|
||||
sb.WriteString(originalBody)
|
||||
sb.WriteString("\n\n</details>\n\n")
|
||||
sb.WriteString(sentinel)
|
||||
return sb.String()
|
||||
}
|
||||
|
||||
// hasSharedToken detects if another review-bot role posted under the same
|
||||
// VCS user. This indicates misconfiguration where two roles share a token
|
||||
|
||||
@@ -162,54 +162,6 @@ func makeReview(id int64, login, state string, stale bool, body string) vcs.Revi
|
||||
}
|
||||
}
|
||||
|
||||
func TestBuildSupersededBody(t *testing.T) {
|
||||
original := "# Review\n\nLooks good.\n\n<!-- review-bot:sonnet -->"
|
||||
sentinel := "<!-- review-bot:sonnet -->"
|
||||
newURL := "https://gitea.example.com/owner/repo/pulls/1#pullrequestreview-99"
|
||||
|
||||
result := buildSupersededBody(original, "abcdef1234567890", newURL, sentinel)
|
||||
|
||||
// Should contain the struck-through banner
|
||||
if !strings.Contains(result, "~~Original review~~") {
|
||||
t.Error("missing struck-through banner")
|
||||
}
|
||||
// Should contain superseded notice with link
|
||||
if !strings.Contains(result, "**Superseded**") {
|
||||
t.Error("missing superseded notice")
|
||||
}
|
||||
if !strings.Contains(result, "[see current review]("+newURL+")") {
|
||||
t.Error("missing link to new review")
|
||||
}
|
||||
// Should contain collapsed original
|
||||
if !strings.Contains(result, "<details>") {
|
||||
t.Error("missing details/collapse")
|
||||
}
|
||||
// Should contain short commit SHA
|
||||
if !strings.Contains(result, "abcdef12") {
|
||||
t.Error("missing short SHA")
|
||||
}
|
||||
// Should NOT contain full SHA
|
||||
if strings.Contains(result, "abcdef1234567890") {
|
||||
t.Error("should truncate SHA to 8 chars")
|
||||
}
|
||||
// Should contain the original body inside details
|
||||
if !strings.Contains(result, original) {
|
||||
t.Error("original body not preserved in collapsed section")
|
||||
}
|
||||
// Should end with sentinel
|
||||
if !strings.Contains(result, sentinel) {
|
||||
t.Error("missing sentinel")
|
||||
}
|
||||
}
|
||||
|
||||
func TestBuildSupersededBodyShortSHA(t *testing.T) {
|
||||
// Short SHA should pass through without panic
|
||||
result := buildSupersededBody("body", "abc", "https://example.com/review", "<!-- review-bot:x -->")
|
||||
if !strings.Contains(result, "abc") {
|
||||
t.Error("short SHA not preserved")
|
||||
}
|
||||
}
|
||||
|
||||
func TestHasSharedToken(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
|
||||
@@ -3,6 +3,8 @@ package gitea
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"strings"
|
||||
|
||||
"gitea.weiker.me/rodin/review-bot/vcs"
|
||||
)
|
||||
@@ -237,3 +239,76 @@ func (a *Adapter) GetAuthenticatedUser(ctx context.Context) (string, error) {
|
||||
func (a *Adapter) RequestReviewerSelf(ctx context.Context, owner, repo string, number int, user string) error {
|
||||
return a.client.RequestReviewer(ctx, owner, repo, number, user)
|
||||
}
|
||||
|
||||
// Compile-time interface conformance assertion for ReviewSuperseder.
|
||||
var _ vcs.ReviewSuperseder = (*Adapter)(nil)
|
||||
|
||||
// SupersedeReviews marks prior reviews as superseded by editing their body with a
|
||||
// link to the new review and resolving their inline comments. This is Gitea-specific
|
||||
// behavior that has no GitHub equivalent (GitHub uses DismissReview instead).
|
||||
//
|
||||
// baseURL is the Gitea instance URL used to construct review permalink URLs.
|
||||
// sentinel is the HTML comment sentinel that identifies reviews belonging to this reviewer.
|
||||
func (a *Adapter) SupersedeReviews(ctx context.Context, owner, repo string, prNumber int, oldReviews []vcs.Review, newReviewID int64, baseURL, sentinel string) error {
|
||||
// Validate baseURL scheme before embedding in Markdown link (defense-in-depth).
|
||||
if !strings.HasPrefix(baseURL, "http://") && !strings.HasPrefix(baseURL, "https://") {
|
||||
return fmt.Errorf("SupersedeReviews: baseURL must have http or https scheme, got %q", baseURL)
|
||||
}
|
||||
|
||||
|
sonnet-review-bot
commented
[NIT] **[NIT]** `SupersedeReviews` silently ignores resolution errors for individual inline comments via `_ = underlying.ResolveComment(...)`. The old code in `main.go` tracked `resolved` and `failed` counts and logged them. The new adapter drops that observability entirely. Consider at minimum logging at debug level on resolution failure, matching the pattern used for `EditComment` failures a few lines above.
|
||||
underlying := a.client
|
||||
|
||||
newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d",
|
||||
strings.TrimRight(baseURL, "/"), owner, repo, prNumber, newReviewID)
|
||||
|
||||
for _, oldReview := range oldReviews {
|
||||
cid, err := underlying.GetTimelineReviewCommentIDForReview(ctx, owner, repo, prNumber, oldReview.ID)
|
||||
if err != nil {
|
||||
slog.Warn("could not find comment ID for old review", "review_id", oldReview.ID, "error", err)
|
||||
continue
|
||||
}
|
||||
supersededBody := buildSupersededBody(oldReview.Body, oldReview.CommitID, newReviewURL, sentinel)
|
||||
if err := underlying.EditComment(ctx, owner, repo, cid, supersededBody); err != nil {
|
||||
slog.Warn("could not mark old review as superseded", "review_id", oldReview.ID, "error", err)
|
||||
continue
|
||||
}
|
||||
|
||||
// Resolve old review's inline comments
|
||||
oldComments, err := underlying.ListReviewComments(ctx, owner, repo, prNumber, oldReview.ID)
|
||||
if err != nil {
|
||||
slog.Warn("could not list old review comments for resolution", "review_id", oldReview.ID, "error", err)
|
||||
continue
|
||||
}
|
||||
for _, c := range oldComments {
|
||||
if c.ID == 0 {
|
||||
continue
|
||||
}
|
||||
_ = underlying.ResolveComment(ctx, owner, repo, c.ID)
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// buildSupersededBody creates the body for a superseded review: struck-through banner
|
||||
// with collapsed original content and the commit it was evaluated against.
|
||||
func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string) string {
|
||||
shortSHA := commitSHA
|
||||
if len(shortSHA) > 8 {
|
||||
shortSHA = shortSHA[:8]
|
||||
}
|
||||
var sb strings.Builder
|
||||
sb.WriteString("~~Original review~~\n\n")
|
||||
sb.WriteString("**Superseded** \u2014 [see current review](")
|
||||
sb.WriteString(newReviewURL)
|
||||
sb.WriteString(") for up-to-date findings.\n\n")
|
||||
if shortSHA != "" {
|
||||
sb.WriteString("<details><summary>Previous findings (commit ")
|
||||
sb.WriteString(shortSHA)
|
||||
sb.WriteString(")</summary>\n\n")
|
||||
} else {
|
||||
sb.WriteString("<details><summary>Previous findings</summary>\n\n")
|
||||
}
|
||||
sb.WriteString(originalBody)
|
||||
sb.WriteString("\n\n</details>\n\n")
|
||||
sb.WriteString(sentinel)
|
||||
return sb.String()
|
||||
}
|
||||
|
||||
@@ -0,0 +1,54 @@
|
||||
package gitea
|
||||
|
||||
import (
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestBuildSupersededBody(t *testing.T) {
|
||||
original := "# Review\n\nLooks good.\n\n<!-- review-bot:sonnet -->"
|
||||
sentinel := "<!-- review-bot:sonnet -->"
|
||||
newURL := "https://gitea.example.com/owner/repo/pulls/1#pullrequestreview-99"
|
||||
|
||||
result := buildSupersededBody(original, "abcdef1234567890", newURL, sentinel)
|
||||
|
||||
// Should contain the struck-through banner
|
||||
if !strings.Contains(result, "~~Original review~~") {
|
||||
t.Error("missing struck-through banner")
|
||||
}
|
||||
// Should contain superseded notice with link
|
||||
if !strings.Contains(result, "**Superseded**") {
|
||||
t.Error("missing superseded notice")
|
||||
}
|
||||
if !strings.Contains(result, "[see current review]("+newURL+")") {
|
||||
t.Error("missing link to new review")
|
||||
}
|
||||
// Should contain collapsed original
|
||||
if !strings.Contains(result, "<details>") {
|
||||
t.Error("missing details/collapse")
|
||||
}
|
||||
// Should contain short commit SHA
|
||||
if !strings.Contains(result, "abcdef12") {
|
||||
t.Error("missing short SHA")
|
||||
}
|
||||
// Should NOT contain full SHA in summary (it's truncated to 8)
|
||||
if strings.Contains(result, "abcdef1234567890") {
|
||||
t.Error("should truncate SHA to 8 chars")
|
||||
}
|
||||
// Should contain the original body inside details
|
||||
if !strings.Contains(result, original) {
|
||||
t.Error("original body not preserved in collapsed section")
|
||||
}
|
||||
// Should end with sentinel
|
||||
if !strings.Contains(result, sentinel) {
|
||||
t.Error("missing sentinel")
|
||||
}
|
||||
}
|
||||
|
||||
func TestBuildSupersededBodyShortSHA(t *testing.T) {
|
||||
// Short SHA should pass through without panic
|
||||
result := buildSupersededBody("body", "abc", "https://example.com/review", "<!-- review-bot:x -->")
|
||||
if !strings.Contains(result, "abc") {
|
||||
t.Error("short SHA not preserved")
|
||||
}
|
||||
}
|
||||
@@ -9,3 +9,6 @@ import (
|
||||
// This verifies github.Client satisfies the full vcs.Client interface
|
||||
// (PRReader, FileReader, Reviewer, Identity).
|
||||
var _ vcs.Client = (*github.Client)(nil)
|
||||
|
||||
// Verify github.Client implements ReviewSuperseder.
|
||||
var _ vcs.ReviewSuperseder = (*github.Client)(nil)
|
||||
|
||||
@@ -210,3 +210,16 @@ func (c *Client) DismissReview(ctx context.Context, owner, repo string, number i
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// SupersedeReviews marks prior reviews as superseded by dismissing them.
|
||||
// This implements vcs.ReviewSuperseder for the GitHub adapter.
|
||||
// The baseURL and sentinel parameters are unused for GitHub (dismissal is the mechanism).
|
||||
func (c *Client) SupersedeReviews(ctx context.Context, owner, repo string, prNumber int, oldReviews []vcs.Review, newReviewID int64, _, _ string) error {
|
||||
var errs []error
|
||||
for _, old := range oldReviews {
|
||||
if err := c.DismissReview(ctx, owner, repo, prNumber, old.ID, "Superseded by new review"); err != nil {
|
||||
errs = append(errs, fmt.Errorf("dismiss review %d: %w", old.ID, err))
|
||||
}
|
||||
}
|
||||
return errors.Join(errs...)
|
||||
}
|
||||
|
||||
@@ -49,3 +49,12 @@ type Client interface {
|
||||
type ReviewerSelfRequester interface {
|
||||
RequestReviewerSelf(ctx context.Context, owner, repo string, number int, user string) error
|
||||
}
|
||||
|
||||
// ReviewSuperseder is an optional interface implemented by adapters that support
|
||||
// marking old reviews as superseded. For Gitea this means editing the review body
|
||||
// with a link to the new review and resolving inline comments. For GitHub this
|
||||
// means dismissing old reviews.
|
||||
// Consumers should use interface assertion: if rs, ok := client.(ReviewSuperseder); ok { ... }
|
||||
type ReviewSuperseder interface {
|
||||
SupersedeReviews(ctx context.Context, owner, repo string, prNumber int, oldReviews []Review, newReviewID int64, baseURL, sentinel string) error
|
||||
}
|
||||
|
||||
@@ -0,0 +1,26 @@
|
||||
package vcs
|
||||
|
||||
// VCSProvider identifies a VCS platform. Using a typed string instead of bare
|
||||
// strings makes provider values compiler-checkable and prevents typos from
|
||||
// silently passing validation.
|
||||
type VCSProvider string
|
||||
|
||||
const (
|
||||
ProviderGitea VCSProvider = "gitea"
|
||||
ProviderGithub VCSProvider = "github"
|
||||
|
sonnet-review-bot
commented
[NIT] **[NIT]** `ProviderGithub VCSProvider = "github"` — the constant name uses `Github` (lowercase 'h') while the package import path and the flag help text use `github`. Per the Go acronym convention documented in `patterns/style.md` (Acronyms Are All-Caps), this should be `ProviderGitHub`. This is a new exported symbol so fixing it now is low-cost; changing it later would break callers.
|
||||
)
|
||||
|
||||
// Valid reports whether p is a known VCS provider.
|
||||
func (p VCSProvider) Valid() bool {
|
||||
switch p {
|
||||
case ProviderGitea, ProviderGithub:
|
||||
return true
|
||||
default:
|
||||
return false
|
||||
}
|
||||
}
|
||||
|
||||
// String returns the string representation of the provider.
|
||||
func (p VCSProvider) String() string {
|
||||
return string(p)
|
||||
}
|
||||
[MINOR] The --gitea-url alias registration uses
flag.StringVar(vcsURL, "gitea-url", *vcsURL, ...)after vcsURL has already been parsed from env vars. This means the default for the alias is whatever vcsURL resolved to at registration time, not a live binding. In practice this works fine because flag.Parse() hasn't run yet, but it's subtly fragile: if the env-var chain for vcsURL ever produces an empty string, the alias and the primary flag start with different defaults. A comment explaining this would help future maintainers.[NIT] The comment says the --gitea-url flag is a hidden alias, but it is registered normally and will appear in usage. Either adjust wording to 'deprecated alias' or consider not exposing it in help if truly intended to be hidden.
[MINOR] The
--gitea-urlbackward-compatible alias is registered withflag.StringVar(vcsURL, "gitea-url", *vcsURL, ...). This dereferences*vcsURLat flag registration time to capture the current default, which is correct. However, the comment says 'whichever flag is set last by flag.Parse wins' — this is accurate but the behavior may surprise users if both--vcs-urland--gitea-urlare passed simultaneously (last one wins rather than an error). This is a known limitation of theflagpackage alias approach and should ideally be documented in the flag usage or a warning emitted if both are set.Added a comment explaining the shared
*stringpointer mechanism.flag.StringVar(vcsURL, ...)binds--gitea-urlto the same underlying*stringas--vcs-url, so there's no divergence risk — both flags always resolve to the same value afterflag.Parse(). Comment added in7d6fe27.Updated the inline comment from "hidden alias" to "backward-compatible alias" in
7d6fe27. The flag does appear in--helpoutput (Go'sflagpackage doesn't support hiding flags), so "hidden" was inaccurate. "Backward-compatible" better describes the intent.[NIT] The comment
// Backward-compatible alias: --gitea-url shares vcsURL's pointer (last flag wins).is accurate but slightly misleading: withflag.StringVar(vcsURL, "gitea-url", *vcsURL, ...)the last parsed flag wins at parse time. If both--vcs-urland--gitea-urlare supplied, the rightmost one on the command line takes effect. This is the correct and intended behaviour, but a small clarification in the comment ("last supplied on the command line wins") would help future readers.