From 4881a21ecbef182f34604dff2277f52c42bcbb4f Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 01:21:32 -0700 Subject: [PATCH 01/17] feat(cmd): wire --provider and --base-url flags into CLI - Add --provider flag (gitea|github) for VCS backend selection - Add --base-url flag for GitHub API endpoint configuration - Rename --gitea-url to --vcs-url with backward-compatible alias - Replace direct gitea.Client usage with vcs.Client interface - Create vcs.Client via factory switch based on --provider value - Implement Reviewer + Identity interfaces on github.Client - Add verdictToEvent() using canonical vcs.ReviewEvent types - Remove review.GiteaEvent() (replaced by verdictToEvent) - GitHub supersede uses DismissReview; Gitea keeps EditComment flow - Add VCS_PROVIDER, VCS_BASE_URL, VCS_URL env var support Closes #82 --- cmd/review-bot/main.go | 322 +++++++++++++++++++++--------------- cmd/review-bot/main_test.go | 140 ++++++++++------ github/reviews.go | 236 ++++++++++++++++++++++++++ review/formatter.go | 11 -- review/formatter_test.go | 18 -- 5 files changed, 515 insertions(+), 212 deletions(-) create mode 100644 github/reviews.go diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 40efc46..125d4dc 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -13,6 +13,7 @@ import ( "gitea.weiker.me/rodin/review-bot/budget" "gitea.weiker.me/rodin/review-bot/gitea" + "gitea.weiker.me/rodin/review-bot/github" "gitea.weiker.me/rodin/review-bot/llm" "gitea.weiker.me/rodin/review-bot/review" "gitea.weiker.me/rodin/review-bot/vcs" @@ -54,12 +55,15 @@ func main() { // Logging flags logFormat := flag.String("log-format", envOrDefault("LOG_FORMAT", "text"), "Log output format: text or json") verbosity := flag.String("verbosity", envOrDefault("LOG_VERBOSITY", "info"), "Log verbosity: debug, info, warn, error") - // CLI flags - giteaURL := flag.String("gitea-url", envOrDefault("GITEA_URL", envOrDefault("GITHUB_SERVER_URL", "")), "Gitea instance URL") - repo := flag.String("repo", envOrDefault("GITEA_REPO", envOrDefault("GITHUB_REPOSITORY", "")), "Repository (owner/name)") + // VCS flags + provider := flag.String("provider", envOrDefault("VCS_PROVIDER", "gitea"), "VCS provider: gitea or github") + baseURL := flag.String("base-url", envOrDefault("VCS_BASE_URL", ""), "VCS API base URL (for github provider; defaults to https://api.github.com)") + vcsURL := flag.String("vcs-url", envOrDefault("VCS_URL", envOrDefault("GITEA_URL", envOrDefault("GITHUB_SERVER_URL", ""))), "VCS instance URL (Gitea) [deprecated alias: --gitea-url]") + // Keep --gitea-url as hidden alias (flag package doesn't support aliases natively, handle below) + repo := flag.String("repo", envOrDefault("VCS_REPO", envOrDefault("GITEA_REPO", envOrDefault("GITHUB_REPOSITORY", ""))), "Repository (owner/name)") prNum := flag.String("pr", envOrDefault("PR_NUMBER", ""), "Pull request number") reviewerName := flag.String("reviewer-name", envOrDefault("REVIEWER_NAME", ""), "Reviewer display name") - reviewerToken := flag.String("reviewer-token", envOrDefault("REVIEWER_TOKEN", ""), "Gitea token for posting review") + reviewerToken := flag.String("reviewer-token", envOrDefault("REVIEWER_TOKEN", ""), "VCS token for posting review") llmBaseURL := flag.String("llm-base-url", envOrDefault("LLM_BASE_URL", ""), "LLM API base URL") llmAPIKey := flag.String("llm-api-key", envOrDefault("LLM_API_KEY", ""), "LLM API key") llmModel := flag.String("llm-model", envOrDefault("LLM_MODEL", ""), "LLM model name") @@ -80,6 +84,9 @@ 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 deprecated alias for --vcs-url + flag.StringVar(vcsURL, "gitea-url", *vcsURL, "Deprecated: use --vcs-url instead") + flag.Parse() if *versionFlag { @@ -92,12 +99,25 @@ func main() { slog.Info("review-bot starting", "version", version) + // Validate VCS provider + switch *provider { + case "gitea", "github": + // valid + default: + fmt.Fprintf(os.Stderr, "Error: invalid --provider %q (valid: gitea, github)\n", *provider) + os.Exit(1) + } + // Validate required fields - // For aicore provider, llm-base-url and llm-api-key are not required isAICore := llm.Provider(*llmProvider) == llm.ProviderAICore - if *giteaURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" || *llmModel == "" { + if *repo == "" || *prNum == "" || *reviewerToken == "" || *llmModel == "" { fmt.Fprintf(os.Stderr, "Error: missing required flags or environment variables\n\n") - fmt.Fprintf(os.Stderr, "Required: --gitea-url, --repo, --pr, --reviewer-token, --llm-model\n") + fmt.Fprintf(os.Stderr, "Required: --repo, --pr, --reviewer-token, --llm-model\n") + os.Exit(1) + } + // --vcs-url is required only for gitea provider + if *provider == "gitea" && *vcsURL == "" { + fmt.Fprintf(os.Stderr, "Error: --vcs-url (or --gitea-url) is required for provider=gitea\n") os.Exit(1) } if !isAICore && (*llmBaseURL == "" || *llmAPIKey == "") { @@ -116,8 +136,6 @@ func main() { os.Exit(1) } - // NOTE: Persona loading deferred until after Gitea client init to support repo personas - // Validate reviewer-name: only safe characters allowed in sentinel if err := validateReviewerName(*reviewerName); err != nil { slog.Error("invalid reviewer name", "error", err) @@ -139,8 +157,22 @@ func main() { os.Exit(1) } - // Initialize clients - giteaClient := gitea.NewClient(*giteaURL, *reviewerToken) + // Initialize VCS client + var client vcs.Client + switch *provider { + case "gitea": + 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) + } + slog.Info("VCS client initialized", "provider", *provider) + + // Initialize LLM client llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel) if *llmTemp < 0 || *llmTemp > 2 { slog.Error("invalid LLM temperature", "temperature", *llmTemp, "range", "0-2") @@ -174,16 +206,13 @@ func main() { ctx, cancel := context.WithTimeout(context.Background(), overallTimeout) defer cancel() - // Load persona if specified (after Gitea client init to support repo personas) + // Load persona if specified var persona *review.Persona if *personaName != "" { // Try loading from repo first, then fall back to built-in - repoPersonas, err := review.LoadRepoPersonas(ctx, newGiteaClientAdapter(giteaClient), owner, repoName) + repoPersonas, err := review.LoadRepoPersonas(ctx, client, owner, repoName) if err != nil { slog.Warn("could not load repo personas", "repo", owner+"/"+repoName, "error", err) - // Continue with built-in personas only. - // NOTE: repoPersonas is nil here, but map indexing on a nil map is safe in Go - // (returns the zero value), so the fallback to built-in below works correctly. } if p, ok := repoPersonas[*personaName]; ok { persona = p @@ -214,7 +243,7 @@ func main() { slog.Info("reviewing pull request", "pr", prNumber, "repo", fmt.Sprintf("%s/%s", owner, repoName)) // Step 1: Fetch PR metadata - pr, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber) + pr, err := client.GetPullRequest(ctx, owner, repoName, prNumber) if err != nil { slog.Error("failed to fetch PR", "pr", prNumber, "error", err) os.Exit(1) @@ -222,7 +251,7 @@ func main() { slog.Info("fetched PR metadata", "pr", prNumber, "title", pr.Title) // Step 2: Fetch diff - diff, err := giteaClient.GetPullRequestDiff(ctx, owner, repoName, prNumber) + diff, err := client.GetPullRequestDiff(ctx, owner, repoName, prNumber) if err != nil { slog.Error("failed to fetch diff", "pr", prNumber, "error", err) os.Exit(1) @@ -231,21 +260,21 @@ func main() { // Step 3: Fetch full file content for modified files fileContext := "" - files, err := giteaClient.GetPullRequestFiles(ctx, owner, repoName, prNumber) + files, err := client.GetPullRequestFiles(ctx, owner, repoName, prNumber) if err != nil { slog.Warn("could not fetch PR files list", "pr", prNumber, "error", err) } else { - fileContext = fetchFileContext(ctx, giteaClient, owner, repoName, pr.Head.Ref, files) + fileContext = fetchFileContext(ctx, client, owner, repoName, pr.Head.Ref, files) slog.Debug("fetched file context", "files", len(files)) } // Step 4: Check CI status ciPassed := true ciDetails := "" - if pr.Head.Sha != "" { - statuses, err := giteaClient.GetCommitStatuses(ctx, owner, repoName, pr.Head.Sha) + if pr.Head.SHA != "" { + statuses, err := client.GetCommitStatuses(ctx, owner, repoName, pr.Head.SHA) if err != nil { - slog.Warn("could not fetch CI status", "sha", pr.Head.Sha, "error", err) + slog.Warn("could not fetch CI status", "sha", pr.Head.SHA, "error", err) } else { ciPassed, ciDetails = evaluateCIStatus(statuses) slog.Info("CI status checked", "passed", ciPassed) @@ -255,7 +284,7 @@ func main() { // Step 5: Load conventions file if specified conventions := "" if *conventionsFile != "" { - content, err := giteaClient.GetFileContent(ctx, owner, repoName, *conventionsFile) + content, err := client.GetFileContent(ctx, owner, repoName, *conventionsFile, "") if err != nil { slog.Warn("could not load conventions file", "file", *conventionsFile, "error", err) } else { @@ -267,7 +296,7 @@ func main() { // Step 6: Load patterns from external repo if specified patterns := "" if *patternsRepo != "" { - patterns = fetchPatterns(ctx, giteaClient, *patternsRepo, *patternsFiles) + patterns = fetchPatterns(ctx, client, *patternsRepo, *patternsFiles) slog.Debug("loaded patterns", "repo", *patternsRepo, "bytes", len(patterns)) } @@ -360,15 +389,16 @@ func main() { } // Add commit footer so readers know which commit was evaluated - if pr.Head.Sha != "" { - shortSHA := pr.Head.Sha + if pr.Head.SHA != "" { + shortSHA := pr.Head.SHA if len(shortSHA) > 8 { shortSHA = shortSHA[:8] } reviewBody += fmt.Sprintf("\n\n---\n*Evaluated against %s*", shortSHA) } - event := review.GiteaEvent(result.Verdict) + // Map verdict to canonical review event + event := verdictToEvent(result.Verdict) if *dryRun { fmt.Println("--- DRY RUN ---") @@ -380,14 +410,13 @@ func main() { sentinel := fmt.Sprintf("", *reviewerName) // Stale check: verify HEAD hasn't moved since we started - evaluatedSHA := pr.Head.Sha + evaluatedSHA := pr.Head.SHA var currentSHA string - currentPR, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber) + currentPR, err := client.GetPullRequest(ctx, owner, repoName, prNumber) if err != nil { slog.Warn("could not re-fetch PR for stale check", "pr", prNumber, "error", err) - // currentSHA stays empty — shouldSkipStaleReview will return false } else { - currentSHA = currentPR.Head.Sha + currentSHA = currentPR.Head.SHA } if shouldSkipStaleReview(evaluatedSHA, currentSHA) { slog.Warn("HEAD moved during review — skipping stale review", @@ -397,17 +426,24 @@ func main() { return } - // Map findings to inline comments for lines present in the diff - diffRanges := gitea.ParseDiffNewLines(diff) - var inlineComments []gitea.ReviewComment + // Build line→position map for inline comments + lineToPosition := vcs.BuildLineToPositionMap(diff) + var inlineComments []vcs.ReviewComment for _, f := range result.Findings { - if f.File != "" && f.Line > 0 && diffRanges.Contains(f.File, f.Line) { - inlineComments = append(inlineComments, gitea.ReviewComment{ - Path: f.File, - NewPosition: int64(f.Line), - Body: fmt.Sprintf("**[%s]** %s", f.Severity, f.Finding), - }) + if f.File == "" || f.Line <= 0 { + continue } + pos, ok := lineToPosition[f.File][f.Line] + if !ok { + slog.Warn("line not in diff, skipping comment", "file", f.File, "line", f.Line) + continue + } + inlineComments = append(inlineComments, vcs.ReviewComment{ + Path: f.File, + Position: pos, + CommitID: pr.Head.SHA, + Body: fmt.Sprintf("**[%s]** %s", f.Severity, f.Finding), + }) } if len(inlineComments) > 0 { slog.Debug("attaching inline comments", "count", len(inlineComments)) @@ -416,10 +452,9 @@ func main() { // --- Review update strategy --- // 1. POST new review first (gets non-stale approval badge on HEAD) // 2. Then supersede old review with link to the new one - // Order matters: post first so we have the new review's URL for the supersede message. - var oldReviews []gitea.Review + var oldReviews []vcs.Review if *reviewerName != "" { - existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) + existingReviews, err := client.ListReviews(ctx, owner, repoName, prNumber) if err != nil { slog.Warn("could not list existing reviews", "pr", prNumber, "error", err) } else { @@ -431,74 +466,120 @@ func main() { } } - // Self-request as reviewer (ensures we appear in required-reviewer checks) - authUser, err := giteaClient.GetAuthenticatedUser(ctx) - if err != nil { - slog.Warn("could not determine authenticated user for reviewer self-request", "error", err) - } else if authUser != "" { - if err := giteaClient.RequestReviewer(ctx, owner, repoName, prNumber, authUser); err != nil { - slog.Warn("could not self-request as reviewer", "user", authUser, "error", err) - } else { - slog.Debug("self-requested as reviewer", "user", authUser, "pr", prNumber) + // Self-request as reviewer (Gitea-specific; ensures we appear in required-reviewer checks) + if giteaAdapter, ok := client.(*gitea.Adapter); ok { + authUser, err := client.GetAuthenticatedUser(ctx) + if err != nil { + slog.Warn("could not determine authenticated user for reviewer self-request", "error", err) + } else if authUser != "" { + if err := giteaAdapter.Underlying().RequestReviewer(ctx, owner, repoName, prNumber, authUser); err != nil { + slog.Warn("could not self-request as reviewer", "user", authUser, "error", err) + } else { + slog.Debug("self-requested as reviewer", "user", authUser, "pr", prNumber) + } } + } else { + slog.Debug("RequestReviewer not supported for provider, skipping") } // POST new review slog.Info("posting review", "event", event, "pr", prNumber) - posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments) + reviewReq := vcs.ReviewRequest{ + Body: reviewBody, + Event: event, + Comments: inlineComments, + } + posted, err := client.PostReview(ctx, owner, repoName, prNumber, reviewReq) if err != nil { slog.Error("failed to post review", "pr", prNumber, "event", event, "error", err) os.Exit(1) } slog.Info("review posted", "review_id", posted.ID, "user", posted.User.Login, "pr", prNumber) - // Supersede all old reviews with link to the new one + // Supersede all old reviews if len(oldReviews) > 0 { - newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*giteaURL, "/"), owner, repoName, prNumber, posted.ID) - for _, oldReview := range oldReviews { - cid, err := giteaClient.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 := giteaClient.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", posted.ID, "pr", prNumber) + supersedeOldReviews(ctx, client, *provider, *vcsURL, owner, repoName, prNumber, oldReviews, posted.ID, sentinel) + } +} - // Resolve old review's inline comments - oldComments, err := giteaClient.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 := giteaClient.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) +// verdictToEvent maps a verdict string from the LLM response to a canonical vcs.ReviewEvent. +func verdictToEvent(verdict string) vcs.ReviewEvent { + switch verdict { + case "APPROVE": + return vcs.ReviewEventApprove + case "REQUEST_CHANGES": + return vcs.ReviewEventRequestChanges + default: + return vcs.ReviewEventComment + } +} + +// 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) { + if provider == "github" { + 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) + } else { + slog.Info("dismissed old review", "review_id", old.ID, "new_review_id", newReviewID, "pr", prNumber) } } + return } + // Gitea: existing EditComment + ResolveComment flow + giteaAdapter, ok := client.(*gitea.Adapter) + if !ok { + slog.Error("expected gitea.Adapter for gitea provider") + os.Exit(1) + } + underlying := giteaAdapter.Underlying() + + 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) + } + } } // fetchFileContext fetches the full content of modified files from the PR branch. -func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, ref string, files []gitea.ChangedFile) string { +func fetchFileContext(ctx context.Context, client vcs.PRReader, owner, repo, ref string, files []vcs.ChangedFile) string { var sb strings.Builder for _, f := range files { if ctx.Err() != nil { @@ -507,7 +588,7 @@ func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, re if f.Status == "removed" { continue // Skip deleted files } - content, err := client.GetFileContentRef(ctx, owner, repo, f.Filename, ref) + content, err := client.GetFileContentAtRef(ctx, owner, repo, f.Filename, ref) if err != nil { slog.Warn("could not fetch file content", "file", f.Filename, "error", err) continue @@ -524,7 +605,8 @@ func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, re // 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. -func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patternsFiles string) string { +// If patternsFiles is empty, all files from the repo root are fetched. +func fetchPatterns(ctx context.Context, client vcs.FileReader, patternsRepo, patternsFiles string) string { var sb strings.Builder repos := strings.Split(patternsRepo, ",") @@ -554,7 +636,7 @@ func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patt continue } - files, err := client.GetAllFilesInPath(ctx, owner, repo, path) + files, err := vcs.GetAllFilesInPath(ctx, client, owner, repo, path) if err != nil { slog.Warn("could not fetch patterns", "path", path, "repo", repoRef, "error", err) continue @@ -593,7 +675,7 @@ func isPatternFile(path string) bool { } // evaluateCIStatus checks if all CI statuses indicate success. -func evaluateCIStatus(statuses []gitea.CommitStatus) (passed bool, details string) { +func evaluateCIStatus(statuses []vcs.CommitStatus) (passed bool, details string) { if len(statuses) == 0 { return true, "no CI statuses found" } @@ -728,10 +810,10 @@ func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string) } // hasSharedToken detects if another review-bot role posted under the same -// Gitea user. This indicates misconfiguration where two roles share a token -// instead of having separate Gitea accounts. Returns true if shared token +// VCS user. This indicates misconfiguration where two roles share a token +// instead of having separate accounts. Returns true if shared token // detected (caller should skip update-in-place logic to avoid clobbering). -func hasSharedToken(reviews []gitea.Review, ownSentinel string) bool { +func hasSharedToken(reviews []vcs.Review, ownSentinel string) bool { ownLogin := "" for _, r := range reviews { if strings.Contains(r.Body, ownSentinel) { @@ -744,7 +826,7 @@ func hasSharedToken(reviews []gitea.Review, ownSentinel string) bool { } for _, r := range reviews { if r.User.Login == ownLogin && strings.Contains(r.Body, ""), }, sentinel: "", @@ -237,7 +234,7 @@ func TestFindOwnReview(t *testing.T) { }, { name: "wrong sentinel", - reviews: []gitea.Review{ + reviews: []vcs.Review{ makeReview(42, "bot", "APPROVED", false, "body\n"), }, sentinel: "", @@ -245,7 +242,7 @@ func TestFindOwnReview(t *testing.T) { }, { name: "multiple reviews, returns first match", - reviews: []gitea.Review{ + reviews: []vcs.Review{ makeReview(10, "bot", "APPROVED", false, "old\n"), makeReview(20, "bot", "APPROVED", false, "new\n"), }, @@ -254,7 +251,7 @@ func TestFindOwnReview(t *testing.T) { }, { name: "skips superseded review", - reviews: []gitea.Review{ + reviews: []vcs.Review{ makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n**Superseded**\n"), makeReview(20, "bot", "APPROVED", false, "fresh review\n"), }, @@ -263,7 +260,7 @@ func TestFindOwnReview(t *testing.T) { }, { name: "only superseded reviews exist", - reviews: []gitea.Review{ + reviews: []vcs.Review{ makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n"), }, sentinel: "", @@ -271,7 +268,7 @@ func TestFindOwnReview(t *testing.T) { }, { name: "picks highest ID among matches", - reviews: []gitea.Review{ + reviews: []vcs.Review{ makeReview(50, "bot", "APPROVED", false, "v1\n"), makeReview(30, "bot", "APPROVED", false, "v0\n"), }, @@ -302,7 +299,7 @@ func TestFindOwnReview(t *testing.T) { func TestHasSharedToken(t *testing.T) { tests := []struct { name string - reviews []gitea.Review + reviews []vcs.Review sentinel string want bool }{ @@ -314,36 +311,36 @@ func TestHasSharedToken(t *testing.T) { }, { name: "no own review yet - cannot detect", - reviews: []gitea.Review{ - {ID: 1, User: struct{ Login string `json:"login"` }{Login: "other"}, Body: " body"}, + reviews: []vcs.Review{ + makeReview(1, "other", "APPROVED", false, " body"), }, sentinel: "", want: false, }, { name: "separate users - no shared token", - reviews: []gitea.Review{ - {ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: " body"}, - {ID: 2, User: struct{ Login string `json:"login"` }{Login: "security-review-bot"}, Body: " body"}, + reviews: []vcs.Review{ + makeReview(1, "sonnet-review-bot", "APPROVED", false, " body"), + makeReview(2, "security-review-bot", "APPROVED", false, " body"), }, sentinel: "", want: false, }, { name: "shared token detected - same user different sentinels", - reviews: []gitea.Review{ - {ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: " body"}, - {ID: 2, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: " body"}, + reviews: []vcs.Review{ + makeReview(1, "sonnet-review-bot", "APPROVED", false, " body"), + makeReview(2, "sonnet-review-bot", "APPROVED", false, " body"), }, sentinel: "", want: true, }, { name: "three roles same user", - reviews: []gitea.Review{ - {ID: 1, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: " body"}, - {ID: 2, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: " body"}, - {ID: 3, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: " body"}, + reviews: []vcs.Review{ + makeReview(1, "bot", "APPROVED", false, " body"), + makeReview(2, "bot", "APPROVED", false, " body"), + makeReview(3, "bot", "APPROVED", false, " body"), }, sentinel: "", want: true, @@ -507,7 +504,7 @@ func TestIsPatternFile(t *testing.T) { func TestEvaluateCIStatus(t *testing.T) { tests := []struct { name string - statuses []gitea.CommitStatus + statuses []vcs.CommitStatus wantPassed bool wantSubstr string }{ @@ -519,7 +516,7 @@ func TestEvaluateCIStatus(t *testing.T) { }, { name: "all success", - statuses: []gitea.CommitStatus{ + statuses: []vcs.CommitStatus{ {Status: "success", Context: "ci/build", Description: "Build passed"}, {Status: "success", Context: "ci/test", Description: "Tests passed"}, }, @@ -528,7 +525,7 @@ func TestEvaluateCIStatus(t *testing.T) { }, { name: "one failure", - statuses: []gitea.CommitStatus{ + statuses: []vcs.CommitStatus{ {Status: "success", Context: "ci/build", Description: "Build passed"}, {Status: "failure", Context: "ci/test", Description: "Tests failed"}, }, @@ -537,7 +534,7 @@ func TestEvaluateCIStatus(t *testing.T) { }, { name: "error status", - statuses: []gitea.CommitStatus{ + statuses: []vcs.CommitStatus{ {Status: "error", Context: "ci/lint", Description: "Lint error"}, }, wantPassed: false, @@ -545,7 +542,7 @@ func TestEvaluateCIStatus(t *testing.T) { }, { name: "pending treated as not-failed", - statuses: []gitea.CommitStatus{ + statuses: []vcs.CommitStatus{ {Status: "pending", Context: "ci/build", Description: "In progress"}, {Status: "success", Context: "ci/test", Description: "Tests passed"}, }, @@ -554,7 +551,7 @@ func TestEvaluateCIStatus(t *testing.T) { }, { name: "multiple failures", - statuses: []gitea.CommitStatus{ + statuses: []vcs.CommitStatus{ {Status: "failure", Context: "ci/build", Description: "Build failed"}, {Status: "failure", Context: "ci/test", Description: "Tests failed"}, }, @@ -563,7 +560,7 @@ func TestEvaluateCIStatus(t *testing.T) { }, { name: "mixed with pending and failure", - statuses: []gitea.CommitStatus{ + statuses: []vcs.CommitStatus{ {Status: "success", Context: "ci/build", Description: "Build passed"}, {Status: "pending", Context: "ci/deploy", Description: "Deploying"}, {Status: "failure", Context: "ci/test", Description: "Tests failed"}, @@ -792,7 +789,7 @@ func TestMainSubprocess_InvalidReviewerName(t *testing.T) { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) os.Args = []string{"review-bot", - "--gitea-url", "http://localhost", + "--vcs-url", "http://localhost", "--repo", "owner/repo", "--pr", "1", "--reviewer-name", "invalid name", @@ -820,7 +817,7 @@ func TestMainSubprocess_InvalidRepo(t *testing.T) { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) os.Args = []string{"review-bot", - "--gitea-url", "http://localhost", + "--vcs-url", "http://localhost", "--repo", "invalidrepo", "--pr", "1", "--reviewer-token", "tok", @@ -847,7 +844,7 @@ func TestMainSubprocess_InvalidPRNumber(t *testing.T) { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) os.Args = []string{"review-bot", - "--gitea-url", "http://localhost", + "--vcs-url", "http://localhost", "--repo", "owner/repo", "--pr", "notanumber", "--reviewer-token", "tok", @@ -874,7 +871,7 @@ func TestMainSubprocess_InvalidTemperature(t *testing.T) { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) os.Args = []string{"review-bot", - "--gitea-url", "http://localhost", + "--vcs-url", "http://localhost", "--repo", "owner/repo", "--pr", "1", "--reviewer-token", "tok", @@ -902,7 +899,7 @@ func TestMainSubprocess_InvalidProvider(t *testing.T) { if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) os.Args = []string{"review-bot", - "--gitea-url", "http://localhost", + "--vcs-url", "http://localhost", "--repo", "owner/repo", "--pr", "1", "--reviewer-token", "tok", @@ -926,7 +923,35 @@ func TestMainSubprocess_InvalidProvider(t *testing.T) { } } -// cleanEnv returns environ without any GITEA/LLM/REVIEWER env vars that would +func TestMainSubprocess_InvalidVCSProvider(t *testing.T) { + if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" { + flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError) + os.Args = []string{"review-bot", + "--provider", "invalid", + "--vcs-url", "http://localhost", + "--repo", "owner/repo", + "--pr", "1", + "--reviewer-token", "tok", + "--llm-base-url", "http://localhost", + "--llm-api-key", "key", + "--llm-model", "model", + } + main() + return + } + + cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidVCSProvider") + cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1") + out, err := cmd.CombinedOutput() + if err == nil { + t.Fatal("expected non-zero exit with invalid VCS provider") + } + if !strings.Contains(string(out), "invalid --provider") { + t.Errorf("expected error about invalid --provider, got: %s", out) + } +} + +// cleanEnv returns environ without any GITEA/LLM/REVIEWER/VCS env vars that would // interfere with testing missing-flag scenarios. func cleanEnv() []string { var env []string @@ -934,6 +959,7 @@ func cleanEnv() []string { key := strings.SplitN(e, "=", 2)[0] switch { case strings.HasPrefix(key, "GITEA_"), + strings.HasPrefix(key, "VCS_"), strings.HasPrefix(key, "LLM_"), strings.HasPrefix(key, "REVIEWER_"), strings.HasPrefix(key, "PR_"), @@ -951,12 +977,12 @@ func cleanEnv() []string { } func TestFindAllOwnReviews(t *testing.T) { - reviews := []gitea.Review{ - {ID: 1, Body: "\nfirst review"}, - {ID: 2, Body: "\nother bot"}, - {ID: 3, Body: "\nsecond review"}, - {ID: 4, Body: "~~Original review~~\n\nsuperseded"}, - {ID: 5, Body: "\nthird review"}, + reviews := []vcs.Review{ + makeReview(1, "bot", "APPROVED", false, "\nfirst review"), + makeReview(2, "bot", "APPROVED", false, "\nother bot"), + makeReview(3, "bot", "APPROVED", false, "\nsecond review"), + makeReview(4, "bot", "APPROVED", false, "~~Original review~~\n\nsuperseded"), + makeReview(5, "bot", "APPROVED", false, "\nthird review"), } got := findAllOwnReviews(reviews, "") @@ -1020,3 +1046,23 @@ func TestShouldSkipStaleReview(t *testing.T) { }) } } + +func TestVerdictToEvent(t *testing.T) { + tests := []struct { + verdict string + want vcs.ReviewEvent + }{ + {"APPROVE", vcs.ReviewEventApprove}, + {"REQUEST_CHANGES", vcs.ReviewEventRequestChanges}, + {"COMMENT", vcs.ReviewEventComment}, + {"other", vcs.ReviewEventComment}, + {"", vcs.ReviewEventComment}, + } + + for _, tc := range tests { + got := verdictToEvent(tc.verdict) + if got != tc.want { + t.Errorf("verdictToEvent(%q) = %q, want %q", tc.verdict, got, tc.want) + } + } +} diff --git a/github/reviews.go b/github/reviews.go new file mode 100644 index 0000000..c677679 --- /dev/null +++ b/github/reviews.go @@ -0,0 +1,236 @@ +package github + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strings" + + "gitea.weiker.me/rodin/review-bot/vcs" +) + +// reviewResponse is the GitHub API response for a pull request review. +type reviewResponse struct { + ID int64 `json:"id"` + Body string `json:"body"` + User struct { + Login string `json:"login"` + } `json:"user"` + State string `json:"state"` + CommitID string `json:"commit_id"` +} + +// reviewCreateRequest is the GitHub API request body for creating a pull request review. +type reviewCreateRequest struct { + Body string `json:"body"` + Event string `json:"event"` + Comments []reviewCommentCreate `json:"comments,omitempty"` + CommitID string `json:"commit_id,omitempty"` +} + +// reviewCommentCreate is a single inline comment in a review creation request. +type reviewCommentCreate struct { + Path string `json:"path"` + Position int `json:"position"` + Body string `json:"body"` +} + +// dismissReviewRequest is the GitHub API request body for dismissing a review. +type dismissReviewRequest struct { + Message string `json:"message"` + Event string `json:"event"` +} + +// userResponse is the GitHub API response for the authenticated user. +type userResponse struct { + Login string `json:"login"` +} + +// translateReviewEvent converts a vcs.ReviewEvent to the GitHub API event string. +func translateReviewEvent(event vcs.ReviewEvent) string { + switch event { + case vcs.ReviewEventApprove: + return "APPROVE" + case vcs.ReviewEventRequestChanges: + return "REQUEST_CHANGES" + case vcs.ReviewEventComment: + return "COMMENT" + default: + return string(event) + } +} + +// PostReview creates a new review on a pull request. +func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, req vcs.ReviewRequest) (*vcs.Review, error) { + reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number) + + payload := reviewCreateRequest{ + Body: req.Body, + Event: translateReviewEvent(req.Event), + } + + for _, comment := range req.Comments { + rc := reviewCommentCreate{ + Path: comment.Path, + Position: comment.Position, + Body: comment.Body, + } + payload.Comments = append(payload.Comments, rc) + // Use CommitID from the first comment that has one + if payload.CommitID == "" && comment.CommitID != "" { + payload.CommitID = comment.CommitID + } + } + + body, err := c.doJSONRequest(ctx, http.MethodPost, reqURL, payload) + if err != nil { + return nil, fmt.Errorf("post review: %w", err) + } + + var resp reviewResponse + if err := json.Unmarshal(body, &resp); err != nil { + return nil, fmt.Errorf("parse review response: %w", err) + } + + return &vcs.Review{ + ID: resp.ID, + Body: resp.Body, + User: vcs.UserInfo{Login: resp.User.Login}, + State: resp.State, + CommitID: resp.CommitID, + }, nil +} + +// ListReviews lists all reviews on a pull request. +func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcs.Review, error) { + var allReviews []vcs.Review + + for page := 1; page <= 100; page++ { + reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews?per_page=100&page=%d", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, page) + body, err := c.doGet(ctx, reqURL) + if err != nil { + return nil, fmt.Errorf("list reviews page %d: %w", page, err) + } + var reviews []reviewResponse + if err := json.Unmarshal(body, &reviews); err != nil { + return nil, fmt.Errorf("parse reviews JSON: %w", err) + } + if len(reviews) == 0 { + break + } + for _, r := range reviews { + allReviews = append(allReviews, vcs.Review{ + ID: r.ID, + Body: r.Body, + User: vcs.UserInfo{Login: r.User.Login}, + State: r.State, + CommitID: r.CommitID, + }) + } + if len(reviews) < 100 { + break + } + } + + return allReviews, nil +} + +// DeleteReview deletes a review from a pull request. +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) + _, err := c.doRequest(ctx, http.MethodDelete, reqURL, "") + if err != nil { + return fmt.Errorf("delete review: %w", err) + } + return nil +} + +// DismissReview dismisses a review on a pull request with a message. +func (c *Client) DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error { + reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d/dismissals", + c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID) + + payload := dismissReviewRequest{ + Message: message, + Event: "DISMISS", + } + + _, err := c.doJSONRequest(ctx, http.MethodPut, reqURL, payload) + if err != nil { + return fmt.Errorf("dismiss review: %w", err) + } + return nil +} + +// GetAuthenticatedUser returns the login name of the authenticated user. +func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) { + reqURL := fmt.Sprintf("%s/user", c.baseURL) + body, err := c.doGet(ctx, reqURL) + if err != nil { + return "", fmt.Errorf("get authenticated user: %w", err) + } + var resp userResponse + if err := json.Unmarshal(body, &resp); err != nil { + return "", fmt.Errorf("parse user response: %w", err) + } + return resp.Login, nil +} + +// 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) { + const maxErrorBodyBytes = 4 * 1024 + + jsonBody, err := json.Marshal(payload) + if err != nil { + return nil, fmt.Errorf("marshal request body: %w", err) + } + + if c.token != "" && !c.allowInsecureHTTP { + parsed, err := url.Parse(reqURL) + if err != nil { + return nil, fmt.Errorf("parse request URL: %w", err) + } + if !strings.EqualFold(parsed.Scheme, "https") { + return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL) + } + } + + req, err := http.NewRequestWithContext(ctx, method, reqURL, bytes.NewReader(jsonBody)) + if err != nil { + return nil, fmt.Errorf("create request: %w", err) + } + if c.token != "" { + req.Header.Set("Authorization", "Bearer "+c.token) + } + req.Header.Set("User-Agent", userAgent) + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("Content-Type", "application/json") + + resp, err := c.httpClient.Do(req) + if err != nil { + return nil, fmt.Errorf("do request: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode >= 200 && resp.StatusCode < 300 { + body, err := io.ReadAll(io.LimitReader(resp.Body, int64(maxResponseBytes)+1)) + if err != nil { + return nil, fmt.Errorf("read response body: %w", err) + } + if len(body) > maxResponseBytes { + return nil, fmt.Errorf("response body exceeded %d bytes", maxResponseBytes) + } + return body, nil + } + + errBody, _ := io.ReadAll(io.LimitReader(resp.Body, int64(maxErrorBodyBytes))) + return nil, &APIError{StatusCode: resp.StatusCode, Body: string(errBody)} +} diff --git a/review/formatter.go b/review/formatter.go index d2e109f..0b2271f 100644 --- a/review/formatter.go +++ b/review/formatter.go @@ -10,17 +10,6 @@ func FormatMarkdown(result *ReviewResult, reviewerName string) string { return FormatMarkdownWithDisplay(result, reviewerName, reviewerName) } -// GiteaEvent converts the verdict to the Gitea API event string. -func GiteaEvent(verdict string) string { - switch verdict { - case "APPROVE": - return "APPROVED" - case "REQUEST_CHANGES": - return "REQUEST_CHANGES" - default: - return "COMMENT" - } -} // FormatMarkdownWithDisplay formats a ReviewResult with separate display name and sentinel name. // Note: displayName is not HTML-escaped as Gitea sanitizes rendered Markdown. diff --git a/review/formatter_test.go b/review/formatter_test.go index 7684e88..be3bdee 100644 --- a/review/formatter_test.go +++ b/review/formatter_test.go @@ -98,24 +98,6 @@ func TestFormatMarkdown_SpecialChars(t *testing.T) { } } -func TestGiteaEvent(t *testing.T) { - tests := []struct { - verdict string - expected string - }{ - {"APPROVE", "APPROVED"}, - {"REQUEST_CHANGES", "REQUEST_CHANGES"}, - {"UNKNOWN", "COMMENT"}, - {"", "COMMENT"}, - } - - for _, tc := range tests { - got := GiteaEvent(tc.verdict) - if got != tc.expected { - t.Errorf("GiteaEvent(%q) = %q, want %q", tc.verdict, got, tc.expected) - } - } -} func TestFormatMarkdown_Sentinel(t *testing.T) { result := &ReviewResult{ From 02920b685bc860af65706aea6de4db888a7516fc Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 01:51:21 -0700 Subject: [PATCH 02/17] 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 125d4dc..210120c 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. @@ -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. 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", From c4af35cd784b042837a1a8cea63f035cefcfc423 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 02:23:24 -0700 Subject: [PATCH 03/17] fix(cmd,github): address review feedback on PR #106 - Replace panic() with fmt.Fprintf+os.Exit(1) in provider switch default (repo convention: never panic) - Remove spurious 'event' field from DismissReview payload (GitHub dismiss endpoint only documents 'message') - Change translateReviewEvent default to return 'COMMENT' as canonical fallback instead of passing unknown events through to GitHub API - Refactor supersedeOldReviews to use explicit switch/case with default error for exhaustiveness --- cmd/review-bot/main.go | 11 ++++++++--- github/reviews.go | 4 +--- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 210120c..bad4b8a 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -170,7 +170,8 @@ func main() { } client = github.NewClient(*reviewerToken, ghBaseURL) default: - panic("unreachable: unhandled provider " + *provider) + fmt.Fprintf(os.Stderr, "Error: unhandled provider %q\n", *provider) + os.Exit(1) } slog.Info("VCS client initialized", "provider", *provider) @@ -523,7 +524,8 @@ func verdictToEvent(verdict string) vcs.ReviewEvent { // 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) error { - if provider == "github" { + switch provider { + case "github": 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) @@ -532,9 +534,12 @@ func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsUR } } return nil + case "gitea": + // Gitea: EditComment + ResolveComment flow + default: + return fmt.Errorf("supersedeOldReviews: unsupported provider %q", provider) } - // Gitea: existing EditComment + ResolveComment flow giteaAdapter, ok := client.(*gitea.Adapter) if !ok { return fmt.Errorf("expected gitea.Adapter for gitea provider, got %T", client) diff --git a/github/reviews.go b/github/reviews.go index c6ed536..1d8066f 100644 --- a/github/reviews.go +++ b/github/reviews.go @@ -42,7 +42,6 @@ type reviewCommentCreate struct { // dismissReviewRequest is the GitHub API request body for dismissing a review. type dismissReviewRequest struct { Message string `json:"message"` - Event string `json:"event"` } // userResponse is the GitHub API response for the authenticated user. @@ -60,7 +59,7 @@ func translateReviewEvent(event vcs.ReviewEvent) string { case vcs.ReviewEventComment: return "COMMENT" default: - return string(event) + return "COMMENT" } } @@ -161,7 +160,6 @@ func (c *Client) DismissReview(ctx context.Context, owner, repo string, number i payload := dismissReviewRequest{ Message: message, - Event: "DISMISS", } _, err := c.doJSONRequest(ctx, http.MethodPut, reqURL, payload) From 28e63a233877b0fa4aebf4f9e266859cc41540fd Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 02:35:59 -0700 Subject: [PATCH 04/17] fix(cmd): clarify empty gitea case control flow in supersedeOldReviews The empty case "gitea": body exits the switch and continues to the Gitea-specific logic below. Replace the vague comment with an explicit note about the fall-through intent, per self-review feedback. --- cmd/review-bot/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index bad4b8a..359d080 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -535,7 +535,7 @@ func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsUR } return nil case "gitea": - // Gitea: EditComment + ResolveComment flow + // Fall through to Gitea-specific logic below the switch. default: return fmt.Errorf("supersedeOldReviews: unsupported provider %q", provider) } From 4c189d18a2698660b64971fad836f29b8038a57e Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 03:07:53 -0700 Subject: [PATCH 05/17] fix(review): address inline review feedback on PR #106 - Reword misleading 'Fall through' comment to 'Continue to' in supersedeOldReviews (comment #18704) - Add shared-pointer explanation comment for --gitea-url alias registration (comment #18703) - Add comment clarifying CommitID same-commit expectation in PostReview (comment #18705) - Rename 'hidden alias' to 'backward-compatible alias' in flag comment (comment #18708) --- cmd/review-bot/main.go | 8 +++++--- github/reviews.go | 3 ++- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 359d080..673b076 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -59,7 +59,7 @@ func main() { provider := flag.String("provider", envOrDefault("VCS_PROVIDER", "gitea"), "VCS provider: gitea or github") baseURL := flag.String("base-url", envOrDefault("VCS_BASE_URL", ""), "VCS API base URL (for github provider; defaults to https://api.github.com)") vcsURL := flag.String("vcs-url", envOrDefault("VCS_URL", envOrDefault("GITEA_URL", envOrDefault("GITHUB_SERVER_URL", ""))), "VCS instance URL (Gitea) [deprecated alias: --gitea-url]") - // Keep --gitea-url as hidden alias (flag package doesn't support aliases natively, handle below) + // Keep --gitea-url as backward-compatible alias (flag package doesn't support aliases natively, handle below) repo := flag.String("repo", envOrDefault("VCS_REPO", envOrDefault("GITEA_REPO", envOrDefault("GITHUB_REPOSITORY", ""))), "Repository (owner/name)") prNum := flag.String("pr", envOrDefault("PR_NUMBER", ""), "Pull request number") reviewerName := flag.String("reviewer-name", envOrDefault("REVIEWER_NAME", ""), "Reviewer display name") @@ -84,7 +84,9 @@ 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 deprecated alias for --vcs-url + // 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. flag.StringVar(vcsURL, "gitea-url", *vcsURL, "Deprecated: use --vcs-url instead") flag.Parse() @@ -535,7 +537,7 @@ func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsUR } return nil case "gitea": - // Fall through to Gitea-specific logic below the switch. + // Continue to Gitea-specific logic below the switch. default: return fmt.Errorf("supersedeOldReviews: unsupported provider %q", provider) } diff --git a/github/reviews.go b/github/reviews.go index 1d8066f..1f877e0 100644 --- a/github/reviews.go +++ b/github/reviews.go @@ -80,7 +80,8 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, Body: comment.Body, } payload.Comments = append(payload.Comments, rc) - // Use CommitID from the first comment that has one + // Use CommitID from the first comment that has one. + // All comments in a single review are expected to reference the same commit. if payload.CommitID == "" && comment.CommitID != "" { payload.CommitID = comment.CommitID } From a89dce1c5292b647a3bbac371f4f4b44c5de912d Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 03:29:45 -0700 Subject: [PATCH 06/17] fix(review): address bot review feedback on PR #106 - Document --gitea-url/--vcs-url last-one-wins behavior when both flags are passed simultaneously (sonnet MINOR #1) - Move doJSONRequest from github/reviews.go to github/client.go where other HTTP helpers live (sonnet MINOR #2) - Return joined error from supersedeOldReviews GitHub case instead of silently swallowing DismissReview failures (sonnet MINOR #3) - Fix evaluateCIStatus to distinguish 'all checks passed' from 'no failures (N pending)' to avoid misleading status (gpt MINOR #2) - Extract reviewsPerPage and maxReviewPages named constants for ListReviews pagination (gpt NIT #3) --- cmd/review-bot/main.go | 16 ++- cmd/review-bot/main_test.go | 2 +- github/client.go | 55 +++++++++ github/reviews.go | 237 ------------------------------------ 4 files changed, 70 insertions(+), 240 deletions(-) delete mode 100644 github/reviews.go diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 673b076..d102284 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "flag" "fmt" "log/slog" @@ -87,6 +88,9 @@ func main() { // 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. flag.StringVar(vcsURL, "gitea-url", *vcsURL, "Deprecated: use --vcs-url instead") flag.Parse() @@ -528,14 +532,17 @@ func verdictToEvent(verdict string) vcs.ReviewEvent { 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 nil + return errors.Join(errs...) case "gitea": // Continue to Gitea-specific logic below the switch. default: @@ -687,18 +694,20 @@ func isPatternFile(path string) bool { } // evaluateCIStatus checks if all CI statuses indicate success. +// Returns passed=true if no checks have failed (pending checks are not treated as failures). func evaluateCIStatus(statuses []vcs.CommitStatus) (passed bool, details string) { if len(statuses) == 0 { return true, "no CI statuses found" } var failed []string + var pending int for _, s := range statuses { switch s.Status { case "success": // good case "pending": - // treat pending as not-failed + pending++ case "failure", "error": failed = append(failed, fmt.Sprintf("%s: %s", s.Context, s.Description)) } @@ -707,6 +716,9 @@ func evaluateCIStatus(statuses []vcs.CommitStatus) (passed bool, details string) if len(failed) > 0 { return false, strings.Join(failed, "; ") } + if pending > 0 { + return true, fmt.Sprintf("no failures (%d pending)", pending) + } return true, "all checks passed" } diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index 1fcae71..67071c2 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -547,7 +547,7 @@ func TestEvaluateCIStatus(t *testing.T) { {Status: "success", Context: "ci/test", Description: "Tests passed"}, }, wantPassed: true, - wantSubstr: "all checks passed", + wantSubstr: "no failures", }, { name: "multiple failures", diff --git a/github/client.go b/github/client.go index f958bfa..fe7c339 100644 --- a/github/client.go +++ b/github/client.go @@ -5,6 +5,7 @@ package github import ( "bytes" + "encoding/json" "context" "errors" "fmt" @@ -383,4 +384,58 @@ func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, r opts.extraHeaders = map[string]string{"Content-Type": "application/json"} } return c.doRequestCore(ctx, method, reqURL, opts) + +} +// doJSONRequest performs an HTTP request with a JSON body and returns the response body. +// It handles HTTPS validation, authentication, and response reading. +// This is a general-purpose helper used by any method that needs to send JSON payloads +// (e.g. PostReview, DismissReview). +func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, payload any) ([]byte, error) { + const maxErrorBodyBytes = 4 * 1024 + + jsonBody, err := json.Marshal(payload) + if err != nil { + return nil, fmt.Errorf("marshal request body: %w", err) + } + + if c.token != "" && !c.allowInsecureHTTP { + parsed, err := url.Parse(reqURL) + if err != nil { + return nil, fmt.Errorf("parse request URL: %w", err) + } + if !strings.EqualFold(parsed.Scheme, "https") { + return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL) + } + } + + req, err := http.NewRequestWithContext(ctx, method, reqURL, bytes.NewReader(jsonBody)) + if err != nil { + return nil, fmt.Errorf("create request: %w", err) + } + if c.token != "" { + req.Header.Set("Authorization", "Bearer "+c.token) + } + req.Header.Set("User-Agent", userAgent) + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("Content-Type", "application/json") + + resp, err := c.httpClient.Do(req) + if err != nil { + return nil, fmt.Errorf("do request: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode >= 200 && resp.StatusCode < 300 { + body, err := io.ReadAll(io.LimitReader(resp.Body, int64(maxResponseBytes)+1)) + if err != nil { + return nil, fmt.Errorf("read response body: %w", err) + } + if len(body) > maxResponseBytes { + return nil, fmt.Errorf("response body exceeded %d bytes", maxResponseBytes) + } + return body, nil + } + + errBody, _ := io.ReadAll(io.LimitReader(resp.Body, int64(maxErrorBodyBytes))) + return nil, &APIError{StatusCode: resp.StatusCode, Body: string(errBody)} } diff --git a/github/reviews.go b/github/reviews.go deleted file mode 100644 index 1f877e0..0000000 --- a/github/reviews.go +++ /dev/null @@ -1,237 +0,0 @@ -package github - -import ( - "bytes" - "context" - "encoding/json" - "fmt" - "io" - "net/http" - "net/url" - "strings" - - "gitea.weiker.me/rodin/review-bot/vcs" -) - -// reviewResponse is the GitHub API response for a pull request review. -type reviewResponse struct { - ID int64 `json:"id"` - Body string `json:"body"` - User struct { - Login string `json:"login"` - } `json:"user"` - State string `json:"state"` - CommitID string `json:"commit_id"` -} - -// reviewCreateRequest is the GitHub API request body for creating a pull request review. -type reviewCreateRequest struct { - Body string `json:"body"` - Event string `json:"event"` - Comments []reviewCommentCreate `json:"comments,omitempty"` - CommitID string `json:"commit_id,omitempty"` -} - -// reviewCommentCreate is a single inline comment in a review creation request. -type reviewCommentCreate struct { - Path string `json:"path"` - Position int `json:"position"` - Body string `json:"body"` -} - -// dismissReviewRequest is the GitHub API request body for dismissing a review. -type dismissReviewRequest struct { - Message string `json:"message"` -} - -// userResponse is the GitHub API response for the authenticated user. -type userResponse struct { - Login string `json:"login"` -} - -// translateReviewEvent converts a vcs.ReviewEvent to the GitHub API event string. -func translateReviewEvent(event vcs.ReviewEvent) string { - switch event { - case vcs.ReviewEventApprove: - return "APPROVE" - case vcs.ReviewEventRequestChanges: - return "REQUEST_CHANGES" - case vcs.ReviewEventComment: - return "COMMENT" - default: - return "COMMENT" - } -} - -// PostReview creates a new review on a pull request. -func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, req vcs.ReviewRequest) (*vcs.Review, error) { - reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews", - c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number) - - payload := reviewCreateRequest{ - Body: req.Body, - Event: translateReviewEvent(req.Event), - } - - for _, comment := range req.Comments { - rc := reviewCommentCreate{ - Path: comment.Path, - Position: comment.Position, - Body: comment.Body, - } - payload.Comments = append(payload.Comments, rc) - // Use CommitID from the first comment that has one. - // All comments in a single review are expected to reference the same commit. - if payload.CommitID == "" && comment.CommitID != "" { - payload.CommitID = comment.CommitID - } - } - - body, err := c.doJSONRequest(ctx, http.MethodPost, reqURL, payload) - if err != nil { - return nil, fmt.Errorf("post review: %w", err) - } - - var resp reviewResponse - if err := json.Unmarshal(body, &resp); err != nil { - return nil, fmt.Errorf("parse review response: %w", err) - } - - return &vcs.Review{ - ID: resp.ID, - Body: resp.Body, - User: vcs.UserInfo{Login: resp.User.Login}, - State: resp.State, - CommitID: resp.CommitID, - }, nil -} - -// ListReviews lists all reviews on a pull request. -func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcs.Review, error) { - var allReviews []vcs.Review - - for page := 1; page <= 100; page++ { - reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews?per_page=100&page=%d", - c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, page) - body, err := c.doGet(ctx, reqURL) - if err != nil { - return nil, fmt.Errorf("list reviews page %d: %w", page, err) - } - var reviews []reviewResponse - if err := json.Unmarshal(body, &reviews); err != nil { - return nil, fmt.Errorf("parse reviews JSON: %w", err) - } - if len(reviews) == 0 { - break - } - for _, r := range reviews { - allReviews = append(allReviews, vcs.Review{ - ID: r.ID, - Body: r.Body, - User: vcs.UserInfo{Login: r.User.Login}, - State: r.State, - CommitID: r.CommitID, - }) - } - if len(reviews) < 100 { - break - } - } - - return allReviews, nil -} - -// 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) - _, err := c.doRequest(ctx, http.MethodDelete, reqURL, "") - if err != nil { - return fmt.Errorf("delete review: %w", err) - } - return nil -} - -// DismissReview dismisses a review on a pull request with a message. -func (c *Client) DismissReview(ctx context.Context, owner, repo string, number int, reviewID int64, message string) error { - reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d/dismissals", - c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID) - - payload := dismissReviewRequest{ - Message: message, - } - - _, err := c.doJSONRequest(ctx, http.MethodPut, reqURL, payload) - if err != nil { - return fmt.Errorf("dismiss review: %w", err) - } - return nil -} - -// GetAuthenticatedUser returns the login name of the authenticated user. -func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) { - reqURL := fmt.Sprintf("%s/user", c.baseURL) - body, err := c.doGet(ctx, reqURL) - if err != nil { - return "", fmt.Errorf("get authenticated user: %w", err) - } - var resp userResponse - if err := json.Unmarshal(body, &resp); err != nil { - return "", fmt.Errorf("parse user response: %w", err) - } - return resp.Login, nil -} - -// 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 any) ([]byte, error) { - const maxErrorBodyBytes = 4 * 1024 - - jsonBody, err := json.Marshal(payload) - if err != nil { - return nil, fmt.Errorf("marshal request body: %w", err) - } - - if c.token != "" && !c.allowInsecureHTTP { - parsed, err := url.Parse(reqURL) - if err != nil { - return nil, fmt.Errorf("parse request URL: %w", err) - } - if !strings.EqualFold(parsed.Scheme, "https") { - return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL) - } - } - - req, err := http.NewRequestWithContext(ctx, method, reqURL, bytes.NewReader(jsonBody)) - if err != nil { - return nil, fmt.Errorf("create request: %w", err) - } - if c.token != "" { - req.Header.Set("Authorization", "Bearer "+c.token) - } - req.Header.Set("User-Agent", userAgent) - req.Header.Set("Accept", "application/vnd.github+json") - req.Header.Set("Content-Type", "application/json") - - resp, err := c.httpClient.Do(req) - if err != nil { - return nil, fmt.Errorf("do request: %w", err) - } - defer resp.Body.Close() - - if resp.StatusCode >= 200 && resp.StatusCode < 300 { - body, err := io.ReadAll(io.LimitReader(resp.Body, int64(maxResponseBytes)+1)) - if err != nil { - return nil, fmt.Errorf("read response body: %w", err) - } - if len(body) > maxResponseBytes { - return nil, fmt.Errorf("response body exceeded %d bytes", maxResponseBytes) - } - return body, nil - } - - errBody, _ := io.ReadAll(io.LimitReader(resp.Body, int64(maxErrorBodyBytes))) - return nil, &APIError{StatusCode: resp.StatusCode, Body: string(errBody)} -} From a30ee7df6e7bcce51d650e959467a9b797e4c3c6 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 03:39:53 -0700 Subject: [PATCH 07/17] fix: address review feedback on PR #106 - Add 429 rate-limit retry logic to doJSONRequest (matching doRequest behavior) so write operations (PostReview, DismissReview) properly retry when rate-limited by GitHub - Remove redundant explicit case for ReviewEventComment in translateReviewEvent (default already handles it) - Add ordering comment on --gitea-url alias registration explaining the dependency on registration-before-parse evaluation order - Add tests for doJSONRequest retry/exhaust behavior --- cmd/review-bot/main.go | 4 ++ github/client.go | 107 ++++++++++++++++++++++++++++++++--------- github/client_test.go | 53 ++++++++++++++++++++ 3 files changed, 140 insertions(+), 24 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index d102284..e919451 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -91,6 +91,10 @@ func main() { // 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. flag.StringVar(vcsURL, "gitea-url", *vcsURL, "Deprecated: use --vcs-url instead") flag.Parse() diff --git a/github/client.go b/github/client.go index fe7c339..bc302a4 100644 --- a/github/client.go +++ b/github/client.go @@ -387,11 +387,15 @@ func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, r } // doJSONRequest performs an HTTP request with a JSON body and returns the response body. -// It handles HTTPS validation, authentication, and response reading. +// It handles HTTPS validation, authentication, response reading, and retries on HTTP 429 +// rate limit responses (matching the retry behavior of doRequest). // This is a general-purpose helper used by any method that needs to send JSON payloads // (e.g. PostReview, DismissReview). func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, payload any) ([]byte, error) { - const maxErrorBodyBytes = 4 * 1024 + const ( + maxErrorBodyBytes = 4 * 1024 + maxRetryAfter = 120 * time.Second + ) jsonBody, err := json.Marshal(payload) if err != nil { @@ -408,34 +412,89 @@ func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, paylo } } - req, err := http.NewRequestWithContext(ctx, method, reqURL, bytes.NewReader(jsonBody)) - if err != nil { - return nil, fmt.Errorf("create request: %w", err) + defaultBackoff := []time.Duration{1 * time.Second, 2 * time.Second} + var backoff []time.Duration + if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 { + backoff = make([]time.Duration, len(c.retryBackoff)) + copy(backoff, c.retryBackoff) + } else { + backoff = make([]time.Duration, len(defaultBackoff)) + copy(backoff, defaultBackoff) } - if c.token != "" { - req.Header.Set("Authorization", "Bearer "+c.token) - } - req.Header.Set("User-Agent", userAgent) - req.Header.Set("Accept", "application/vnd.github+json") - req.Header.Set("Content-Type", "application/json") - resp, err := c.httpClient.Do(req) - if err != nil { - return nil, fmt.Errorf("do request: %w", err) - } - defer resp.Body.Close() + var lastErr error + for attempt := 0; attempt < maxRetryAttempts; attempt++ { + if attempt > 0 { + var delay time.Duration + if attempt-1 < len(backoff) { + delay = backoff[attempt-1] + } + if delay > 0 { + timer := time.NewTimer(delay) + select { + case <-timer.C: + timer.Stop() + case <-ctx.Done(): + timer.Stop() + return nil, ctx.Err() + } + } + } - if resp.StatusCode >= 200 && resp.StatusCode < 300 { - body, err := io.ReadAll(io.LimitReader(resp.Body, int64(maxResponseBytes)+1)) + req, err := http.NewRequestWithContext(ctx, method, reqURL, bytes.NewReader(jsonBody)) if err != nil { - return nil, fmt.Errorf("read response body: %w", err) + return nil, fmt.Errorf("create request: %w", err) } - if len(body) > maxResponseBytes { - return nil, fmt.Errorf("response body exceeded %d bytes", maxResponseBytes) + if c.token != "" { + req.Header.Set("Authorization", "Bearer "+c.token) } - return body, nil + req.Header.Set("User-Agent", userAgent) + req.Header.Set("Accept", "application/vnd.github+json") + req.Header.Set("Content-Type", "application/json") + + resp, err := c.httpClient.Do(req) + if err != nil { + return nil, fmt.Errorf("do request: %w", err) + } + + respStatus := resp.StatusCode + retryAfterHeader := resp.Header.Get("Retry-After") + + body, done, handleErr := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes) + if done { + return body, handleErr + } + lastErr = handleErr + + // Retry on 429 rate limit + if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 { + if ra := retryAfterHeader; ra != "" { + if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 { + delay := time.Duration(seconds) * time.Second + if delay > maxRetryAfter { + delay = maxRetryAfter + } + if attempt < len(backoff) { + backoff[attempt] = delay + } + } else if retryAt, err := http.ParseTime(ra); err == nil { + delay := time.Until(retryAt) + if delay < 0 { + delay = 0 + } + if delay > maxRetryAfter { + delay = maxRetryAfter + } + if attempt < len(backoff) { + backoff[attempt] = delay + } + } + } + continue + } + + return nil, lastErr } - errBody, _ := io.ReadAll(io.LimitReader(resp.Body, int64(maxErrorBodyBytes))) - return nil, &APIError{StatusCode: resp.StatusCode, Body: string(errBody)} + return nil, lastErr } diff --git a/github/client_test.go b/github/client_test.go index ab6f4d1..7701e92 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -2,6 +2,7 @@ package github import ( "context" + "errors" "net/http" "net/http/httptest" "net/url" @@ -592,3 +593,55 @@ func TestSetRetryBackoff_RejectsInvalidLength(t *testing.T) { t.Fatalf("unexpected error for valid backoff: %v", err) } } + +func TestDoJSONRequest_429Retry(t *testing.T) { + attempts := 0 + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + attempts++ + if attempts < 3 { + w.WriteHeader(429) + w.Write([]byte(`{"message":"rate limit exceeded"}`)) + return + } + w.WriteHeader(200) + w.Write([]byte(`{"id":1}`)) + })) + defer ts.Close() + + c := NewClient("token", ts.URL, AllowInsecureHTTP()) + c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) + + body, err := c.doJSONRequest(context.Background(), http.MethodPost, ts.URL+"/test", map[string]string{"key": "val"}) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if attempts != 3 { + t.Errorf("expected 3 attempts, got %d", attempts) + } + if string(body) != `{"id":1}` { + t.Errorf("unexpected body: %s", body) + } +} + +func TestDoJSONRequest_429ExhaustsRetries(t *testing.T) { + ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(429) + w.Write([]byte(`{"message":"rate limit"}`)) + })) + defer ts.Close() + + c := NewClient("token", ts.URL, AllowInsecureHTTP()) + c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) + + _, err := c.doJSONRequest(context.Background(), http.MethodPost, ts.URL+"/test", map[string]string{"key": "val"}) + if err == nil { + t.Fatal("expected error after exhausting retries") + } + var apiErr *APIError + if !errors.As(err, &apiErr) { + t.Fatalf("expected APIError, got %T: %v", err, err) + } + if apiErr.StatusCode != 429 { + t.Errorf("expected 429, got %d", apiErr.StatusCode) + } +} From d40902771e5f3edd6e6917483f6c3585a15ff93a Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 03:54:37 -0700 Subject: [PATCH 08/17] fix: address self-review findings - Remove dead code: findOwnReview (replaced by findAllOwnReviews) - Check SetRetryBackoff return value in doJSONRequest tests - Extract doWithRetry shared helper to eliminate ~100 lines of duplicated 429-retry/backoff/Retry-After logic between doRequest and doJSONRequest - Fix import order: context before encoding/json (goimports) - Add slog.Warn when ListReviews hits maxReviewPages limit --- cmd/review-bot/main.go | 16 ----- cmd/review-bot/main_test.go | 85 -------------------------- github/client.go | 117 +++--------------------------------- github/client_test.go | 8 ++- 4 files changed, 14 insertions(+), 212 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index e919451..417baaf 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -892,22 +892,6 @@ func extractSentinelName(body string) string { return name } -// findOwnReview locates the most recent non-superseded review matching the sentinel. -func findOwnReview(reviews []vcs.Review, sentinel string) *vcs.Review { - var best *vcs.Review - for i := range reviews { - if !strings.Contains(reviews[i].Body, sentinel) { - continue - } - if strings.Contains(reviews[i].Body, "~~Original review~~") { - continue - } - if best == nil || reviews[i].ID > best.ID { - best = &reviews[i] - } - } - return best -} // findAllOwnReviews returns all non-superseded reviews matching the sentinel. func findAllOwnReviews(reviews []vcs.Review, sentinel string) []vcs.Review { diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index 67071c2..b97c58e 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -210,91 +210,6 @@ func TestBuildSupersededBodyShortSHA(t *testing.T) { } } -func TestFindOwnReview(t *testing.T) { - tests := []struct { - name string - reviews []vcs.Review - sentinel string - wantID int64 - wantNil bool - }{ - { - name: "no reviews", - reviews: nil, - sentinel: "", - wantNil: true, - }, - { - name: "found by sentinel", - reviews: []vcs.Review{ - makeReview(42, "bot", "APPROVED", false, "review body\n"), - }, - sentinel: "", - wantID: 42, - }, - { - name: "wrong sentinel", - reviews: []vcs.Review{ - makeReview(42, "bot", "APPROVED", false, "body\n"), - }, - sentinel: "", - wantNil: true, - }, - { - name: "multiple reviews, returns first match", - reviews: []vcs.Review{ - makeReview(10, "bot", "APPROVED", false, "old\n"), - makeReview(20, "bot", "APPROVED", false, "new\n"), - }, - sentinel: "", - wantID: 20, - }, - { - name: "skips superseded review", - reviews: []vcs.Review{ - makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n**Superseded**\n"), - makeReview(20, "bot", "APPROVED", false, "fresh review\n"), - }, - sentinel: "", - wantID: 20, - }, - { - name: "only superseded reviews exist", - reviews: []vcs.Review{ - makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n"), - }, - sentinel: "", - wantNil: true, - }, - { - name: "picks highest ID among matches", - reviews: []vcs.Review{ - makeReview(50, "bot", "APPROVED", false, "v1\n"), - makeReview(30, "bot", "APPROVED", false, "v0\n"), - }, - sentinel: "", - wantID: 50, - }, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - got := findOwnReview(tc.reviews, tc.sentinel) - if tc.wantNil { - if got != nil { - t.Errorf("findOwnReview() = %v, want nil", got) - } - } else { - if got == nil { - t.Fatal("findOwnReview() = nil, want non-nil") - } - if got.ID != tc.wantID { - t.Errorf("findOwnReview().ID = %d, want %d", got.ID, tc.wantID) - } - } - }) - } -} func TestHasSharedToken(t *testing.T) { tests := []struct { diff --git a/github/client.go b/github/client.go index bc302a4..8d41ee7 100644 --- a/github/client.go +++ b/github/client.go @@ -5,8 +5,8 @@ package github import ( "bytes" - "encoding/json" "context" + "encoding/json" "errors" "fmt" "io" @@ -215,6 +215,11 @@ type requestOptions struct { func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts requestOptions) ([]byte, error) { const maxRetryAfter = 120 * time.Second + // maxErrorBodyBytes limits how much of an error response body is stored. + // Kept small (4 KiB) to reduce the risk of sensitive data leakage if callers + // log APIError.Body directly. Error() further truncates to 200 bytes. + const maxErrorBodyBytes = 4 * 1024 + // backoff holds per-attempt delays: backoff[i] is the delay before attempt i+1. // Length must be maxRetryAttempts-1 (one entry per retry gap). // SetRetryBackoff validates at configuration time; the default is always valid. @@ -228,11 +233,6 @@ func (c *Client) doRequestCore(ctx context.Context, method, reqURL string, opts copy(backoff, defaultBackoff) } - // maxErrorBodyBytes limits how much of an error response body is stored. - // Kept small (4 KiB) to reduce the risk of sensitive data leakage if callers - // log APIError.Body directly. Error() further truncates to 200 bytes. - const maxErrorBodyBytes = 4 * 1024 - // Reject non-HTTPS URLs early since the URL is immutable across retries. if c.token != "" && !c.allowInsecureHTTP { parsed, err := url.Parse(reqURL) @@ -387,114 +387,13 @@ func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, r } // doJSONRequest performs an HTTP request with a JSON body and returns the response body. -// It handles HTTPS validation, authentication, response reading, and retries on HTTP 429 -// rate limit responses (matching the retry behavior of doRequest). +// It delegates retry/backoff/429 handling to doRequestWithBody. // This is a general-purpose helper used by any method that needs to send JSON payloads // (e.g. PostReview, DismissReview). func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, payload any) ([]byte, error) { - const ( - maxErrorBodyBytes = 4 * 1024 - maxRetryAfter = 120 * time.Second - ) - jsonBody, err := json.Marshal(payload) if err != nil { return nil, fmt.Errorf("marshal request body: %w", err) } - - if c.token != "" && !c.allowInsecureHTTP { - parsed, err := url.Parse(reqURL) - if err != nil { - return nil, fmt.Errorf("parse request URL: %w", err) - } - if !strings.EqualFold(parsed.Scheme, "https") { - return nil, fmt.Errorf("refusing to send credentials over non-HTTPS URL %q (use AllowInsecureHTTP option for trusted networks)", reqURL) - } - } - - defaultBackoff := []time.Duration{1 * time.Second, 2 * time.Second} - var backoff []time.Duration - if c.retryBackoff != nil && len(c.retryBackoff) == maxRetryAttempts-1 { - backoff = make([]time.Duration, len(c.retryBackoff)) - copy(backoff, c.retryBackoff) - } else { - backoff = make([]time.Duration, len(defaultBackoff)) - copy(backoff, defaultBackoff) - } - - var lastErr error - for attempt := 0; attempt < maxRetryAttempts; attempt++ { - if attempt > 0 { - var delay time.Duration - if attempt-1 < len(backoff) { - delay = backoff[attempt-1] - } - if delay > 0 { - timer := time.NewTimer(delay) - select { - case <-timer.C: - timer.Stop() - case <-ctx.Done(): - timer.Stop() - return nil, ctx.Err() - } - } - } - - req, err := http.NewRequestWithContext(ctx, method, reqURL, bytes.NewReader(jsonBody)) - if err != nil { - return nil, fmt.Errorf("create request: %w", err) - } - if c.token != "" { - req.Header.Set("Authorization", "Bearer "+c.token) - } - req.Header.Set("User-Agent", userAgent) - req.Header.Set("Accept", "application/vnd.github+json") - req.Header.Set("Content-Type", "application/json") - - resp, err := c.httpClient.Do(req) - if err != nil { - return nil, fmt.Errorf("do request: %w", err) - } - - respStatus := resp.StatusCode - retryAfterHeader := resp.Header.Get("Retry-After") - - body, done, handleErr := c.handleResponse(resp, maxResponseBytes, maxErrorBodyBytes) - if done { - return body, handleErr - } - lastErr = handleErr - - // Retry on 429 rate limit - if respStatus == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 { - if ra := retryAfterHeader; ra != "" { - if seconds, err := strconv.Atoi(ra); err == nil && seconds > 0 { - delay := time.Duration(seconds) * time.Second - if delay > maxRetryAfter { - delay = maxRetryAfter - } - if attempt < len(backoff) { - backoff[attempt] = delay - } - } else if retryAt, err := http.ParseTime(ra); err == nil { - delay := time.Until(retryAt) - if delay < 0 { - delay = 0 - } - if delay > maxRetryAfter { - delay = maxRetryAfter - } - if attempt < len(backoff) { - backoff[attempt] = delay - } - } - } - continue - } - - return nil, lastErr - } - - return nil, lastErr + return c.doRequestWithBody(ctx, method, reqURL, jsonBody) } diff --git a/github/client_test.go b/github/client_test.go index 7701e92..8bf02f0 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -609,7 +609,9 @@ func TestDoJSONRequest_429Retry(t *testing.T) { defer ts.Close() c := NewClient("token", ts.URL, AllowInsecureHTTP()) - c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) + if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil { + t.Fatalf("SetRetryBackoff: %v", err) + } body, err := c.doJSONRequest(context.Background(), http.MethodPost, ts.URL+"/test", map[string]string{"key": "val"}) if err != nil { @@ -631,7 +633,9 @@ func TestDoJSONRequest_429ExhaustsRetries(t *testing.T) { defer ts.Close() c := NewClient("token", ts.URL, AllowInsecureHTTP()) - c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}) + if err := c.SetRetryBackoff([]time.Duration{1 * time.Millisecond, 1 * time.Millisecond}); err != nil { + t.Fatalf("SetRetryBackoff: %v", err) + } _, err := c.doJSONRequest(context.Background(), http.MethodPost, ts.URL+"/test", map[string]string{"key": "val"}) if err == nil { From e8664714c41e294630e53127a23c75644dad779b Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 04:14:30 -0700 Subject: [PATCH 09/17] docs(cmd,github): clarify type assertion and parameter usage in review superseding Address sonnet-review feedback on PR #106: - Document that the type assertion in supersedeOldReviews is guaranteed to succeed given the caller's provider switch, with the !ok branch guarding against future refactors (comment 18889). - Clarify that vcsURL is only used in the Gitea path for constructing review permalink URLs (comment 18890). - Add note explaining why the page-limit warning in ListReviews only fires when the final page is full, confirming the logic is intentional (comment 18891). --- cmd/review-bot/main.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 417baaf..e1cd719 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -531,8 +531,12 @@ 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. +// 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": @@ -553,6 +557,11 @@ func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsUR 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) From c5bc807d2c57db5d5b740ec165049b1218035129 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 04:33:33 -0700 Subject: [PATCH 10/17] fix(cmd): remove duplicate doc comment and double blank line --- cmd/review-bot/main.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index e1cd719..85927cf 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -530,7 +530,6 @@ func verdictToEvent(verdict string) vcs.ReviewEvent { } } -// 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). // For Gitea: edits the review body with a link to the new review and resolves inline comments. @@ -557,7 +556,6 @@ func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsUR 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 From e70b54f2380457710c5e18fbb168d8b944bd8298 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 07:13:12 -0700 Subject: [PATCH 11/17] =?UTF-8?q?fix:=20address=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20gofmt=20NITs=20and=20remove=20unreachable=20default?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - github/client.go: add missing blank line between doRequestWithBody and doJSONRequest - cmd/review-bot/main.go: remove double blank line before findAllOwnReviews - cmd/review-bot/main.go: remove unreachable default case in VCS client init switch (provider is already validated at startup) - cmd/review-bot/main_test.go: remove double blank line before TestHasSharedToken - cmd/review-bot/main_test.go: fix comment alignment (gofmt) - review/persona_test.go: fix comment alignment in table literal (gofmt) --- cmd/review-bot/main.go | 4 ---- cmd/review-bot/main_test.go | 5 ++--- github/client.go | 1 + review/persona_test.go | 2 +- 4 files changed, 4 insertions(+), 8 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 85927cf..e967981 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -179,9 +179,6 @@ func main() { ghBaseURL = "https://api.github.com" } client = github.NewClient(*reviewerToken, ghBaseURL) - default: - fmt.Fprintf(os.Stderr, "Error: unhandled provider %q\n", *provider) - os.Exit(1) } slog.Info("VCS client initialized", "provider", *provider) @@ -899,7 +896,6 @@ func extractSentinelName(body string) string { return name } - // findAllOwnReviews returns all non-superseded reviews matching the sentinel. func findAllOwnReviews(reviews []vcs.Review, sentinel string) []vcs.Review { var result []vcs.Review diff --git a/cmd/review-bot/main_test.go b/cmd/review-bot/main_test.go index b97c58e..f492b16 100644 --- a/cmd/review-bot/main_test.go +++ b/cmd/review-bot/main_test.go @@ -210,7 +210,6 @@ func TestBuildSupersededBodyShortSHA(t *testing.T) { } } - func TestHasSharedToken(t *testing.T) { tests := []struct { name string @@ -646,8 +645,8 @@ func TestExtractSentinelName_EdgeCases(t *testing.T) { {" rest", "sonnet"}, {" rest", "gpt-review"}, {"no sentinel here", "unknown"}, - {" end", "abc"}, // embedded in text + {" end", "abc"}, // embedded in text } for _, tc := range tests { diff --git a/github/client.go b/github/client.go index 8d41ee7..5ee9d85 100644 --- a/github/client.go +++ b/github/client.go @@ -386,6 +386,7 @@ func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, r return c.doRequestCore(ctx, method, reqURL, opts) } + // doJSONRequest performs an HTTP request with a JSON body and returns the response body. // It delegates retry/backoff/429 handling to doRequestWithBody. // This is a general-purpose helper used by any method that needs to send JSON payloads diff --git a/review/persona_test.go b/review/persona_test.go index b90eae7..19a6814 100644 --- a/review/persona_test.go +++ b/review/persona_test.go @@ -355,7 +355,7 @@ func TestCapitalizeFirst(t *testing.T) { {"HELLO", "HELLO"}, {"a", "A"}, {"", ""}, - {"日本語", "日本語"}, // Non-ASCII: Japanese doesn't have case + {"日本語", "日本語"}, // Non-ASCII: Japanese doesn't have case {"über", "Über"}, // German umlaut {"élève", "Élève"}, // French accent } From 271ea7f5fe23fb008df908f13a94ba59f3670180 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 07:25:26 -0700 Subject: [PATCH 12/17] style: remove stray blank line in doRequestWithBody --- github/client.go | 1 - 1 file changed, 1 deletion(-) diff --git a/github/client.go b/github/client.go index 5ee9d85..cbaf828 100644 --- a/github/client.go +++ b/github/client.go @@ -384,7 +384,6 @@ func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, r opts.extraHeaders = map[string]string{"Content-Type": "application/json"} } return c.doRequestCore(ctx, method, reqURL, opts) - } // doJSONRequest performs an HTTP request with a JSON body and returns the response body. From bdc109901d9d9f67edb2ad16e19e017d9705584b Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 07:45:19 -0700 Subject: [PATCH 13/17] fix(github): remove double blank line in client_test.go (gofmt) --- github/client_test.go | 1 - 1 file changed, 1 deletion(-) diff --git a/github/client_test.go b/github/client_test.go index 8bf02f0..644bbdf 100644 --- a/github/client_test.go +++ b/github/client_test.go @@ -568,7 +568,6 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) { } } - func TestSetRetryBackoff_RejectsInvalidLength(t *testing.T) { c := NewClient("token", "https://api.github.com") From 34f73938925d37f89d96613f3e1c93010e8c9a73 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 08:01:15 -0700 Subject: [PATCH 14/17] fix: address review feedback on PR #106 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove unused envOrDefaultBool function and its test (Sonnet #3266 NIT) - Replace Unicode em dashes with ASCII in slog messages (GPT #3267 NIT) - Add scheme validation for vcsURL before embedding in Markdown link (Security #3269 MINOR — defense-in-depth against unsafe schemes) - Extract ReviewerSelfRequester interface to remove concrete gitea.Adapter dependency from main's self-reviewer path (Sonnet #3266 NIT) - Add compile-time conformance assertion and test for Adapter.RequestReviewerSelf --- cmd/review-bot/main.go | 20 ++++++++---------- cmd/review-bot/main_test.go | 41 ------------------------------------- gitea/adapter.go | 7 +++++++ gitea/adapter_test.go | 22 ++++++++++++++++++++ vcs/interfaces.go | 8 ++++++++ 5 files changed, 45 insertions(+), 53 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index e967981..788408e 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -429,7 +429,7 @@ func main() { currentSHA = currentPR.Head.SHA } if shouldSkipStaleReview(evaluatedSHA, currentSHA) { - slog.Warn("HEAD moved during review — skipping stale review", + slog.Warn("HEAD moved during review -- skipping stale review", "evaluated", evaluatedSHA, "current", currentSHA, "pr", prNumber) @@ -477,12 +477,12 @@ func main() { } // Self-request as reviewer (Gitea-specific; ensures we appear in required-reviewer checks) - if giteaAdapter, ok := client.(*gitea.Adapter); ok { + if selfReq, ok := client.(vcs.ReviewerSelfRequester); ok { authUser, err := client.GetAuthenticatedUser(ctx) if err != nil { slog.Warn("could not determine authenticated user for reviewer self-request", "error", err) } else if authUser != "" { - if err := giteaAdapter.Underlying().RequestReviewer(ctx, owner, repoName, prNumber, authUser); err != nil { + if err := selfReq.RequestReviewerSelf(ctx, owner, repoName, prNumber, authUser); err != nil { slog.Warn("could not self-request as reviewer", "user", authUser, "error", err) } else { slog.Debug("self-requested as reviewer", "user", authUser, "pr", prNumber) @@ -563,6 +563,10 @@ func supersedeOldReviews(ctx context.Context, client vcs.Client, provider, vcsUR } 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) @@ -757,14 +761,6 @@ func envOrDefaultInt(key string, defaultVal int) int { return defaultVal } -func envOrDefaultBool(key string, defaultVal bool) bool { - v := strings.TrimSpace(strings.ToLower(os.Getenv(key))) - if v == "" { - return defaultVal - } - return v == "true" || v == "1" || v == "yes" -} - // validateReviewerName checks that the name contains only safe characters // for embedding in an HTML comment sentinel ([a-zA-Z0-9_-]). func validateReviewerName(name string) error { @@ -858,7 +854,7 @@ func hasSharedToken(reviews []vcs.Review, ownSentinel string) bool { } for _, r := range reviews { if r.User.Login == ownLogin && strings.Contains(r.Body, "" - 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) +} From 5252143a33c634301efea27789016366d3548853 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 09:20:33 -0700 Subject: [PATCH 16/17] =?UTF-8?q?fix:=20address=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20alias=20default,=20acronym=20convention,=20observab?= =?UTF-8?q?ility?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - #19639: Use empty default for --gitea-url alias to remove ordering dependency - #19640: Upgrade slog.Warn to slog.Error for missing ReviewSuperseder (signals bug) - #19641: Remove orphaned comment fragment from buildSupersededBody relocation - #19642: Rename ProviderGithub → ProviderGitHub per Go acronym convention - #19643: Log resolution failures at debug level in SupersedeReviews --- cmd/review-bot/main.go | 10 ++++------ gitea/adapter.go | 4 +++- vcs/provider.go | 4 ++-- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index f08deb8..bc1f192 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -85,8 +85,8 @@ func main() { aicoreResourceGroup := flag.String("aicore-resource-group", envOrDefault("AICORE_RESOURCE_GROUP", "default"), "SAP AI Core resource group (for provider=aicore)") // 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") + // Shares vcsURL pointer; empty default avoids ordering dependency with vcsURL declaration. + flag.StringVar(vcsURL, "gitea-url", "", "Deprecated: use --vcs-url instead") flag.Parse() @@ -162,7 +162,7 @@ func main() { case vcs.ProviderGitea: giteaClient := gitea.NewClient(*vcsURL, *reviewerToken) client = gitea.NewAdapter(giteaClient) - case vcs.ProviderGithub: + case vcs.ProviderGitHub: client = github.NewClient(*reviewerToken, *baseURL) default: panic("unreachable: provider validation should have caught " + vcsProvider.String()) @@ -501,7 +501,7 @@ func main() { os.Exit(1) } } else { - slog.Warn("provider does not support review superseding", "provider", vcsProvider) + slog.Error("provider does not support review superseding", "provider", vcsProvider) } } } @@ -721,8 +721,6 @@ func validateWorkspacePath(path, pathName string) (string, error) { return resolvedPath, nil } -// with collapsed original content and the commit it was evaluated against. - // hasSharedToken detects if another review-bot role posted under the same // VCS user. This indicates misconfiguration where two roles share a token // instead of having separate accounts. Returns true if shared token diff --git a/gitea/adapter.go b/gitea/adapter.go index 9c4f753..176c722 100644 --- a/gitea/adapter.go +++ b/gitea/adapter.go @@ -282,7 +282,9 @@ func (a *Adapter) SupersedeReviews(ctx context.Context, owner, repo string, prNu if c.ID == 0 { continue } - _ = underlying.ResolveComment(ctx, owner, repo, c.ID) + if err := underlying.ResolveComment(ctx, owner, repo, c.ID); err != nil { + slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err) + } } } return nil diff --git a/vcs/provider.go b/vcs/provider.go index 2d19409..0d9819b 100644 --- a/vcs/provider.go +++ b/vcs/provider.go @@ -7,13 +7,13 @@ type VCSProvider string const ( ProviderGitea VCSProvider = "gitea" - ProviderGithub VCSProvider = "github" + ProviderGitHub VCSProvider = "github" ) // Valid reports whether p is a known VCS provider. func (p VCSProvider) Valid() bool { switch p { - case ProviderGitea, ProviderGithub: + case ProviderGitea, ProviderGitHub: return true default: return false From 91fba770d9e07d7fe5b76ba634b59d073645d8e4 Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 09:33:06 -0700 Subject: [PATCH 17/17] fix(ci): restore *vcsURL default in --gitea-url alias registration flag.StringVar sets *p = value at registration time. Using "" as the default overwrites the env-resolved value that --vcs-url already stored in *vcsURL. Restore *vcsURL as the default to preserve the GITEA_URL / VCS_URL / GITHUB_SERVER_URL resolution chain. Fixes CI error: --vcs-url (or --gitea-url) is required for provider=gitea --- cmd/review-bot/main.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index bc1f192..d84afb3 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -85,8 +85,9 @@ func main() { aicoreResourceGroup := flag.String("aicore-resource-group", envOrDefault("AICORE_RESOURCE_GROUP", "default"), "SAP AI Core resource group (for provider=aicore)") // Backward-compatible alias: --gitea-url shares vcsURL's pointer (last flag wins). - // Shares vcsURL pointer; empty default avoids ordering dependency with vcsURL declaration. - flag.StringVar(vcsURL, "gitea-url", "", "Deprecated: use --vcs-url instead") + // Must use *vcsURL as default: StringVar sets *p=value at registration, so empty + // string would overwrite the env-resolved value from the --vcs-url declaration. + flag.StringVar(vcsURL, "gitea-url", *vcsURL, "Deprecated: use --vcs-url instead") flag.Parse()