rodin/review-bot
2
1
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 6 Wiki Activity

feat: implement GitHub API methods and VCS routing (issue #130) #131

Merged
rodin merged 3 commits from issue-130 into main 2026-05-14 22:06:21 +00:00
Conversation 8 Commits 3 Files Changed 5
+473 -90
3 changed files with 473 additions and 90 deletions
Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 10ef451c20 - Show all commits
cmd/review-bot/main.go
+82 -58
View File
@@ -14,6 +14,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"
)
@@ -169,7 +170,39 @@ func main() {
}
// Initialize clients
giteaClient := gitea.NewClient(*vcsURL, *reviewerToken)
// Detect VCS type: explicit flag > env var > URL heuristic (default: gitea).
vcsType := envOrDefault("VCS_TYPE", "")
if vcsType == "" {
sonnet-review-bot commented 2026-05-14 21:12:18 +00:00
Review
Copy Link
Copy Source

[MINOR] The URL heuristic strings.Contains(*vcsURL, "github.concur.com") is a hardcoded company-specific domain baked into an open-source tool. This is fragile — any GitHub Enterprise host other than github.concur.com won't be detected. The more robust approach would be to rely on the explicit VCS_TYPE env var for all GHES instances, or detect GHES by checking for /api/v3 in the URL, or check the GitHub API endpoint itself. The comment acknowledges the composite action sets VCS_TYPE explicitly, but this hardcoded fallback will silently mis-route for other GHES deployments.

**[MINOR]** The URL heuristic `strings.Contains(*vcsURL, "github.concur.com")` is a hardcoded company-specific domain baked into an open-source tool. This is fragile — any GitHub Enterprise host other than `github.concur.com` won't be detected. The more robust approach would be to rely on the explicit `VCS_TYPE` env var for all GHES instances, or detect GHES by checking for `/api/v3` in the URL, or check the GitHub API endpoint itself. The comment acknowledges the composite action sets `VCS_TYPE` explicitly, but this hardcoded fallback will silently mis-route for other GHES deployments.
// Heuristic: if the URL looks like github.com or a GitHub Enterprise host,
// default to GitHub. The composite action sets VCS_TYPE explicitly, so this
// is a fallback for manual invocations.
sonnet-review-bot commented 2026-05-14 20:45:20 +00:00
Review
Copy Link
Copy Source

[MINOR] The URL heuristic for VCS type detection hardcodes 'github.concur.com' as a special case alongside 'github.com'. This company-specific hostname leaks internal infrastructure details into open-source code and is brittle — any other GHES instance won't be detected. Since VCS_TYPE env var is the recommended path and the comment acknowledges this is a fallback for manual invocations, consider dropping the hardcoded GHES heuristic and just matching 'github.com', or document the pattern more generically (e.g., check for '/api/v3' in the URL).

**[MINOR]** The URL heuristic for VCS type detection hardcodes 'github.concur.com' as a special case alongside 'github.com'. This company-specific hostname leaks internal infrastructure details into open-source code and is brittle — any other GHES instance won't be detected. Since VCS_TYPE env var is the recommended path and the comment acknowledges this is a fallback for manual invocations, consider dropping the hardcoded GHES heuristic and just matching 'github.com', or document the pattern more generically (e.g., check for '/api/v3' in the URL).
gpt-review-bot commented 2026-05-14 20:46:17 +00:00
Review
Copy Link
Copy Source

[MINOR] VCS type heuristic hard-codes an organization-specific domain ("github.concur.com"). This is brittle and may misclassify GitHub Enterprise instances without that substring; consider relying on VCS_TYPE or a more general host-based detection.

**[MINOR]** VCS type heuristic hard-codes an organization-specific domain ("github.concur.com"). This is brittle and may misclassify GitHub Enterprise instances without that substring; consider relying on VCS_TYPE or a more general host-based detection.
if strings.Contains(*vcsURL, "github.com") || strings.Contains(*vcsURL, "github.concur.com") {
vcsType = "github"
} else {
vcsType = "gitea"
}
}
slog.Info("VCS type detected", "vcs_type", vcsType, "vcs_url", *vcsURL)
var vcs vcsClient
switch vcsType {
case "github":
// GitHub: baseURL is the API URL, derived from server URL.
// github.com → https://api.github.com
// GHES (e.g. https://ghe.example.com) → https://ghe.example.com/api/v3
apiURL := githubAPIURL(*vcsURL)
ghClient := github.NewClient(*reviewerToken, apiURL)
vcs = newGithubVCSAdapter(ghClient)
slog.Info("using GitHub VCS client", "api_url", apiURL)
case "gitea":
giteaClient := gitea.NewClient(*vcsURL, *reviewerToken)
vcs = newGiteaVCSAdapter(giteaClient)
slog.Info("using Gitea VCS client", "url", *vcsURL)
default:
slog.Error("unsupported VCS type", "vcs_type", vcsType, "valid", "gitea, github")
os.Exit(1)
}
llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel)
if *llmTemp < 0 || *llmTemp > 2 {
slog.Error("invalid LLM temperature", "temperature", *llmTemp, "range", "0-2")
@@ -207,7 +240,7 @@ func main() {
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, vcs, owner, repoName)
if err != nil {
slog.Warn("could not load repo personas", "repo", owner+"/"+repoName, "error", err)
// Continue with built-in personas only.
@@ -243,7 +276,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 := vcs.GetPullRequest(ctx, owner, repoName, prNumber)
if err != nil {
slog.Error("failed to fetch PR", "pr", prNumber, "error", err)
os.Exit(1)
@@ -251,7 +284,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 := vcs.GetPullRequestDiff(ctx, owner, repoName, prNumber)
if err != nil {
slog.Error("failed to fetch diff", "pr", prNumber, "error", err)
os.Exit(1)
@@ -260,11 +293,11 @@ func main() {
// Step 3: Fetch full file content for modified files
fileContext := ""
files, err := giteaClient.GetPullRequestFiles(ctx, owner, repoName, prNumber)
files, err := vcs.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, vcs, owner, repoName, pr.Head.Ref, files)
slog.Debug("fetched file context", "files", len(files))
}
@@ -272,7 +305,7 @@ func main() {
ciPassed := true
ciDetails := ""
if pr.Head.Sha != "" {
statuses, err := giteaClient.GetCommitStatuses(ctx, owner, repoName, pr.Head.Sha)
statuses, err := vcs.GetCommitStatuses(ctx, owner, repoName, pr.Head.Sha)
if err != nil {
slog.Warn("could not fetch CI status", "sha", pr.Head.Sha, "error", err)
} else {
@@ -284,7 +317,7 @@ func main() {
// Step 5: Load conventions file if specified
conventions := ""
if *conventionsFile != "" {
content, err := giteaClient.GetFileContent(ctx, owner, repoName, *conventionsFile)
content, err := vcs.GetFileContent(ctx, owner, repoName, *conventionsFile)
if err != nil {
slog.Warn("could not load conventions file", "file", *conventionsFile, "error", err)
} else {
@@ -296,7 +329,7 @@ func main() {
// Step 6: Load patterns from external repo if specified
patterns := ""
if *patternsRepo != "" {
patterns = fetchPatterns(ctx, giteaClient, *patternsRepo, *patternsFiles)
patterns = fetchPatterns(ctx, vcs, *patternsRepo, *patternsFiles)
slog.Debug("loaded patterns", "repo", *patternsRepo, "bytes", len(patterns))
}
@@ -411,7 +444,7 @@ func main() {
// Stale check: verify HEAD hasn't moved since we started
evaluatedSHA := pr.Head.Sha
var currentSHA string
currentPR, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber)
currentPR, err := vcs.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
@@ -428,10 +461,10 @@ func main() {
// Map findings to inline comments for lines present in the diff
diffRanges := gitea.ParseDiffNewLines(diff)
var inlineComments []gitea.ReviewComment
var inlineComments []vcsReviewComment
for _, f := range result.Findings {
if f.File != "" && f.Line > 0 && diffRanges.Contains(f.File, f.Line) {
inlineComments = append(inlineComments, gitea.ReviewComment{
inlineComments = append(inlineComments, vcsReviewComment{
Path: f.File,
NewPosition: int64(f.Line),
Body: fmt.Sprintf("**[%s]** %s", f.Severity, f.Finding),
@@ -446,9 +479,9 @@ func main() {
// 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 []vcsReview
if *reviewerName != "" {
existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
existingReviews, err := vcs.ListReviews(ctx, owner, repoName, prNumber)
if err != nil {
slog.Warn("could not list existing reviews", "pr", prNumber, "error", err)
} else {
@@ -461,11 +494,11 @@ func main() {
}
// Self-request as reviewer (ensures we appear in required-reviewer checks)
authUser, err := giteaClient.GetAuthenticatedUser(ctx)
authUser, err := vcs.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 {
if err := vcs.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)
@@ -474,31 +507,34 @@ func main() {
// POST new review
slog.Info("posting review", "event", event, "pr", prNumber)
posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, evaluatedSHA, inlineComments)
posted, err := vcs.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, evaluatedSHA, inlineComments)
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
if len(oldReviews) > 0 {
// Supersede all old reviews with link to the new one.
// This is only supported on Gitea (requires timeline API); GitHub reviews cannot
// be edited after submission, so we skip the supersede step there.
extVCS, isGiteaExt := vcs.(giteaExtClient)
if len(oldReviews) > 0 && isGiteaExt {
newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*vcsURL, "/"), owner, repoName, prNumber, posted.ID)
for _, oldReview := range oldReviews {
cid, err := giteaClient.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID)
cid, err := extVCS.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, int64(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 {
if err := extVCS.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)
oldComments, err := extVCS.ListReviewComments(ctx, owner, repoName, int64(prNumber), oldReview.ID)
if err != nil {
slog.Warn("could not list old review comments for resolution", "review_id", oldReview.ID, "error", err)
continue
@@ -508,7 +544,7 @@ func main() {
if c.ID == 0 {
continue
}
if err := giteaClient.ResolveComment(ctx, owner, repoName, c.ID); err != nil {
if err := extVCS.ResolveComment(ctx, owner, repoName, c.ID); err != nil {
slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err)
failed++
} else {
@@ -522,12 +558,14 @@ func main() {
slog.Warn("some inline comments could not be resolved", "review_id", oldReview.ID, "failed", failed, "pr", prNumber)
}
}
} else if len(oldReviews) > 0 {
slog.Info("skipping supersede of old reviews (not supported on this VCS)", "old_count", len(oldReviews), "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 vcsClient, owner, repo, ref string, files []vcsChangedFile) string {
var sb strings.Builder
for _, f := range files {
if ctx.Err() != nil {
@@ -554,7 +592,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 vcsClient, patternsRepo, patternsFiles string) string {
var sb strings.Builder
repos := strings.Split(patternsRepo, ",")
@@ -631,7 +669,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 []vcsCommitStatus) (passed bool, details string) {
if len(statuses) == 0 {
return true, "no CI statuses found"
}
@@ -654,6 +692,19 @@ func evaluateCIStatus(statuses []gitea.CommitStatus) (passed bool, details strin
return true, "all checks passed"
security-review-bot commented 2026-05-14 21:13:15 +00:00
Review
Copy Link
Copy Source

[MINOR] githubAPIURL derives the API host directly from the provided server URL. With the heuristic that detects GitHub when the URL contains "github.com", a misconfigured or malicious host like "https://github.com.evil.com" would be treated as GHES and receive the Authorization bearer token. While this is an operator configuration input (not user-controlled), consider hardening by requiring explicit VCS_TYPE and/or validating the host against an allowlist to reduce the chance of credential leakage due to misconfiguration.

**[MINOR]** githubAPIURL derives the API host directly from the provided server URL. With the heuristic that detects GitHub when the URL contains "github.com", a misconfigured or malicious host like "https://github.com.evil.com" would be treated as GHES and receive the Authorization bearer token. While this is an operator configuration input (not user-controlled), consider hardening by requiring explicit VCS_TYPE and/or validating the host against an allowlist to reduce the chance of credential leakage due to misconfiguration.
}
// githubAPIURL converts a GitHub server URL to its API base URL.
// github.com → https://api.github.com
// GHES (e.g. https://ghe.example.com) → https://ghe.example.com/api/v3
func githubAPIURL(serverURL string) string {
const canonicalGitHub = "https://github.com"
const githubAPIBase = "https://api.github.com"
if serverURL == "" || strings.TrimRight(serverURL, "/") == canonicalGitHub {
return githubAPIBase
}
// GitHub Enterprise Server: /api/v3 suffix
return strings.TrimRight(serverURL, "/") + "/api/v3"
}
func envOrDefault(key, defaultVal string) string {
if v := os.Getenv(key); v != "" {
return v
@@ -769,7 +820,7 @@ func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string)
// Gitea user. This indicates misconfiguration where two roles share a token
// instead of having separate Gitea 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 []vcsReview, ownSentinel string) bool {
ownLogin := ""
for _, r := range reviews {
if strings.Contains(r.Body, ownSentinel) {
@@ -807,8 +858,8 @@ func extractSentinelName(body string) string {
}
// findOwnReview locates the most recent non-superseded review matching the sentinel.
func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review {
var best *gitea.Review
func findOwnReview(reviews []vcsReview, sentinel string) *vcsReview {
var best *vcsReview
for i := range reviews {
if !strings.Contains(reviews[i].Body, sentinel) {
continue
@@ -824,8 +875,8 @@ func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review {
}
// findAllOwnReviews returns all non-superseded reviews matching the sentinel.
func findAllOwnReviews(reviews []gitea.Review, sentinel string) []gitea.Review {
var result []gitea.Review
func findAllOwnReviews(reviews []vcsReview, sentinel string) []vcsReview {
var result []vcsReview
for i := range reviews {
if !strings.Contains(reviews[i].Body, sentinel) {
continue
@@ -851,31 +902,4 @@ func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool {
return evaluatedSHA != currentSHA
}
// giteaClientAdapter adapts gitea.Client to review.GiteaClient interface.
type giteaClientAdapter struct {
client *gitea.Client
}
func newGiteaClientAdapter(c *gitea.Client) *giteaClientAdapter {
return &giteaClientAdapter{client: c}
}
func (a *giteaClientAdapter) ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error) {
entries, err := a.client.ListContents(ctx, owner, repo, path)
if err != nil {
return nil, err
}
result := make([]review.ContentEntry, len(entries))
for i, e := range entries {
result[i] = review.ContentEntry{
Name: e.Name,
Path: e.Path,
Type: e.Type,
}
}
return result, nil
}
func (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
return a.client.GetFileContent(ctx, owner, repo, filepath)
}
4
cmd/review-bot/main_test.go
+30 -32
View File
@@ -10,7 +10,6 @@ import (
"strings"
"testing"
"gitea.weiker.me/rodin/review-bot/gitea"
)
func TestValidateReviewerName(t *testing.T) {
@@ -154,12 +153,11 @@ func TestValidateWorkspacePath(t *testing.T) {
}
}
func makeReview(id int64, login, state string, stale bool, body string) gitea.Review {
r := gitea.Review{
func makeReview(id int64, login, state string, _ bool, body string) vcsReview {
r := vcsReview{
ID: id,
Body: body,
State: state,
Stale: stale,
}
r.User.Login = login
return r
@@ -216,7 +214,7 @@ func TestBuildSupersededBodyShortSHA(t *testing.T) {
func TestFindOwnReview(t *testing.T) {
tests := []struct {
name string
reviews []gitea.Review
reviews []vcsReview
sentinel string
wantID int64
wantNil bool
@@ -229,7 +227,7 @@ func TestFindOwnReview(t *testing.T) {
},
{
name: "found by sentinel",
reviews: []gitea.Review{
reviews: []vcsReview{
makeReview(42, "bot", "APPROVED", false, "review body\n<!-- review-bot:sonnet -->"),
},
sentinel: "<!-- review-bot:sonnet -->",
@@ -237,7 +235,7 @@ func TestFindOwnReview(t *testing.T) {
},
{
name: "wrong sentinel",
reviews: []gitea.Review{
reviews: []vcsReview{
makeReview(42, "bot", "APPROVED", false, "body\n<!-- review-bot:gpt -->"),
},
sentinel: "<!-- review-bot:sonnet -->",
@@ -245,7 +243,7 @@ func TestFindOwnReview(t *testing.T) {
},
{
name: "multiple reviews, returns first match",
reviews: []gitea.Review{
reviews: []vcsReview{
makeReview(10, "bot", "APPROVED", false, "old\n<!-- review-bot:gpt -->"),
makeReview(20, "bot", "APPROVED", false, "new\n<!-- review-bot:sonnet -->"),
},
@@ -254,7 +252,7 @@ func TestFindOwnReview(t *testing.T) {
},
{
name: "skips superseded review",
reviews: []gitea.Review{
reviews: []vcsReview{
makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n**Superseded**\n<!-- review-bot:sonnet -->"),
makeReview(20, "bot", "APPROVED", false, "fresh review\n<!-- review-bot:sonnet -->"),
},
@@ -263,7 +261,7 @@ func TestFindOwnReview(t *testing.T) {
},
{
name: "only superseded reviews exist",
reviews: []gitea.Review{
reviews: []vcsReview{
makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n<!-- review-bot:sonnet -->"),
},
sentinel: "<!-- review-bot:sonnet -->",
@@ -271,7 +269,7 @@ func TestFindOwnReview(t *testing.T) {
},
{
name: "picks highest ID among matches",
reviews: []gitea.Review{
reviews: []vcsReview{
makeReview(50, "bot", "APPROVED", false, "v1\n<!-- review-bot:sonnet -->"),
makeReview(30, "bot", "APPROVED", false, "v0\n<!-- review-bot:sonnet -->"),
},
@@ -302,7 +300,7 @@ func TestFindOwnReview(t *testing.T) {
func TestHasSharedToken(t *testing.T) {
tests := []struct {
name string
reviews []gitea.Review
reviews []vcsReview
sentinel string
want bool
}{
@@ -314,36 +312,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: "<!-- review-bot:gpt --> body"},
reviews: []vcsReview{
{ID: 1, User: struct{ Login string }{Login: "other"}, Body: "<!-- review-bot:gpt --> body"},
},
sentinel: "<!-- review-bot:sonnet -->",
want: false,
},
{
name: "separate users - no shared token",
reviews: []gitea.Review{
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"},
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "security-review-bot"}, Body: "<!-- review-bot:security --> body"},
reviews: []vcsReview{
{ID: 1, User: struct{ Login string }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"},
{ID: 2, User: struct{ Login string }{Login: "security-review-bot"}, Body: "<!-- review-bot:security --> body"},
},
sentinel: "<!-- review-bot:sonnet -->",
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: "<!-- review-bot:sonnet --> body"},
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:security --> body"},
reviews: []vcsReview{
{ID: 1, User: struct{ Login string }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"},
{ID: 2, User: struct{ Login string }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:security --> body"},
},
sentinel: "<!-- review-bot:sonnet -->",
want: true,
},
{
name: "three roles same user",
reviews: []gitea.Review{
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:sonnet --> body"},
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:security --> body"},
{ID: 3, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:gpt --> body"},
reviews: []vcsReview{
{ID: 1, User: struct{ Login string }{Login: "bot"}, Body: "<!-- review-bot:sonnet --> body"},
{ID: 2, User: struct{ Login string }{Login: "bot"}, Body: "<!-- review-bot:security --> body"},
{ID: 3, User: struct{ Login string }{Login: "bot"}, Body: "<!-- review-bot:gpt --> body"},
},
sentinel: "<!-- review-bot:sonnet -->",
want: true,
@@ -553,7 +551,7 @@ func TestBuildPatternPaths(t *testing.T) {
func TestEvaluateCIStatus(t *testing.T) {
tests := []struct {
name string
statuses []gitea.CommitStatus
statuses []vcsCommitStatus
wantPassed bool
wantSubstr string
}{
@@ -565,7 +563,7 @@ func TestEvaluateCIStatus(t *testing.T) {
},
{
name: "all success",
statuses: []gitea.CommitStatus{
statuses: []vcsCommitStatus{
{Status: "success", Context: "ci/build", Description: "Build passed"},
{Status: "success", Context: "ci/test", Description: "Tests passed"},
},
@@ -574,7 +572,7 @@ func TestEvaluateCIStatus(t *testing.T) {
},
{
name: "one failure",
statuses: []gitea.CommitStatus{
statuses: []vcsCommitStatus{
{Status: "success", Context: "ci/build", Description: "Build passed"},
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
},
@@ -583,7 +581,7 @@ func TestEvaluateCIStatus(t *testing.T) {
},
{
name: "error status",
statuses: []gitea.CommitStatus{
statuses: []vcsCommitStatus{
{Status: "error", Context: "ci/lint", Description: "Lint error"},
},
wantPassed: false,
@@ -591,7 +589,7 @@ func TestEvaluateCIStatus(t *testing.T) {
},
{
name: "pending treated as not-failed",
statuses: []gitea.CommitStatus{
statuses: []vcsCommitStatus{
{Status: "pending", Context: "ci/build", Description: "In progress"},
{Status: "success", Context: "ci/test", Description: "Tests passed"},
},
@@ -600,7 +598,7 @@ func TestEvaluateCIStatus(t *testing.T) {
},
{
name: "multiple failures",
statuses: []gitea.CommitStatus{
statuses: []vcsCommitStatus{
{Status: "failure", Context: "ci/build", Description: "Build failed"},
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
},
@@ -609,7 +607,7 @@ func TestEvaluateCIStatus(t *testing.T) {
},
{
name: "mixed with pending and failure",
statuses: []gitea.CommitStatus{
statuses: []vcsCommitStatus{
{Status: "success", Context: "ci/build", Description: "Build passed"},
{Status: "pending", Context: "ci/deploy", Description: "Deploying"},
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
@@ -997,7 +995,7 @@ func cleanEnv() []string {
}
func TestFindAllOwnReviews(t *testing.T) {
reviews := []gitea.Review{
reviews := []vcsReview{
{ID: 1, Body: "<!-- review-bot:sonnet -->\nfirst review"},
{ID: 2, Body: "<!-- review-bot:gpt -->\nother bot"},
{ID: 3, Body: "<!-- review-bot:sonnet -->\nsecond review"},
cmd/review-bot/vcs.go
+361
View File
@@ -0,0 +1,361 @@
package main
// vcs.go defines the vcsClient interface that both gitea.Client (via giteaVCSAdapter)
// and github.Client (via githubVCSAdapter) satisfy, enabling VCS-type routing in main.go.
//
// Interface design:
// - Methods cover all PR review operations used by main.go.
// - Gitea-specific operations (supersede, comment resolution) are in the separate
// giteaExtClient interface. GitHub implementations return ErrNotSupported for those.
// - Types are defined here as package-local VCS types; each adapter converts from
// its respective client package's types.
import (
"context"
gpt-review-bot commented 2026-05-14 20:46:17 +00:00
Review
Copy Link
Copy Source

[NIT] ErrNotSupported is defined but not used. It's harmless but can be removed until needed to avoid dead code.

**[NIT]** ErrNotSupported is defined but not used. It's harmless but can be removed until needed to avoid dead code.
"errors"
"gitea.weiker.me/rodin/review-bot/gitea"
"gitea.weiker.me/rodin/review-bot/github"
"gitea.weiker.me/rodin/review-bot/review"
)
// ErrNotSupported is returned by VCS methods that have no implementation for
sonnet-review-bot commented 2026-05-14 21:12:18 +00:00
Review
Copy Link
Copy Source

[MINOR] ErrNotSupported is declared but never used — the comment on giteaExtClient says "GitHub implementations return ErrNotSupported for those", but githubVCSAdapter never returns it (those methods simply don't exist on the adapter). Either remove the variable or use it if the intent is to provide stub implementations. As-is, a linter would flag this as an unused export (though go vet won't).

**[MINOR]** `ErrNotSupported` is declared but never used — the comment on `giteaExtClient` says "GitHub implementations return ErrNotSupported for those", but `githubVCSAdapter` never returns it (those methods simply don't exist on the adapter). Either remove the variable or use it if the intent is to provide stub implementations. As-is, a linter would flag this as an unused export (though `go vet` won't).
// a particular VCS backend (e.g., Gitea-specific timeline APIs on GitHub).
var ErrNotSupported = errors.New("operation not supported on this VCS backend")
sonnet-review-bot commented 2026-05-14 20:45:20 +00:00
Review
Copy Link
Copy Source

[MINOR] ErrNotSupported is declared but never used anywhere in the codebase (the giteaExtClient type assertion in main.go gates calls cleanly without needing this error). Either use it (e.g., have githubVCSAdapter return it from a hypothetical future extension method) or remove it to avoid dead code. The comment on the type says 'GitHub implementations return ErrNotSupported for those' but githubVCSAdapter doesn't implement giteaExtClient at all, so this sentinel serves no purpose currently.

**[MINOR]** ErrNotSupported is declared but never used anywhere in the codebase (the giteaExtClient type assertion in main.go gates calls cleanly without needing this error). Either use it (e.g., have githubVCSAdapter return it from a hypothetical future extension method) or remove it to avoid dead code. The comment on the type says 'GitHub implementations return ErrNotSupported for those' but githubVCSAdapter doesn't implement giteaExtClient at all, so this sentinel serves no purpose currently.
// vcsClient is the interface for all PR operations used by main.go.
// It is implemented by both giteaVCSAdapter and githubVCSAdapter.
// Interface defined here (in the consumer package) per Go idiom.
type vcsClient interface {
// PR metadata and content
GetPullRequest(ctx context.Context, owner, repo string, number int) (*vcsPullRequest, error)
GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error)
GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcsChangedFile, error)
GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcsCommitStatus, error)
GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error)
GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error)
ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error)
GetAllFilesInPath(ctx context.Context, owner, repo, path string) (map[string]string, error)
// Review operations
PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []vcsReviewComment) (*vcsReview, error)
ListReviews(ctx context.Context, owner, repo string, number int) ([]vcsReview, error)
DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error
GetAuthenticatedUser(ctx context.Context) (string, error)
RequestReviewer(ctx context.Context, owner, repo string, number int, reviewer string) error
}
// giteaExtClient extends vcsClient with Gitea-specific operations that have no
// GitHub equivalent. Code that uses these methods should first do a type assertion.
type giteaExtClient interface {
vcsClient
GetTimelineReviewCommentIDForReview(ctx context.Context, owner, repo string, prNum, reviewID int64) (int64, error)
EditComment(ctx context.Context, owner, repo string, commentID int64, body string) error
sonnet-review-bot commented 2026-05-14 20:45:20 +00:00
Review
Copy Link
Copy Source

[MINOR] The giteaExtClient interface exposes []gitea.ReviewComment (a concrete gitea package type) through its ListReviewComments method signature. This breaks the abstraction layer — the interface in the consumer package (cmd/review-bot) now directly depends on the gitea package's internal types. Since giteaExtClient is Gitea-specific by design, this is pragmatically acceptable, but it's worth noting as a design smell. If a second Gitea-compatible VCS were added, the return type would need to change.

**[MINOR]** The giteaExtClient interface exposes []gitea.ReviewComment (a concrete gitea package type) through its ListReviewComments method signature. This breaks the abstraction layer — the interface in the consumer package (cmd/review-bot) now directly depends on the gitea package's internal types. Since giteaExtClient is Gitea-specific by design, this is pragmatically acceptable, but it's worth noting as a design smell. If a second Gitea-compatible VCS were added, the return type would need to change.
ListReviewComments(ctx context.Context, owner, repo string, prNum, reviewID int64) ([]gitea.ReviewComment, error)
ResolveComment(ctx context.Context, owner, repo string, commentID int64) error
}
// --- shared VCS types ---
// vcsPullRequest is VCS-agnostic PR metadata.
type vcsPullRequest struct {
Title string
Body string
Head struct {
Sha string
Ref string
}
}
// vcsChangedFile is a file changed in a PR.
type vcsChangedFile struct {
Filename string
Status string
}
// vcsCommitStatus is a CI status entry.
type vcsCommitStatus struct {
Status string
Context string
Description string
TargetURL string
}
// vcsReviewComment is an inline review comment.
type vcsReviewComment struct {
Path string
NewPosition int64 // Gitea: absolute line; GitHub: diff hunk position
sonnet-review-bot commented 2026-05-14 21:12:18 +00:00
Review
Copy Link
Copy Source

[NIT] vcsPullRequest uses an anonymous struct for Head rather than a named type. This is minor but means the struct literal syntax r.Head.Sha = ... across both adapters is slightly awkward. A named struct would be marginally cleaner, though the current approach matches what the gitea/github packages themselves do.

**[NIT]** `vcsPullRequest` uses an anonymous struct for `Head` rather than a named type. This is minor but means the struct literal syntax `r.Head.Sha = ...` across both adapters is slightly awkward. A named struct would be marginally cleaner, though the current approach matches what the gitea/github packages themselves do.
Body string
}
// vcsReview is a submitted PR review.
type vcsReview struct {
ID int64
Body string
CommitID string
User struct {
Login string
}
State string
}
// ============================================================
// giteaVCSAdapter
// ============================================================
// giteaVCSAdapter wraps gitea.Client to implement vcsClient + giteaExtClient.
type giteaVCSAdapter struct {
c *gitea.Client
}
func newGiteaVCSAdapter(c *gitea.Client) *giteaVCSAdapter { return &giteaVCSAdapter{c: c} }
func (a *giteaVCSAdapter) GetPullRequest(ctx context.Context, owner, repo string, number int) (*vcsPullRequest, error) {
pr, err := a.c.GetPullRequest(ctx, owner, repo, number)
if err != nil {
return nil, err
}
r := &vcsPullRequest{Title: pr.Title, Body: pr.Body}
r.Head.Sha = pr.Head.Sha
r.Head.Ref = pr.Head.Ref
return r, nil
}
func (a *giteaVCSAdapter) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) {
return a.c.GetPullRequestDiff(ctx, owner, repo, number)
}
func (a *giteaVCSAdapter) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcsChangedFile, error) {
files, err := a.c.GetPullRequestFiles(ctx, owner, repo, number)
if err != nil {
return nil, err
}
out := make([]vcsChangedFile, len(files))
for i, f := range files {
out[i] = vcsChangedFile{Filename: f.Filename, Status: f.Status}
}
return out, nil
}
func (a *giteaVCSAdapter) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcsCommitStatus, error) {
statuses, err := a.c.GetCommitStatuses(ctx, owner, repo, sha)
if err != nil {
return nil, err
}
out := make([]vcsCommitStatus, len(statuses))
for i, s := range statuses {
out[i] = vcsCommitStatus{Status: s.Status, Context: s.Context, Description: s.Description, TargetURL: s.TargetURL}
}
return out, nil
}
func (a *giteaVCSAdapter) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
return a.c.GetFileContent(ctx, owner, repo, filepath)
}
func (a *giteaVCSAdapter) GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) {
return a.c.GetFileContentRef(ctx, owner, repo, filepath, ref)
}
func (a *giteaVCSAdapter) ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error) {
entries, err := a.c.ListContents(ctx, owner, repo, path)
if err != nil {
return nil, err
}
out := make([]review.ContentEntry, len(entries))
for i, e := range entries {
out[i] = review.ContentEntry{Name: e.Name, Path: e.Path, Type: e.Type}
}
return out, nil
}
func (a *giteaVCSAdapter) GetAllFilesInPath(ctx context.Context, owner, repo, path string) (map[string]string, error) {
return a.c.GetAllFilesInPath(ctx, owner, repo, path)
}
func (a *giteaVCSAdapter) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []vcsReviewComment) (*vcsReview, error) {
gc := make([]gitea.ReviewComment, len(comments))
for i, c := range comments {
gc[i] = gitea.ReviewComment{Path: c.Path, NewPosition: c.NewPosition, Body: c.Body}
}
r, err := a.c.PostReview(ctx, owner, repo, number, event, body, commitID, gc)
if err != nil {
return nil, err
}
out := &vcsReview{ID: r.ID, Body: r.Body, CommitID: r.CommitID, State: r.State}
out.User.Login = r.User.Login
return out, nil
}
func (a *giteaVCSAdapter) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcsReview, error) {
reviews, err := a.c.ListReviews(ctx, owner, repo, number)
if err != nil {
return nil, err
}
out := make([]vcsReview, len(reviews))
for i, r := range reviews {
out[i] = vcsReview{ID: r.ID, Body: r.Body, CommitID: r.CommitID, State: r.State}
out[i].User.Login = r.User.Login
}
return out, nil
}
func (a *giteaVCSAdapter) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
return a.c.DeleteReview(ctx, owner, repo, number, reviewID)
}
func (a *giteaVCSAdapter) GetAuthenticatedUser(ctx context.Context) (string, error) {
return a.c.GetAuthenticatedUser(ctx)
}
func (a *giteaVCSAdapter) RequestReviewer(ctx context.Context, owner, repo string, number int, reviewer string) error {
return a.c.RequestReviewer(ctx, owner, repo, number, reviewer)
}
// Gitea-specific extension methods.
func (a *giteaVCSAdapter) GetTimelineReviewCommentIDForReview(ctx context.Context, owner, repo string, prNum, reviewID int64) (int64, error) {
return a.c.GetTimelineReviewCommentIDForReview(ctx, owner, repo, int(prNum), reviewID)
}
func (a *giteaVCSAdapter) EditComment(ctx context.Context, owner, repo string, commentID int64, body string) error {
return a.c.EditComment(ctx, owner, repo, commentID, body)
}
func (a *giteaVCSAdapter) ListReviewComments(ctx context.Context, owner, repo string, prNum, reviewID int64) ([]gitea.ReviewComment, error) {
return a.c.ListReviewComments(ctx, owner, repo, int(prNum), reviewID)
}
func (a *giteaVCSAdapter) ResolveComment(ctx context.Context, owner, repo string, commentID int64) error {
return a.c.ResolveComment(ctx, owner, repo, commentID)
}
// ============================================================
// githubVCSAdapter
// ============================================================
// githubVCSAdapter wraps github.Client to implement vcsClient.
// Gitea-specific extension methods (GetTimelineReviewCommentIDForReview, EditComment,
// ListReviewComments, ResolveComment) are not available on GitHub and will not be called
// because main.go gates them with a type assertion to giteaExtClient.
type githubVCSAdapter struct {
c *github.Client
}
func newGithubVCSAdapter(c *github.Client) *githubVCSAdapter { return &githubVCSAdapter{c: c} }
func (a *githubVCSAdapter) GetPullRequest(ctx context.Context, owner, repo string, number int) (*vcsPullRequest, error) {
pr, err := a.c.GetPullRequest(ctx, owner, repo, number)
if err != nil {
return nil, err
}
r := &vcsPullRequest{Title: pr.Title, Body: pr.Body}
r.Head.Sha = pr.Head.Sha
r.Head.Ref = pr.Head.Ref
return r, nil
}
func (a *githubVCSAdapter) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) {
return a.c.GetPullRequestDiff(ctx, owner, repo, number)
gpt-review-bot commented 2026-05-14 20:46:17 +00:00
Review
Copy Link
Copy Source

[NIT] Comment in githubVCSAdapter.PostReview says comments that cannot be mapped will be omitted, but the code unconditionally includes comments with Line+Side. Consider aligning the comment with behavior or adding filtering if needed.

**[NIT]** Comment in githubVCSAdapter.PostReview says comments that cannot be mapped will be omitted, but the code unconditionally includes comments with Line+Side. Consider aligning the comment with behavior or adding filtering if needed.
}
func (a *githubVCSAdapter) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]vcsChangedFile, error) {
files, err := a.c.GetPullRequestFiles(ctx, owner, repo, number)
if err != nil {
return nil, err
}
out := make([]vcsChangedFile, len(files))
for i, f := range files {
out[i] = vcsChangedFile{Filename: f.Filename, Status: f.Status}
}
return out, nil
}
func (a *githubVCSAdapter) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]vcsCommitStatus, error) {
statuses, err := a.c.GetCommitStatuses(ctx, owner, repo, sha)
if err != nil {
return nil, err
}
out := make([]vcsCommitStatus, len(statuses))
for i, s := range statuses {
// CommitStatus.Status is tagged as json:"state" — already the normalized "state" value
out[i] = vcsCommitStatus{Status: s.Status, Context: s.Context, Description: s.Description, TargetURL: s.TargetURL}
}
return out, nil
}
func (a *githubVCSAdapter) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
return a.c.GetFileContent(ctx, owner, repo, filepath)
}
func (a *githubVCSAdapter) GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) {
return a.c.GetFileContentRef(ctx, owner, repo, filepath, ref)
}
func (a *githubVCSAdapter) ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error) {
entries, err := a.c.ListContents(ctx, owner, repo, path)
if err != nil {
return nil, err
sonnet-review-bot commented 2026-05-14 20:45:20 +00:00
Review
Copy Link
Copy Source

[NIT] The comment on githubVCSAdapter.PostReview mentions that 'NewPosition from gitea diff parsing gives absolute line numbers, which will not match GitHub's position values' — this is accurate but the code proceeds to pass them anyway with Line+Side. Consider adding a TODO noting that proper GitHub diff hunk position mapping is needed for reliable inline comments, rather than just noting the limitation in a comment.

**[NIT]** The comment on githubVCSAdapter.PostReview mentions that 'NewPosition from gitea diff parsing gives absolute line numbers, which will not match GitHub's position values' — this is accurate but the code proceeds to pass them anyway with Line+Side. Consider adding a TODO noting that proper GitHub diff hunk position mapping is needed for reliable inline comments, rather than just noting the limitation in a comment.
}
out := make([]review.ContentEntry, len(entries))
for i, e := range entries {
out[i] = review.ContentEntry{Name: e.Name, Path: e.Path, Type: e.Type}
}
return out, nil
}
func (a *githubVCSAdapter) GetAllFilesInPath(ctx context.Context, owner, repo, path string) (map[string]string, error) {
return a.c.GetAllFilesInPath(ctx, owner, repo, path)
}
func (a *githubVCSAdapter) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []vcsReviewComment) (*vcsReview, error) {
gc := make([]github.ReviewComment, len(comments))
for i, c := range comments {
// GitHub inline comments use diff hunk "position", not absolute line numbers.
// NewPosition from gitea diff parsing gives absolute line numbers, which
// will not match GitHub's position values. For initial GitHub support, we
// attach comments with Line+Side (absolute line on the RIGHT side) instead.
// Comments that cannot be mapped will be omitted (GitHub rejects invalid positions).
gc[i] = github.ReviewComment{
Path: c.Path,
Line: c.NewPosition,
Side: "RIGHT",
Body: c.Body,
}
}
r, err := a.c.PostReview(ctx, owner, repo, number, event, body, commitID, gc)
if err != nil {
return nil, err
}
out := &vcsReview{ID: r.ID, Body: r.Body, State: r.State}
out.User.Login = r.User.Login
return out, nil
}
func (a *githubVCSAdapter) ListReviews(ctx context.Context, owner, repo string, number int) ([]vcsReview, error) {
reviews, err := a.c.ListReviews(ctx, owner, repo, number)
if err != nil {
return nil, err
}
out := make([]vcsReview, len(reviews))
for i, r := range reviews {
out[i] = vcsReview{ID: r.ID, Body: r.Body, State: r.State}
out[i].User.Login = r.User.Login
}
return out, nil
}
func (a *githubVCSAdapter) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
// GitHub only allows deleting PENDING (draft) reviews. review-bot posts submitted
// reviews, so this will return an error for any review we actually posted.
// Callers should treat 422 errors here gracefully.
return a.c.DeleteReview(ctx, owner, repo, number, reviewID)
}
func (a *githubVCSAdapter) GetAuthenticatedUser(ctx context.Context) (string, error) {
return a.c.GetAuthenticatedUser(ctx)
}
func (a *githubVCSAdapter) RequestReviewer(ctx context.Context, owner, repo string, number int, reviewer string) error {
return a.c.RequestReviewer(ctx, owner, repo, number, reviewer)
}