From cd4521feb1266b6f9c53d637361054badf12492b Mon Sep 17 00:00:00 2001 From: claw Date: Wed, 13 May 2026 01:21:32 -0700 Subject: [PATCH] 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 | 321 +++++++++++++++++++++--------------- cmd/review-bot/main_test.go | 140 ++++++++++------ github/conformance_test.go | 7 +- github/reviews.go | 236 ++++++++++++++++++++++++++ review/formatter.go | 11 -- review/formatter_test.go | 18 -- 6 files changed, 516 insertions(+), 217 deletions(-) create mode 100644 github/reviews.go diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index ef6e4e9..9dbcda4 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 @@ -525,7 +606,7 @@ func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, re // patternsFiles is comma-separated list of file paths or directories. // If a path ends with / or is a directory, all files within it are fetched recursively. // If patternsFiles is empty, all files from the repo root are fetched. -func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patternsFiles string) string { +func fetchPatterns(ctx context.Context, client vcs.FileReader, patternsRepo, patternsFiles string) string { var sb strings.Builder repos := strings.Split(patternsRepo, ",") @@ -563,7 +644,7 @@ func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patt var repoSkippedFiles []string for _, path := range paths { - 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 @@ -602,7 +683,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" } @@ -737,10 +818,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) { @@ -753,7 +834,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, @@ -553,7 +550,7 @@ func TestBuildPatternPaths(t *testing.T) { func TestEvaluateCIStatus(t *testing.T) { tests := []struct { name string - statuses []gitea.CommitStatus + statuses []vcs.CommitStatus wantPassed bool wantSubstr string }{ @@ -565,7 +562,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"}, }, @@ -574,7 +571,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"}, }, @@ -583,7 +580,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, @@ -591,7 +588,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"}, }, @@ -600,7 +597,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"}, }, @@ -609,7 +606,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"}, @@ -838,7 +835,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", @@ -866,7 +863,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", @@ -893,7 +890,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", @@ -920,7 +917,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", @@ -948,7 +945,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", @@ -972,7 +969,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 @@ -980,6 +1005,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_"), @@ -997,12 +1023,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, "") @@ -1066,3 +1092,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/conformance_test.go b/github/conformance_test.go index ca13188..e47ec26 100644 --- a/github/conformance_test.go +++ b/github/conformance_test.go @@ -6,8 +6,5 @@ import ( ) // Compile-time interface conformance assertions. -// These verify github.Client satisfies vcs.PRReader and vcs.FileReader. -var ( - _ vcs.PRReader = (*github.Client)(nil) - _ vcs.FileReader = (*github.Client)(nil) -) +// These verify github.Client satisfies vcs.Client (the full interface). +var _ vcs.Client = (*github.Client)(nil) 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{