From ac6d34f5bd09cb340db87acee6f34c1c2c504f53 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 09:03:42 -0700 Subject: [PATCH] fix: address review feedback - eliminate type assertion via ReviewSuperseder interface - Introduce vcs.VCSProvider typed constant (replaces plain string provider) - Introduce vcs.ReviewSuperseder optional interface for supersede logic - Implement SupersedeReviews on gitea.Adapter (edit + resolve) and github.Client (dismiss) - Remove concrete type assertion client.(*gitea.Adapter) from main - Remove redundant baseURL fallback for github (NewClient defaults it) - Condense --gitea-url alias comment block - Fix fetchPatterns comment (empty paths are skipped, not fetched) - Add default panic to VCS client init switch Addresses: #19607, #19608, #19609, #19610, #19621, #19622, #19623 --- cmd/review-bot/main.go | 156 +++++------------------------------- cmd/review-bot/main_test.go | 48 ----------- gitea/adapter.go | 75 +++++++++++++++++ gitea/supersede_test.go | 54 +++++++++++++ github/conformance_test.go | 3 + github/review.go | 13 +++ vcs/interfaces.go | 9 +++ vcs/provider.go | 26 ++++++ 8 files changed, 201 insertions(+), 183 deletions(-) create mode 100644 gitea/supersede_test.go create mode 100644 vcs/provider.go diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 788408e..f08deb8 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -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)") - // 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). + // Must stay after vcsURL declaration and before flag.Parse(). 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) // 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: + panic("unreachable: provider validation should have caught " + vcsProvider.String()) } - slog.Info("VCS client initialized", "provider", *provider) + slog.Info("VCS client initialized", "provider", vcsProvider) // Initialize LLM client llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel) @@ -506,11 +493,15 @@ func main() { } slog.Info("review posted", "review_id", posted.ID, "user", posted.User.Login, "pr", prNumber) - // 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 { + slog.Error("failed to supersede old reviews", "error", err) + os.Exit(1) + } + } else { + slog.Warn("provider does not support review superseding", "provider", vcsProvider) } } } @@ -527,88 +518,6 @@ func verdictToEvent(verdict string) vcs.ReviewEvent { } } -// 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). 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 } -// 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("
Previous findings (commit ") - sb.WriteString(shortSHA) - sb.WriteString(")\n\n") - } else { - sb.WriteString("
Previous findings\n\n") - } - sb.WriteString(originalBody) - sb.WriteString("\n\n
\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 diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index 3f88d8f..49050f0 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -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" - sentinel := "" - 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, "
") { - 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", "") - if !strings.Contains(result, "abc") { - t.Error("short SHA not preserved") - } -} - func TestHasSharedToken(t *testing.T) { tests := []struct { name string diff --git a/gitea/adapter.go b/gitea/adapter.go index 905c36c..9c4f753 100644 --- a/gitea/adapter.go +++ b/gitea/adapter.go @@ -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) + } + + 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("
Previous findings (commit ") + sb.WriteString(shortSHA) + sb.WriteString(")\n\n") + } else { + sb.WriteString("
Previous findings\n\n") + } + sb.WriteString(originalBody) + sb.WriteString("\n\n
\n\n") + sb.WriteString(sentinel) + return sb.String() +} diff --git a/gitea/supersede_test.go b/gitea/supersede_test.go new file mode 100644 index 0000000..8116b55 --- /dev/null +++ b/gitea/supersede_test.go @@ -0,0 +1,54 @@ +package gitea + +import ( + "strings" + "testing" +) + +func TestBuildSupersededBody(t *testing.T) { + original := "# Review\n\nLooks good.\n\n" + sentinel := "" + 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, "
") { + 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", "") + if !strings.Contains(result, "abc") { + t.Error("short SHA not preserved") + } +} diff --git a/github/conformance_test.go b/github/conformance_test.go index 3e06e2d..a0fc252 100644 --- a/github/conformance_test.go +++ b/github/conformance_test.go @@ -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) diff --git a/github/review.go b/github/review.go index 1a54fcf..467f890 100644 --- a/github/review.go +++ b/github/review.go @@ -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...) +} diff --git a/vcs/interfaces.go b/vcs/interfaces.go index 61f3483..4b1adff 100644 --- a/vcs/interfaces.go +++ b/vcs/interfaces.go @@ -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 +} diff --git a/vcs/provider.go b/vcs/provider.go new file mode 100644 index 0000000..2d19409 --- /dev/null +++ b/vcs/provider.go @@ -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" +) + +// 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) +}