diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 40efc46..d84afb3 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 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") - 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,11 @@ 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)") + // Backward-compatible alias: --gitea-url shares vcsURL's pointer (last flag wins). + // 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() if *versionFlag { @@ -92,12 +101,23 @@ func main() { slog.Info("review-bot starting", "version", version) + // Validate VCS provider + vcsProvider := vcs.VCSProvider(*provider) + if !vcsProvider.Valid() { + 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 vcsProvider == vcs.ProviderGitea && *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,20 @@ func main() { os.Exit(1) } - // Initialize clients - giteaClient := gitea.NewClient(*giteaURL, *reviewerToken) + // Initialize VCS client + var client vcs.Client + switch vcsProvider { + case vcs.ProviderGitea: + giteaClient := gitea.NewClient(*vcsURL, *reviewerToken) + client = gitea.NewAdapter(giteaClient) + case vcs.ProviderGitHub: + client = github.NewClient(*reviewerToken, *baseURL) + default: + panic("unreachable: provider validation should have caught " + vcsProvider.String()) + } + slog.Info("VCS client initialized", "provider", vcsProvider) + + // 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 +204,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 +241,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 +249,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 +258,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 +282,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 +294,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 +387,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,34 +408,40 @@ 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", + slog.Warn("HEAD moved during review -- skipping stale review", "evaluated", evaluatedSHA, "current", currentSHA, "pr", prNumber) 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 +450,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 +464,63 @@ 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 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 := 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) + } } + } 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 via optional interface 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) - - // 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) + if superseder, ok := client.(vcs.ReviewSuperseder); ok { + if err := superseder.SupersedeReviews(ctx, owner, repoName, prNumber, oldReviews, posted.ID, *vcsURL, sentinel); err != nil { + slog.Error("failed to supersede old reviews", "error", err) + os.Exit(1) } + } else { + slog.Error("provider does not support review superseding", "provider", vcsProvider) } } +} +// 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 + } } // 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 +529,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 +546,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 { +// Empty entries in patternsFiles are skipped (no implicit repo-root fetch). +func fetchPatterns(ctx context.Context, client vcs.FileReader, patternsRepo, patternsFiles string) string { var sb strings.Builder repos := strings.Split(patternsRepo, ",") @@ -554,7 +577,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,18 +616,20 @@ func isPatternFile(path string) bool { } // evaluateCIStatus checks if all CI statuses indicate success. -func evaluateCIStatus(statuses []gitea.CommitStatus) (passed bool, details string) { +// 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)) } @@ -613,6 +638,9 @@ func evaluateCIStatus(statuses []gitea.CommitStatus) (passed bool, details strin 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" } @@ -643,14 +671,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 { @@ -702,36 +722,11 @@ func validateWorkspacePath(path, pathName string) (string, error) { return resolvedPath, nil } -// buildSupersededBody creates the body for a superseded review: struck-through banner -// with collapsed original content and the commit it was evaluated against. -func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string) string { - shortSHA := commitSHA - if len(shortSHA) > 8 { - shortSHA = shortSHA[:8] - } - var sb strings.Builder - sb.WriteString("~~Original review~~\n\n") - sb.WriteString("**Superseded** \u2014 [see current review](") - sb.WriteString(newReviewURL) - sb.WriteString(") for up-to-date findings.\n\n") - if shortSHA != "" { - sb.WriteString("
Previous findings (commit ") - sb.WriteString(shortSHA) - sb.WriteString(")\n\n") - } else { - sb.WriteString("
Previous findings\n\n") - } - sb.WriteString(originalBody) - sb.WriteString("\n\n
\n\n") - sb.WriteString(sentinel) - return sb.String() -} - // hasSharedToken detects if another review-bot role posted under the same -// 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 +739,7 @@ func hasSharedToken(reviews []gitea.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 TestFindOwnReview(t *testing.T) { - tests := []struct { - name string - reviews []gitea.Review - sentinel string - wantID int64 - wantNil bool - }{ - { - name: "no reviews", - reviews: nil, - sentinel: "", - wantNil: true, - }, - { - name: "found by sentinel", - reviews: []gitea.Review{ - makeReview(42, "bot", "APPROVED", false, "review body\n"), - }, - sentinel: "", - wantID: 42, - }, - { - name: "wrong sentinel", - reviews: []gitea.Review{ - makeReview(42, "bot", "APPROVED", false, "body\n"), - }, - sentinel: "", - wantNil: true, - }, - { - name: "multiple reviews, returns first match", - reviews: []gitea.Review{ - makeReview(10, "bot", "APPROVED", false, "old\n"), - makeReview(20, "bot", "APPROVED", false, "new\n"), - }, - sentinel: "", - wantID: 20, - }, - { - name: "skips superseded review", - reviews: []gitea.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: []gitea.Review{ - makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n"), - }, - sentinel: "", - wantNil: true, - }, - { - name: "picks highest ID among matches", - reviews: []gitea.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 { name string - reviews []gitea.Review + reviews []vcs.Review sentinel string want bool }{ @@ -314,36 +177,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 +370,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 +382,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 +391,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 +400,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,16 +408,16 @@ 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"}, }, wantPassed: true, - wantSubstr: "all checks passed", + wantSubstr: "no failures", }, { 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 +426,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"}, @@ -685,47 +548,6 @@ func TestEnvOrDefaultInt(t *testing.T) { } } -func TestEnvOrDefaultBool(t *testing.T) { - tests := []struct { - name string - envVal string - setEnv bool - defaultVal bool - want bool - }{ - {"unset returns default true", "", false, true, true}, - {"unset returns default false", "", false, false, false}, - {"true", "true", true, false, true}, - {"TRUE", "TRUE", true, false, true}, - {"True", "True", true, false, true}, - {"1", "1", true, false, true}, - {"yes", "yes", true, false, true}, - {"YES", "YES", true, false, true}, - {"false", "false", true, true, false}, - {"0", "0", true, true, false}, - {"no", "no", true, true, false}, - {"random string", "random", true, true, false}, - {"empty string returns default", "", true, true, true}, - {"whitespace true", " true ", true, false, true}, - } - - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - envKey := "TEST_ENV_BOOL_" + strings.ReplaceAll(tc.name, " ", "_") - if tc.setEnv { - os.Setenv(envKey, tc.envVal) - defer os.Unsetenv(envKey) - } else { - os.Unsetenv(envKey) - } - got := envOrDefaultBool(envKey, tc.defaultVal) - if got != tc.want { - t.Errorf("envOrDefaultBool(%q, %v) = %v, want %v", tc.envVal, tc.defaultVal, got, tc.want) - } - }) - } -} - func TestExtractSentinelName_EdgeCases(t *testing.T) { tests := []struct { body string @@ -734,8 +556,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 { @@ -792,7 +614,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 +642,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 +669,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 +696,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 +724,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 +748,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 +784,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 +802,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 +871,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/gitea/adapter.go b/gitea/adapter.go index 4cef61e..176c722 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" ) @@ -16,6 +18,7 @@ type Adapter struct { // Compile-time interface conformance assertion. var _ vcs.Client = (*Adapter)(nil) +var _ vcs.ReviewerSelfRequester = (*Adapter)(nil) // NewAdapter creates a new Adapter wrapping the given gitea Client. func NewAdapter(client *Client) *Adapter { @@ -230,3 +233,84 @@ func (a *Adapter) DismissReview(ctx context.Context, owner, repo string, number func (a *Adapter) GetAuthenticatedUser(ctx context.Context) (string, error) { return a.client.GetAuthenticatedUser(ctx) } + +// RequestReviewerSelf adds the given user as a requested reviewer on a pull request. +// This implements vcs.ReviewerSelfRequester for the Gitea adapter. +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 + } + 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 +} + +// 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/adapter_test.go b/gitea/adapter_test.go index 0071699..a4aea9d 100644 --- a/gitea/adapter_test.go +++ b/gitea/adapter_test.go @@ -386,3 +386,25 @@ func TestAdapter_GetFileContent_RefRouting(t *testing.T) { t.Errorf("GetFileContent(ref=\"abc123\") = %q, want %q", got, "content-at-ref") } } + +func TestAdapter_RequestReviewerSelf(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + t.Errorf("expected POST, got %s", r.Method) + } + expected := "/api/v1/repos/owner/repo/pulls/5/requested_reviewers" + if r.URL.Path != expected { + t.Errorf("path = %q, want %q", r.URL.Path, expected) + } + w.WriteHeader(http.StatusCreated) + })) + defer server.Close() + + client := gitea.NewClient(server.URL, "token") + adapter := gitea.NewAdapter(client) + + err := adapter.RequestReviewerSelf(context.Background(), "owner", "repo", 5, "bot-user") + if err != nil { + t.Fatalf("RequestReviewerSelf() error = %v", err) + } +} 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/client.go b/github/client.go index f958bfa..cbaf828 100644 --- a/github/client.go +++ b/github/client.go @@ -6,6 +6,7 @@ package github import ( "bytes" "context" + "encoding/json" "errors" "fmt" "io" @@ -214,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. @@ -227,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) @@ -384,3 +385,15 @@ 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 +// (e.g. PostReview, DismissReview). +func (c *Client) doJSONRequest(ctx context.Context, method, reqURL string, payload any) ([]byte, error) { + jsonBody, err := json.Marshal(payload) + if err != nil { + return nil, fmt.Errorf("marshal request body: %w", err) + } + return c.doRequestWithBody(ctx, method, reqURL, jsonBody) +} diff --git a/github/client_test.go b/github/client_test.go index ab6f4d1..644bbdf 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" @@ -567,7 +568,6 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) { } } - func TestSetRetryBackoff_RejectsInvalidLength(t *testing.T) { c := NewClient("token", "https://api.github.com") @@ -592,3 +592,59 @@ 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()) + 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 { + 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()) + 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 { + 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) + } +} 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/review/formatter.go b/review/formatter.go index d2e109f..de9b558 100644 --- a/review/formatter.go +++ b/review/formatter.go @@ -10,18 +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. // Persona display names are controlled by repo owners (trusted input). diff --git a/review/formatter_test.go b/review/formatter_test.go index 7684e88..22da660 100644 --- a/review/formatter_test.go +++ b/review/formatter_test.go @@ -98,25 +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{ Verdict: "APPROVE", 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 } diff --git a/vcs/interfaces.go b/vcs/interfaces.go index 3bc299e..4b1adff 100644 --- a/vcs/interfaces.go +++ b/vcs/interfaces.go @@ -41,3 +41,20 @@ type Client interface { Reviewer Identity } + +// ReviewerSelfRequester is an optional interface implemented by adapters that support +// requesting the authenticated user as a reviewer on a pull request. This is used for +// Gitea-specific behavior (ensuring the bot appears in required-reviewer checks). +// Consumers should use interface assertion: if sr, ok := client.(ReviewerSelfRequester); ok { ... } +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..0d9819b --- /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) +}