From 24e3ab105a7b49eba332856329fa0c16bab7f7ff Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 01:51:21 -0700 Subject: [PATCH] fix: address review feedback on PR #106 - Replace interface{} with any in github/reviews.go (Go 1.18+ idiom) - Add default panic case to VCS client init switch - Refactor supersedeOldReviews to return error instead of os.Exit(1) - Remove spurious blank lines in formatter.go and formatter_test.go - Add doc comment to DeleteReview explaining when to use vs DismissReview - Sanitize extractSentinelName output to prevent log injection --- cmd/review-bot/main.go | 31 +++++++++++++++++++++++++------ github/reviews.go | 6 ++++-- review/formatter.go | 1 - review/formatter_test.go | 1 - 4 files changed, 29 insertions(+), 10 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 4183c22..28d2d42 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -169,6 +169,8 @@ func main() { ghBaseURL = "https://api.github.com" } client = github.NewClient(*reviewerToken, ghBaseURL) + default: + panic("unreachable: unhandled provider " + *provider) } slog.Info("VCS client initialized", "provider", *provider) @@ -498,7 +500,10 @@ func main() { // Supersede all old reviews if len(oldReviews) > 0 { - 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) + } } } @@ -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 } // Gitea: existing EditComment + ResolveComment flow giteaAdapter, ok := client.(*gitea.Adapter) if !ok { - 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. @@ -846,7 +851,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. diff --git a/github/reviews.go b/github/reviews.go index c677679..c6ed536 100644 --- a/github/reviews.go +++ b/github/reviews.go @@ -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", 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) diff --git a/review/formatter.go b/review/formatter.go index 0b2271f..de9b558 100644 --- a/review/formatter.go +++ b/review/formatter.go @@ -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). diff --git a/review/formatter_test.go b/review/formatter_test.go index be3bdee..22da660 100644 --- a/review/formatter_test.go +++ b/review/formatter_test.go @@ -98,7 +98,6 @@ func TestFormatMarkdown_SpecialChars(t *testing.T) { } } - func TestFormatMarkdown_Sentinel(t *testing.T) { result := &ReviewResult{ Verdict: "APPROVE",