feat: implement GitHub API methods and VCS routing (issue #130) #131
@@ -14,6 +14,7 @@ import (
|
|||||||
|
|
||||||
"gitea.weiker.me/rodin/review-bot/budget"
|
"gitea.weiker.me/rodin/review-bot/budget"
|
||||||
"gitea.weiker.me/rodin/review-bot/gitea"
|
"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/llm"
|
||||||
"gitea.weiker.me/rodin/review-bot/review"
|
"gitea.weiker.me/rodin/review-bot/review"
|
||||||
)
|
)
|
||||||
@@ -169,7 +170,39 @@ func main() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Initialize clients
|
// Initialize clients
|
||||||
giteaClient := gitea.NewClient(*vcsURL, *reviewerToken)
|
// Detect VCS type: explicit flag > env var > URL heuristic (default: gitea).
|
||||||
|
vcsType := envOrDefault("VCS_TYPE", "")
|
||||||
|
if vcsType == "" {
|
||||||
|
|
|||||||
|
// 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
[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
[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)
|
llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel)
|
||||||
if *llmTemp < 0 || *llmTemp > 2 {
|
if *llmTemp < 0 || *llmTemp > 2 {
|
||||||
slog.Error("invalid LLM temperature", "temperature", *llmTemp, "range", "0-2")
|
slog.Error("invalid LLM temperature", "temperature", *llmTemp, "range", "0-2")
|
||||||
@@ -207,7 +240,7 @@ func main() {
|
|||||||
var persona *review.Persona
|
var persona *review.Persona
|
||||||
if *personaName != "" {
|
if *personaName != "" {
|
||||||
// Try loading from repo first, then fall back to built-in
|
// 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 {
|
if err != nil {
|
||||||
slog.Warn("could not load repo personas", "repo", owner+"/"+repoName, "error", err)
|
slog.Warn("could not load repo personas", "repo", owner+"/"+repoName, "error", err)
|
||||||
// Continue with built-in personas only.
|
// 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))
|
slog.Info("reviewing pull request", "pr", prNumber, "repo", fmt.Sprintf("%s/%s", owner, repoName))
|
||||||
|
|
||||||
// Step 1: Fetch PR metadata
|
// Step 1: Fetch PR metadata
|
||||||
pr, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber)
|
pr, err := vcs.GetPullRequest(ctx, owner, repoName, prNumber)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Error("failed to fetch PR", "pr", prNumber, "error", err)
|
slog.Error("failed to fetch PR", "pr", prNumber, "error", err)
|
||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
@@ -251,7 +284,7 @@ func main() {
|
|||||||
slog.Info("fetched PR metadata", "pr", prNumber, "title", pr.Title)
|
slog.Info("fetched PR metadata", "pr", prNumber, "title", pr.Title)
|
||||||
|
|
||||||
// Step 2: Fetch diff
|
// Step 2: Fetch diff
|
||||||
diff, err := giteaClient.GetPullRequestDiff(ctx, owner, repoName, prNumber)
|
diff, err := vcs.GetPullRequestDiff(ctx, owner, repoName, prNumber)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Error("failed to fetch diff", "pr", prNumber, "error", err)
|
slog.Error("failed to fetch diff", "pr", prNumber, "error", err)
|
||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
@@ -260,11 +293,11 @@ func main() {
|
|||||||
|
|
||||||
// Step 3: Fetch full file content for modified files
|
// Step 3: Fetch full file content for modified files
|
||||||
fileContext := ""
|
fileContext := ""
|
||||||
files, err := giteaClient.GetPullRequestFiles(ctx, owner, repoName, prNumber)
|
files, err := vcs.GetPullRequestFiles(ctx, owner, repoName, prNumber)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Warn("could not fetch PR files list", "pr", prNumber, "error", err)
|
slog.Warn("could not fetch PR files list", "pr", prNumber, "error", err)
|
||||||
} else {
|
} 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))
|
slog.Debug("fetched file context", "files", len(files))
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -272,7 +305,7 @@ func main() {
|
|||||||
ciPassed := true
|
ciPassed := true
|
||||||
ciDetails := ""
|
ciDetails := ""
|
||||||
if pr.Head.Sha != "" {
|
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 {
|
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 {
|
} else {
|
||||||
@@ -284,7 +317,7 @@ func main() {
|
|||||||
// Step 5: Load conventions file if specified
|
// Step 5: Load conventions file if specified
|
||||||
conventions := ""
|
conventions := ""
|
||||||
if *conventionsFile != "" {
|
if *conventionsFile != "" {
|
||||||
content, err := giteaClient.GetFileContent(ctx, owner, repoName, *conventionsFile)
|
content, err := vcs.GetFileContent(ctx, owner, repoName, *conventionsFile)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Warn("could not load conventions file", "file", *conventionsFile, "error", err)
|
slog.Warn("could not load conventions file", "file", *conventionsFile, "error", err)
|
||||||
} else {
|
} else {
|
||||||
@@ -296,7 +329,7 @@ func main() {
|
|||||||
// Step 6: Load patterns from external repo if specified
|
// Step 6: Load patterns from external repo if specified
|
||||||
patterns := ""
|
patterns := ""
|
||||||
if *patternsRepo != "" {
|
if *patternsRepo != "" {
|
||||||
patterns = fetchPatterns(ctx, giteaClient, *patternsRepo, *patternsFiles)
|
patterns = fetchPatterns(ctx, vcs, *patternsRepo, *patternsFiles)
|
||||||
slog.Debug("loaded patterns", "repo", *patternsRepo, "bytes", len(patterns))
|
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
|
// Stale check: verify HEAD hasn't moved since we started
|
||||||
evaluatedSHA := pr.Head.Sha
|
evaluatedSHA := pr.Head.Sha
|
||||||
var currentSHA string
|
var currentSHA string
|
||||||
currentPR, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber)
|
currentPR, err := vcs.GetPullRequest(ctx, owner, repoName, prNumber)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Warn("could not re-fetch PR for stale check", "pr", prNumber, "error", err)
|
slog.Warn("could not re-fetch PR for stale check", "pr", prNumber, "error", err)
|
||||||
// currentSHA stays empty — shouldSkipStaleReview will return false
|
// currentSHA stays empty — shouldSkipStaleReview will return false
|
||||||
@@ -428,10 +461,10 @@ func main() {
|
|||||||
|
|
||||||
// Map findings to inline comments for lines present in the diff
|
// Map findings to inline comments for lines present in the diff
|
||||||
diffRanges := gitea.ParseDiffNewLines(diff)
|
diffRanges := gitea.ParseDiffNewLines(diff)
|
||||||
var inlineComments []gitea.ReviewComment
|
var inlineComments []vcsReviewComment
|
||||||
for _, f := range result.Findings {
|
for _, f := range result.Findings {
|
||||||
if f.File != "" && f.Line > 0 && diffRanges.Contains(f.File, f.Line) {
|
if f.File != "" && f.Line > 0 && diffRanges.Contains(f.File, f.Line) {
|
||||||
inlineComments = append(inlineComments, gitea.ReviewComment{
|
inlineComments = append(inlineComments, vcsReviewComment{
|
||||||
Path: f.File,
|
Path: f.File,
|
||||||
NewPosition: int64(f.Line),
|
NewPosition: int64(f.Line),
|
||||||
Body: fmt.Sprintf("**[%s]** %s", f.Severity, f.Finding),
|
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)
|
// 1. POST new review first (gets non-stale approval badge on HEAD)
|
||||||
// 2. Then supersede old review with link to the new one
|
// 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.
|
// 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 != "" {
|
if *reviewerName != "" {
|
||||||
existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
|
existingReviews, err := vcs.ListReviews(ctx, owner, repoName, prNumber)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Warn("could not list existing reviews", "pr", prNumber, "error", err)
|
slog.Warn("could not list existing reviews", "pr", prNumber, "error", err)
|
||||||
} else {
|
} else {
|
||||||
@@ -461,11 +494,11 @@ func main() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Self-request as reviewer (ensures we appear in required-reviewer checks)
|
// Self-request as reviewer (ensures we appear in required-reviewer checks)
|
||||||
authUser, err := giteaClient.GetAuthenticatedUser(ctx)
|
authUser, err := vcs.GetAuthenticatedUser(ctx)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
slog.Warn("could not determine authenticated user for reviewer self-request", "error", err)
|
slog.Warn("could not determine authenticated user for reviewer self-request", "error", err)
|
||||||
} else if authUser != "" {
|
} 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)
|
slog.Warn("could not self-request as reviewer", "user", authUser, "error", err)
|
||||||
} else {
|
} else {
|
||||||
slog.Debug("self-requested as reviewer", "user", authUser, "pr", prNumber)
|
slog.Debug("self-requested as reviewer", "user", authUser, "pr", prNumber)
|
||||||
@@ -474,31 +507,34 @@ func main() {
|
|||||||
|
|
||||||
// POST new review
|
// POST new review
|
||||||
slog.Info("posting review", "event", event, "pr", prNumber)
|
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 {
|
if err != nil {
|
||||||
slog.Error("failed to post review", "pr", prNumber, "event", event, "error", err)
|
slog.Error("failed to post review", "pr", prNumber, "event", event, "error", err)
|
||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
}
|
}
|
||||||
slog.Info("review posted", "review_id", posted.ID, "user", posted.User.Login, "pr", prNumber)
|
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 with link to the new one.
|
||||||
if len(oldReviews) > 0 {
|
// 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)
|
newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*vcsURL, "/"), owner, repoName, prNumber, posted.ID)
|
||||||
for _, oldReview := range oldReviews {
|
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 {
|
if err != nil {
|
||||||
slog.Warn("could not find comment ID for old review", "review_id", oldReview.ID, "error", err)
|
slog.Warn("could not find comment ID for old review", "review_id", oldReview.ID, "error", err)
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
supersededBody := buildSupersededBody(oldReview.Body, oldReview.CommitID, newReviewURL, sentinel)
|
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)
|
slog.Warn("could not mark old review as superseded", "review_id", oldReview.ID, "comment_id", cid, "error", err)
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
slog.Info("marked old review as superseded", "review_id", oldReview.ID, "new_review_id", posted.ID, "pr", prNumber)
|
slog.Info("marked old review as superseded", "review_id", oldReview.ID, "new_review_id", posted.ID, "pr", prNumber)
|
||||||
|
|
||||||
// Resolve old review's inline comments
|
// 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 {
|
if err != nil {
|
||||||
slog.Warn("could not list old review comments for resolution", "review_id", oldReview.ID, "error", err)
|
slog.Warn("could not list old review comments for resolution", "review_id", oldReview.ID, "error", err)
|
||||||
continue
|
continue
|
||||||
@@ -508,7 +544,7 @@ func main() {
|
|||||||
if c.ID == 0 {
|
if c.ID == 0 {
|
||||||
continue
|
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)
|
slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err)
|
||||||
failed++
|
failed++
|
||||||
} else {
|
} else {
|
||||||
@@ -522,12 +558,14 @@ func main() {
|
|||||||
slog.Warn("some inline comments could not be resolved", "review_id", oldReview.ID, "failed", failed, "pr", prNumber)
|
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.
|
// 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
|
var sb strings.Builder
|
||||||
for _, f := range files {
|
for _, f := range files {
|
||||||
if ctx.Err() != nil {
|
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.
|
// 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 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.
|
// 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
|
var sb strings.Builder
|
||||||
|
|
||||||
repos := strings.Split(patternsRepo, ",")
|
repos := strings.Split(patternsRepo, ",")
|
||||||
@@ -631,7 +669,7 @@ func isPatternFile(path string) bool {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// evaluateCIStatus checks if all CI statuses indicate success.
|
// 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 {
|
if len(statuses) == 0 {
|
||||||
return true, "no CI statuses found"
|
return true, "no CI statuses found"
|
||||||
}
|
}
|
||||||
@@ -654,6 +692,19 @@ func evaluateCIStatus(statuses []gitea.CommitStatus) (passed bool, details strin
|
|||||||
return true, "all checks passed"
|
return true, "all checks passed"
|
||||||
|
[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 {
|
func envOrDefault(key, defaultVal string) string {
|
||||||
if v := os.Getenv(key); v != "" {
|
if v := os.Getenv(key); v != "" {
|
||||||
return 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
|
// Gitea user. This indicates misconfiguration where two roles share a token
|
||||||
// instead of having separate Gitea accounts. Returns true if shared token
|
// instead of having separate Gitea accounts. Returns true if shared token
|
||||||
// detected (caller should skip update-in-place logic to avoid clobbering).
|
// 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 := ""
|
ownLogin := ""
|
||||||
for _, r := range reviews {
|
for _, r := range reviews {
|
||||||
if strings.Contains(r.Body, ownSentinel) {
|
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.
|
// findOwnReview locates the most recent non-superseded review matching the sentinel.
|
||||||
func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review {
|
func findOwnReview(reviews []vcsReview, sentinel string) *vcsReview {
|
||||||
var best *gitea.Review
|
var best *vcsReview
|
||||||
for i := range reviews {
|
for i := range reviews {
|
||||||
if !strings.Contains(reviews[i].Body, sentinel) {
|
if !strings.Contains(reviews[i].Body, sentinel) {
|
||||||
continue
|
continue
|
||||||
@@ -824,8 +875,8 @@ func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// findAllOwnReviews returns all non-superseded reviews matching the sentinel.
|
// findAllOwnReviews returns all non-superseded reviews matching the sentinel.
|
||||||
func findAllOwnReviews(reviews []gitea.Review, sentinel string) []gitea.Review {
|
func findAllOwnReviews(reviews []vcsReview, sentinel string) []vcsReview {
|
||||||
var result []gitea.Review
|
var result []vcsReview
|
||||||
for i := range reviews {
|
for i := range reviews {
|
||||||
if !strings.Contains(reviews[i].Body, sentinel) {
|
if !strings.Contains(reviews[i].Body, sentinel) {
|
||||||
continue
|
continue
|
||||||
@@ -851,31 +902,4 @@ func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool {
|
|||||||
return evaluatedSHA != currentSHA
|
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)
|
|
||||||
}
|
|
||||||
|
|||||||
@@ -10,7 +10,6 @@ import (
|
|||||||
"strings"
|
"strings"
|
||||||
"testing"
|
"testing"
|
||||||
|
|
||||||
"gitea.weiker.me/rodin/review-bot/gitea"
|
|
||||||
)
|
)
|
||||||
|
|
||||||
func TestValidateReviewerName(t *testing.T) {
|
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 {
|
func makeReview(id int64, login, state string, _ bool, body string) vcsReview {
|
||||||
r := gitea.Review{
|
r := vcsReview{
|
||||||
ID: id,
|
ID: id,
|
||||||
Body: body,
|
Body: body,
|
||||||
State: state,
|
State: state,
|
||||||
Stale: stale,
|
|
||||||
}
|
}
|
||||||
r.User.Login = login
|
r.User.Login = login
|
||||||
return r
|
return r
|
||||||
@@ -216,7 +214,7 @@ func TestBuildSupersededBodyShortSHA(t *testing.T) {
|
|||||||
func TestFindOwnReview(t *testing.T) {
|
func TestFindOwnReview(t *testing.T) {
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
name string
|
name string
|
||||||
reviews []gitea.Review
|
reviews []vcsReview
|
||||||
sentinel string
|
sentinel string
|
||||||
wantID int64
|
wantID int64
|
||||||
wantNil bool
|
wantNil bool
|
||||||
@@ -229,7 +227,7 @@ func TestFindOwnReview(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "found by sentinel",
|
name: "found by sentinel",
|
||||||
reviews: []gitea.Review{
|
reviews: []vcsReview{
|
||||||
makeReview(42, "bot", "APPROVED", false, "review body\n<!-- review-bot:sonnet -->"),
|
makeReview(42, "bot", "APPROVED", false, "review body\n<!-- review-bot:sonnet -->"),
|
||||||
},
|
},
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
sentinel: "<!-- review-bot:sonnet -->",
|
||||||
@@ -237,7 +235,7 @@ func TestFindOwnReview(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "wrong sentinel",
|
name: "wrong sentinel",
|
||||||
reviews: []gitea.Review{
|
reviews: []vcsReview{
|
||||||
makeReview(42, "bot", "APPROVED", false, "body\n<!-- review-bot:gpt -->"),
|
makeReview(42, "bot", "APPROVED", false, "body\n<!-- review-bot:gpt -->"),
|
||||||
},
|
},
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
sentinel: "<!-- review-bot:sonnet -->",
|
||||||
@@ -245,7 +243,7 @@ func TestFindOwnReview(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "multiple reviews, returns first match",
|
name: "multiple reviews, returns first match",
|
||||||
reviews: []gitea.Review{
|
reviews: []vcsReview{
|
||||||
makeReview(10, "bot", "APPROVED", false, "old\n<!-- review-bot:gpt -->"),
|
makeReview(10, "bot", "APPROVED", false, "old\n<!-- review-bot:gpt -->"),
|
||||||
makeReview(20, "bot", "APPROVED", false, "new\n<!-- review-bot:sonnet -->"),
|
makeReview(20, "bot", "APPROVED", false, "new\n<!-- review-bot:sonnet -->"),
|
||||||
},
|
},
|
||||||
@@ -254,7 +252,7 @@ func TestFindOwnReview(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "skips superseded review",
|
name: "skips superseded review",
|
||||||
reviews: []gitea.Review{
|
reviews: []vcsReview{
|
||||||
makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n**Superseded**\n<!-- review-bot:sonnet -->"),
|
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 -->"),
|
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",
|
name: "only superseded reviews exist",
|
||||||
reviews: []gitea.Review{
|
reviews: []vcsReview{
|
||||||
makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n<!-- review-bot:sonnet -->"),
|
makeReview(10, "bot", "APPROVED", false, "~~Original review~~\n\n<!-- review-bot:sonnet -->"),
|
||||||
},
|
},
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
sentinel: "<!-- review-bot:sonnet -->",
|
||||||
@@ -271,7 +269,7 @@ func TestFindOwnReview(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "picks highest ID among matches",
|
name: "picks highest ID among matches",
|
||||||
reviews: []gitea.Review{
|
reviews: []vcsReview{
|
||||||
makeReview(50, "bot", "APPROVED", false, "v1\n<!-- review-bot:sonnet -->"),
|
makeReview(50, "bot", "APPROVED", false, "v1\n<!-- review-bot:sonnet -->"),
|
||||||
makeReview(30, "bot", "APPROVED", false, "v0\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) {
|
func TestHasSharedToken(t *testing.T) {
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
name string
|
name string
|
||||||
reviews []gitea.Review
|
reviews []vcsReview
|
||||||
sentinel string
|
sentinel string
|
||||||
want bool
|
want bool
|
||||||
}{
|
}{
|
||||||
@@ -314,36 +312,36 @@ func TestHasSharedToken(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "no own review yet - cannot detect",
|
name: "no own review yet - cannot detect",
|
||||||
reviews: []gitea.Review{
|
reviews: []vcsReview{
|
||||||
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "other"}, Body: "<!-- review-bot:gpt --> body"},
|
{ID: 1, User: struct{ Login string }{Login: "other"}, Body: "<!-- review-bot:gpt --> body"},
|
||||||
},
|
},
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
sentinel: "<!-- review-bot:sonnet -->",
|
||||||
want: false,
|
want: false,
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "separate users - no shared token",
|
name: "separate users - no shared token",
|
||||||
reviews: []gitea.Review{
|
reviews: []vcsReview{
|
||||||
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"},
|
{ID: 1, User: struct{ Login string }{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"},
|
{ID: 2, User: struct{ Login string }{Login: "security-review-bot"}, Body: "<!-- review-bot:security --> body"},
|
||||||
},
|
},
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
sentinel: "<!-- review-bot:sonnet -->",
|
||||||
want: false,
|
want: false,
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "shared token detected - same user different sentinels",
|
name: "shared token detected - same user different sentinels",
|
||||||
reviews: []gitea.Review{
|
reviews: []vcsReview{
|
||||||
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:sonnet --> body"},
|
{ID: 1, User: struct{ Login string }{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"},
|
{ID: 2, User: struct{ Login string }{Login: "sonnet-review-bot"}, Body: "<!-- review-bot:security --> body"},
|
||||||
},
|
},
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
sentinel: "<!-- review-bot:sonnet -->",
|
||||||
want: true,
|
want: true,
|
||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "three roles same user",
|
name: "three roles same user",
|
||||||
reviews: []gitea.Review{
|
reviews: []vcsReview{
|
||||||
{ID: 1, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:sonnet --> body"},
|
{ID: 1, User: struct{ Login string }{Login: "bot"}, Body: "<!-- review-bot:sonnet --> body"},
|
||||||
{ID: 2, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:security --> body"},
|
{ID: 2, User: struct{ Login string }{Login: "bot"}, Body: "<!-- review-bot:security --> body"},
|
||||||
{ID: 3, User: struct{ Login string `json:"login"` }{Login: "bot"}, Body: "<!-- review-bot:gpt --> body"},
|
{ID: 3, User: struct{ Login string }{Login: "bot"}, Body: "<!-- review-bot:gpt --> body"},
|
||||||
},
|
},
|
||||||
sentinel: "<!-- review-bot:sonnet -->",
|
sentinel: "<!-- review-bot:sonnet -->",
|
||||||
want: true,
|
want: true,
|
||||||
@@ -553,7 +551,7 @@ func TestBuildPatternPaths(t *testing.T) {
|
|||||||
func TestEvaluateCIStatus(t *testing.T) {
|
func TestEvaluateCIStatus(t *testing.T) {
|
||||||
tests := []struct {
|
tests := []struct {
|
||||||
name string
|
name string
|
||||||
statuses []gitea.CommitStatus
|
statuses []vcsCommitStatus
|
||||||
wantPassed bool
|
wantPassed bool
|
||||||
wantSubstr string
|
wantSubstr string
|
||||||
}{
|
}{
|
||||||
@@ -565,7 +563,7 @@ func TestEvaluateCIStatus(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "all success",
|
name: "all success",
|
||||||
statuses: []gitea.CommitStatus{
|
statuses: []vcsCommitStatus{
|
||||||
{Status: "success", Context: "ci/build", Description: "Build passed"},
|
{Status: "success", Context: "ci/build", Description: "Build passed"},
|
||||||
{Status: "success", Context: "ci/test", Description: "Tests passed"},
|
{Status: "success", Context: "ci/test", Description: "Tests passed"},
|
||||||
},
|
},
|
||||||
@@ -574,7 +572,7 @@ func TestEvaluateCIStatus(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "one failure",
|
name: "one failure",
|
||||||
statuses: []gitea.CommitStatus{
|
statuses: []vcsCommitStatus{
|
||||||
{Status: "success", Context: "ci/build", Description: "Build passed"},
|
{Status: "success", Context: "ci/build", Description: "Build passed"},
|
||||||
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
|
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
|
||||||
},
|
},
|
||||||
@@ -583,7 +581,7 @@ func TestEvaluateCIStatus(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "error status",
|
name: "error status",
|
||||||
statuses: []gitea.CommitStatus{
|
statuses: []vcsCommitStatus{
|
||||||
{Status: "error", Context: "ci/lint", Description: "Lint error"},
|
{Status: "error", Context: "ci/lint", Description: "Lint error"},
|
||||||
},
|
},
|
||||||
wantPassed: false,
|
wantPassed: false,
|
||||||
@@ -591,7 +589,7 @@ func TestEvaluateCIStatus(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "pending treated as not-failed",
|
name: "pending treated as not-failed",
|
||||||
statuses: []gitea.CommitStatus{
|
statuses: []vcsCommitStatus{
|
||||||
{Status: "pending", Context: "ci/build", Description: "In progress"},
|
{Status: "pending", Context: "ci/build", Description: "In progress"},
|
||||||
{Status: "success", Context: "ci/test", Description: "Tests passed"},
|
{Status: "success", Context: "ci/test", Description: "Tests passed"},
|
||||||
},
|
},
|
||||||
@@ -600,7 +598,7 @@ func TestEvaluateCIStatus(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "multiple failures",
|
name: "multiple failures",
|
||||||
statuses: []gitea.CommitStatus{
|
statuses: []vcsCommitStatus{
|
||||||
{Status: "failure", Context: "ci/build", Description: "Build failed"},
|
{Status: "failure", Context: "ci/build", Description: "Build failed"},
|
||||||
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
|
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
|
||||||
},
|
},
|
||||||
@@ -609,7 +607,7 @@ func TestEvaluateCIStatus(t *testing.T) {
|
|||||||
},
|
},
|
||||||
{
|
{
|
||||||
name: "mixed with pending and failure",
|
name: "mixed with pending and failure",
|
||||||
statuses: []gitea.CommitStatus{
|
statuses: []vcsCommitStatus{
|
||||||
{Status: "success", Context: "ci/build", Description: "Build passed"},
|
{Status: "success", Context: "ci/build", Description: "Build passed"},
|
||||||
{Status: "pending", Context: "ci/deploy", Description: "Deploying"},
|
{Status: "pending", Context: "ci/deploy", Description: "Deploying"},
|
||||||
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
|
{Status: "failure", Context: "ci/test", Description: "Tests failed"},
|
||||||
@@ -997,7 +995,7 @@ func cleanEnv() []string {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func TestFindAllOwnReviews(t *testing.T) {
|
func TestFindAllOwnReviews(t *testing.T) {
|
||||||
reviews := []gitea.Review{
|
reviews := []vcsReview{
|
||||||
{ID: 1, Body: "<!-- review-bot:sonnet -->\nfirst review"},
|
{ID: 1, Body: "<!-- review-bot:sonnet -->\nfirst review"},
|
||||||
{ID: 2, Body: "<!-- review-bot:gpt -->\nother bot"},
|
{ID: 2, Body: "<!-- review-bot:gpt -->\nother bot"},
|
||||||
{ID: 3, Body: "<!-- review-bot:sonnet -->\nsecond review"},
|
{ID: 3, Body: "<!-- review-bot:sonnet -->\nsecond review"},
|
||||||
|
|||||||
@@ -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
[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
[MINOR] **[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
[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
[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
[NIT] **[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
[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
[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)
|
||||||
|
}
|
||||||
@@ -4,7 +4,10 @@
|
|||||||
package github
|
package github
|
||||||
|
|
||||||
import (
|
import (
|
||||||
|
"bytes"
|
||||||
"context"
|
"context"
|
||||||
|
"encoding/base64"
|
||||||
|
"encoding/json"
|
||||||
"errors"
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"io"
|
"io"
|
||||||
@@ -92,10 +95,6 @@ func asAPIError(err error) (*APIError, bool) {
|
|||||||
// SetHTTPClient and SetRetryBackoff are intended for test setup only and must
|
// SetHTTPClient and SetRetryBackoff are intended for test setup only and must
|
||||||
// be called before any goroutines issue requests; they have no synchronization.
|
// be called before any goroutines issue requests; they have no synchronization.
|
||||||
type Client struct {
|
type Client struct {
|
||||||
// TODO: baseURL is populated by NewClient but not yet consumed by doRequest/doGet.
|
|
||||||
// Higher-level exported methods (GetPullRequest, etc.) will use it to
|
|
||||||
// construct request URLs; remove this field if those methods end up
|
|
||||||
// accepting full URLs instead.
|
|
||||||
baseURL string
|
baseURL string
|
||||||
token string
|
token string
|
||||||
httpClient *http.Client
|
httpClient *http.Client
|
||||||
@@ -376,3 +375,457 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
|
|||||||
func (c *Client) doGet(ctx context.Context, url string) ([]byte, error) {
|
func (c *Client) doGet(ctx context.Context, url string) ([]byte, error) {
|
||||||
return c.doRequest(ctx, http.MethodGet, url, "")
|
return c.doRequest(ctx, http.MethodGet, url, "")
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// doRequestWithBody performs an HTTP request with an optional body, applying the
|
||||||
|
// same HTTPS enforcement as doRequest. It is used by write methods (POST, PUT,
|
||||||
|
// DELETE) that bypass the retry loop in doRequest because write operations are
|
||||||
|
// not idempotent.
|
||||||
|
//
|
||||||
|
// body may be nil for requests that carry no payload (e.g. DELETE).
|
||||||
|
// When body is non-nil, Content-Type is set to application/json.
|
||||||
|
func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, body []byte) ([]byte, error) {
|
||||||
|
if !c.allowInsecureHTTP {
|
||||||
|
parsed, err := url.Parse(reqURL)
|
||||||
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("parse request URL: %w", err)
|
||||||
|
}
|
||||||
|
if strings.EqualFold(parsed.Scheme, "http") {
|
||||||
|
return nil, fmt.Errorf("refusing HTTP request to %s: use HTTPS or set AllowInsecureHTTP option", redactURL(reqURL))
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
var reqBody io.Reader
|
||||||
|
if body != nil {
|
||||||
|
reqBody = bytes.NewReader(body)
|
||||||
|
}
|
||||||
|
|
||||||
|
req, err := http.NewRequestWithContext(ctx, method, reqURL, reqBody)
|
||||||
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("create request: %w", err)
|
||||||
|
}
|
||||||
|
req.Header.Set("Authorization", "Bearer "+c.token)
|
||||||
|
req.Header.Set("Accept", "application/vnd.github+json")
|
||||||
|
if body != nil {
|
||||||
|
req.Header.Set("Content-Type", "application/json")
|
||||||
|
}
|
||||||
|
|
||||||
|
resp, err := c.httpClient.Do(req)
|
||||||
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("do request: %w", err)
|
||||||
|
}
|
||||||
|
defer resp.Body.Close()
|
||||||
|
|
||||||
|
if resp.StatusCode >= 200 && resp.StatusCode < 300 {
|
||||||
|
respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBodyBytes))
|
||||||
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("read response body: %w", err)
|
||||||
|
}
|
||||||
|
return respBody, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
errBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes))
|
||||||
|
return nil, &APIError{StatusCode: resp.StatusCode, Body: string(errBody)}
|
||||||
|
}
|
||||||
|
|
||||||
|
// --- API types ---
|
||||||
|
security-review-bot marked this conversation as resolved
[MAJOR] PostReview, DeleteReview, and RequestReviewer construct and execute HTTP requests directly (c.httpClient.Do) without the HTTPS-only guard used in doRequest. If baseURL is accidentally configured with an http:// scheme (e.g., GHES on HTTP), these methods will send the Authorization header over plaintext, exposing tokens. Enforce scheme checks consistently or reuse a common request helper for non-GET methods. **[MAJOR]** PostReview, DeleteReview, and RequestReviewer construct and execute HTTP requests directly (c.httpClient.Do) without the HTTPS-only guard used in doRequest. If baseURL is accidentally configured with an http:// scheme (e.g., GHES on HTTP), these methods will send the Authorization header over plaintext, exposing tokens. Enforce scheme checks consistently or reuse a common request helper for non-GET methods.
|
|||||||
|
|
||||||
|
// PullRequest holds relevant PR metadata.
|
||||||
|
sonnet-review-bot
commented
[NIT] getFileContentAtRef uses **[NIT]** getFileContentAtRef uses `filepath` as a parameter name, which shadows the standard library package `path/filepath`. While `filepath` is not imported in this file, the naming is potentially confusing. The gitea client likely uses a different name; consider `filePath` or `path` for consistency.
|
|||||||
|
type PullRequest struct {
|
||||||
|
Title string `json:"title"`
|
||||||
|
Body string `json:"body"`
|
||||||
|
Head struct {
|
||||||
|
Sha string `json:"sha"`
|
||||||
|
Ref string `json:"ref"`
|
||||||
|
} `json:"head"`
|
||||||
|
Draft bool `json:"draft"`
|
||||||
|
}
|
||||||
|
|
||||||
|
// CommitStatus represents a single CI status entry.
|
||||||
|
// GitHub returns "state" not "status"; this type uses Status for consistency
|
||||||
|
// with the gitea package (both are normalized before use).
|
||||||
|
type CommitStatus struct {
|
||||||
|
Status string `json:"state"` // GitHub field is "state"
|
||||||
|
Context string `json:"context"`
|
||||||
|
Description string `json:"description"`
|
||||||
|
TargetURL string `json:"target_url"`
|
||||||
|
}
|
||||||
|
|
||||||
|
// ChangedFile represents a file modified in a PR.
|
||||||
|
type ChangedFile struct {
|
||||||
|
Filename string `json:"filename"`
|
||||||
|
Status string `json:"status"`
|
||||||
|
}
|
||||||
|
|
||||||
|
// ReviewComment represents an inline comment to attach to a review.
|
||||||
|
// GitHub uses "position" (diff hunk position), whereas Gitea uses "new_position" (line number).
|
||||||
|
// When posting inline comments on GitHub, position is required; line numbers
|
||||||
|
// from the diff cannot be used directly.
|
||||||
|
type ReviewComment struct {
|
||||||
|
ID int64 `json:"id,omitempty"`
|
||||||
|
Path string `json:"path"`
|
||||||
|
Position int64 `json:"position,omitempty"` // GitHub diff hunk position
|
||||||
|
Line int64 `json:"line,omitempty"` // GitHub absolute line number (alternative to position)
|
||||||
|
Side string `json:"side,omitempty"` // "RIGHT" or "LEFT"
|
||||||
|
Body string `json:"body"`
|
||||||
|
}
|
||||||
|
|
||||||
|
// Review represents a pull request review from the GitHub API.
|
||||||
|
type Review struct {
|
||||||
|
ID int64 `json:"id"`
|
||||||
|
Body string `json:"body"`
|
||||||
|
User struct {
|
||||||
|
Login string `json:"login"`
|
||||||
|
} `json:"user"`
|
||||||
|
State string `json:"state"`
|
||||||
|
}
|
||||||
|
|
||||||
|
// contentResponse is the GitHub contents API response for a single file.
|
||||||
|
type contentResponse struct {
|
||||||
|
Name string `json:"name"`
|
||||||
|
Path string `json:"path"`
|
||||||
|
Type string `json:"type"` // "file" or "dir" or "symlink" or "submodule"
|
||||||
|
Content string `json:"content"` // Base64-encoded file content (with embedded newlines)
|
||||||
|
Encoding string `json:"encoding"` // "base64" or ""
|
||||||
|
}
|
||||||
|
|
||||||
|
// ContentEntry represents a file or directory entry from the contents API.
|
||||||
|
type ContentEntry struct {
|
||||||
|
Name string `json:"name"`
|
||||||
|
Path string `json:"path"`
|
||||||
|
Type string `json:"type"` // "file" or "dir"
|
||||||
|
}
|
||||||
|
|
||||||
|
// --- PR methods ---
|
||||||
|
|
||||||
|
// GetPullRequest fetches PR metadata.
|
||||||
|
func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) {
|
||||||
|
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d",
|
||||||
|
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
|
||||||
|
body, err := c.doGet(ctx, reqURL)
|
||||||
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("fetch PR: %w", err)
|
||||||
|
}
|
||||||
|
var pr PullRequest
|
||||||
|
if err := json.Unmarshal(body, &pr); err != nil {
|
||||||
|
return nil, fmt.Errorf("parse PR JSON: %w", err)
|
||||||
|
}
|
||||||
|
return &pr, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// GetPullRequestDiff fetches the unified diff for a PR.
|
||||||
|
func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) {
|
||||||
|
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d",
|
||||||
|
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
|
||||||
|
body, err := c.doRequest(ctx, http.MethodGet, reqURL, "application/vnd.github.diff")
|
||||||
|
if err != nil {
|
||||||
|
return "", fmt.Errorf("fetch diff: %w", err)
|
||||||
|
}
|
||||||
|
return string(body), nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// GetPullRequestFiles fetches the list of files changed in a PR.
|
||||||
|
// GitHub paginates this endpoint (100 per page max).
|
||||||
|
func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error) {
|
||||||
|
const perPage = 100
|
||||||
|
var all []ChangedFile
|
||||||
|
for page := 1; ; page++ {
|
||||||
|
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/files?per_page=%d&page=%d",
|
||||||
|
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, perPage, page)
|
||||||
|
body, err := c.doGet(ctx, reqURL)
|
||||||
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("fetch PR files (page %d): %w", page, err)
|
||||||
|
}
|
||||||
|
var batch []ChangedFile
|
||||||
|
if err := json.Unmarshal(body, &batch); err != nil {
|
||||||
|
return nil, fmt.Errorf("parse PR files JSON (page %d): %w", page, err)
|
||||||
|
}
|
||||||
|
all = append(all, batch...)
|
||||||
|
if len(batch) < perPage {
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return all, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// GetCommitStatuses fetches CI statuses for a commit SHA.
|
||||||
|
// GitHub has two status systems: legacy "commit statuses" and newer "check runs".
|
||||||
|
// This method returns commit statuses only; check runs are a separate API.
|
||||||
|
// Note: GitHub returns "state" in the JSON; CommitStatus.Status is tagged accordingly.
|
||||||
|
func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]CommitStatus, error) {
|
||||||
|
const perPage = 100
|
||||||
|
var all []CommitStatus
|
||||||
|
for page := 1; ; page++ {
|
||||||
|
reqURL := fmt.Sprintf("%s/repos/%s/%s/commits/%s/statuses?per_page=%d&page=%d",
|
||||||
|
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha), perPage, page)
|
||||||
|
body, err := c.doGet(ctx, reqURL)
|
||||||
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("fetch commit statuses (page %d): %w", page, err)
|
||||||
|
}
|
||||||
|
var batch []CommitStatus
|
||||||
|
if err := json.Unmarshal(body, &batch); err != nil {
|
||||||
|
return nil, fmt.Errorf("parse statuses JSON (page %d): %w", page, err)
|
||||||
|
}
|
||||||
|
all = append(all, batch...)
|
||||||
|
if len(batch) < perPage {
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return all, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// --- File content methods ---
|
||||||
|
|
||||||
|
// GetFileContent fetches a file from the default branch of a repo.
|
||||||
|
// GitHub returns base64-encoded content; this method decodes it.
|
||||||
|
func (c *Client) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
|
||||||
|
return c.getFileContentAtRef(ctx, owner, repo, filepath, "")
|
||||||
|
}
|
||||||
|
|
||||||
|
// GetFileContentRef fetches a file from a specific ref (branch/tag/sha).
|
||||||
|
func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) {
|
||||||
|
return c.getFileContentAtRef(ctx, owner, repo, filepath, ref)
|
||||||
|
}
|
||||||
|
|
||||||
|
// getFileContentAtRef fetches a file at the given ref (empty = default branch).
|
||||||
|
// GitHub's contents API returns base64-encoded file content.
|
||||||
|
func (c *Client) getFileContentAtRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) {
|
||||||
|
reqURL := fmt.Sprintf("%s/repos/%s/%s/contents/%s",
|
||||||
|
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(filepath))
|
||||||
|
if ref != "" {
|
||||||
|
reqURL += "?ref=" + url.QueryEscape(ref)
|
||||||
|
}
|
||||||
|
body, err := c.doGet(ctx, reqURL)
|
||||||
|
if err != nil {
|
||||||
|
return "", fmt.Errorf("fetch file %s: %w", filepath, err)
|
||||||
|
}
|
||||||
|
var resp contentResponse
|
||||||
|
if err := json.Unmarshal(body, &resp); err != nil {
|
||||||
|
return "", fmt.Errorf("parse file content JSON for %s: %w", filepath, err)
|
||||||
|
}
|
||||||
|
if resp.Type != "file" {
|
||||||
|
return "", fmt.Errorf("path %s is a %s, not a file", filepath, resp.Type)
|
||||||
|
}
|
||||||
|
if resp.Encoding == "base64" {
|
||||||
|
// GitHub embeds newlines in the base64 content for readability.
|
||||||
|
// Strip them before decoding.
|
||||||
|
cleaned := strings.ReplaceAll(resp.Content, "\n", "")
|
||||||
|
decoded, err := base64.StdEncoding.DecodeString(cleaned)
|
||||||
|
if err != nil {
|
||||||
|
return "", fmt.Errorf("decode base64 content for %s: %w", filepath, err)
|
||||||
|
}
|
||||||
|
return string(decoded), nil
|
||||||
|
}
|
||||||
|
sonnet-review-bot
commented
[MINOR] In **[MINOR]** In `GetAllFilesInPath`, when `ListContents` returns a 404, the code falls back to `GetFileContent`. But `ListContents` on a file path returns the file as a single-object response (normalized to a `[]ContentEntry` slice) — it does NOT return 404 for files. The 404 fallback path is dead code for GitHub and misleads readers. The comment `// 404 means path may be a file — try fetching directly` is accurate for Gitea but not for this GitHub client where `ListContents` already handles the single-file case. This won't cause a bug, but it's confusing and the fallback is untested.
|
|||||||
|
// Non-base64 encoding (shouldn't happen normally, but handle gracefully).
|
||||||
|
return resp.Content, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// ListContents lists files and directories at a given path.
|
||||||
|
// Pass an empty path to list the repository root.
|
||||||
|
// GitHub returns a single object (not array) when path is a file — this
|
||||||
|
// method normalizes both cases to a slice, matching Gitea's behavior.
|
||||||
|
func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) {
|
||||||
|
var reqURL string
|
||||||
|
if path == "" || path == "." {
|
||||||
|
reqURL = fmt.Sprintf("%s/repos/%s/%s/contents",
|
||||||
|
c.baseURL, url.PathEscape(owner), url.PathEscape(repo))
|
||||||
|
} else {
|
||||||
|
reqURL = fmt.Sprintf("%s/repos/%s/%s/contents/%s",
|
||||||
|
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(path))
|
||||||
|
}
|
||||||
|
body, err := c.doGet(ctx, reqURL)
|
||||||
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("list contents %s: %w", path, err)
|
||||||
|
}
|
||||||
|
|
||||||
|
var entries []ContentEntry
|
||||||
|
if err := json.Unmarshal(body, &entries); err != nil {
|
||||||
|
// GitHub returns a single object when path is a file (not an array).
|
||||||
|
var single contentResponse
|
||||||
|
if err2 := json.Unmarshal(body, &single); err2 != nil {
|
||||||
|
return nil, fmt.Errorf("parse contents JSON: %w", err)
|
||||||
|
}
|
||||||
|
if single.Name == "" && single.Path == "" {
|
||||||
|
return nil, fmt.Errorf("parse contents JSON: empty response for path %q", path)
|
||||||
|
}
|
||||||
|
entries = []ContentEntry{{
|
||||||
|
Name: single.Name,
|
||||||
|
Path: single.Path,
|
||||||
|
Type: single.Type,
|
||||||
|
}}
|
||||||
|
}
|
||||||
|
return entries, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// GetAllFilesInPath recursively fetches all file contents under a path.
|
||||||
|
// If the path is a file, returns just that file's content.
|
||||||
|
// If the path is a directory, recursively fetches all files within it.
|
||||||
|
func (c *Client) GetAllFilesInPath(ctx context.Context, owner, repo, path string) (map[string]string, error) {
|
||||||
|
results := make(map[string]string)
|
||||||
|
|
||||||
|
entries, err := c.ListContents(ctx, owner, repo, path)
|
||||||
|
if err != nil {
|
||||||
|
if !IsNotFound(err) {
|
||||||
|
return nil, fmt.Errorf("list contents %q: %w", path, err)
|
||||||
|
}
|
||||||
|
// 404 means path may be a file — try fetching directly.
|
||||||
|
content, fileErr := c.GetFileContent(ctx, owner, repo, path)
|
||||||
|
if fileErr != nil {
|
||||||
|
return nil, fmt.Errorf("path %q is neither a file nor directory: %w", path, fileErr)
|
||||||
|
}
|
||||||
|
results[path] = content
|
||||||
|
return results, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
for _, entry := range entries {
|
||||||
|
switch entry.Type {
|
||||||
|
case "file":
|
||||||
|
content, err := c.GetFileContent(ctx, owner, repo, entry.Path)
|
||||||
|
if err != nil {
|
||||||
|
slog.Warn("could not fetch file from patterns repo", "file", entry.Path, "error", err)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
results[entry.Path] = content
|
||||||
|
case "dir":
|
||||||
|
subResults, err := c.GetAllFilesInPath(ctx, owner, repo, entry.Path)
|
||||||
|
if err != nil {
|
||||||
|
slog.Warn("could not recurse into directory", "dir", entry.Path, "error", err)
|
||||||
|
continue
|
||||||
|
}
|
||||||
|
for k, v := range subResults {
|
||||||
|
results[k] = v
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return results, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// --- Review methods ---
|
||||||
|
|
||||||
|
// PostReview submits a review to a PR.
|
||||||
|
// event should be one of "APPROVE", "REQUEST_CHANGES", or "COMMENT".
|
||||||
|
// commitID anchors the review to a specific commit SHA. If empty, defaults to current HEAD.
|
||||||
|
// comments are optional inline comments; GitHub uses diff hunk position (not line numbers).
|
||||||
|
// Note: unlike Gitea, GitHub does not support deleting submitted reviews.
|
||||||
|
// Use COMMENT event to supersede old reviews.
|
||||||
|
func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []ReviewComment) (*Review, error) {
|
||||||
|
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews",
|
||||||
|
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
|
||||||
|
|
||||||
|
payload := struct {
|
||||||
|
Body string `json:"body"`
|
||||||
|
Event string `json:"event"`
|
||||||
|
CommitID string `json:"commit_id,omitempty"`
|
||||||
|
Comments []ReviewComment `json:"comments,omitempty"`
|
||||||
|
}{
|
||||||
|
Body: body,
|
||||||
|
Event: event,
|
||||||
|
CommitID: commitID,
|
||||||
|
Comments: comments,
|
||||||
|
}
|
||||||
|
|
||||||
|
data, err := json.Marshal(payload)
|
||||||
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("marshal review payload: %w", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
respBody, err := c.doRequestWithBody(ctx, http.MethodPost, reqURL, data)
|
||||||
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("post review: %w", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
var review Review
|
||||||
|
if err := json.Unmarshal(respBody, &review); err != nil {
|
||||||
|
return nil, fmt.Errorf("parse review response: %w", err)
|
||||||
|
}
|
||||||
|
return &review, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// ListReviews returns all reviews on a pull request.
|
||||||
|
// GitHub paginates via Link header; this method uses per_page=100.
|
||||||
|
func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error) {
|
||||||
|
const perPage = 100
|
||||||
|
var all []Review
|
||||||
|
for page := 1; ; page++ {
|
||||||
|
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews?per_page=%d&page=%d",
|
||||||
|
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, perPage, page)
|
||||||
|
body, err := c.doGet(ctx, reqURL)
|
||||||
|
if err != nil {
|
||||||
|
return nil, fmt.Errorf("list reviews (page %d): %w", page, err)
|
||||||
|
}
|
||||||
|
var batch []Review
|
||||||
|
if err := json.Unmarshal(body, &batch); err != nil {
|
||||||
|
return nil, fmt.Errorf("parse reviews (page %d): %w", page, err)
|
||||||
|
}
|
||||||
|
all = append(all, batch...)
|
||||||
|
if len(batch) < perPage {
|
||||||
|
break
|
||||||
|
}
|
||||||
|
}
|
||||||
|
return all, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// DeleteReview attempts to delete a pull request review.
|
||||||
|
// GitHub only allows deleting PENDING (draft) reviews. Submitted reviews cannot
|
||||||
|
// be deleted via the API; this method returns a descriptive error in that case.
|
||||||
|
// review-bot callers should handle this error gracefully (e.g., by not attempting
|
||||||
|
// supersede and instead posting a new review alongside the old one).
|
||||||
|
func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
|
||||||
|
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d",
|
||||||
|
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
|
||||||
|
|
||||||
|
// nil body: the GitHub DELETE endpoint for reviews requires no request body.
|
||||||
|
_, err := c.doRequestWithBody(ctx, http.MethodDelete, reqURL, nil)
|
||||||
|
if err != nil {
|
||||||
|
return fmt.Errorf("delete review: %w", err)
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// GetAuthenticatedUser returns the login of the authenticated user.
|
||||||
|
func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) {
|
||||||
|
reqURL := c.baseURL + "/user"
|
||||||
|
body, err := c.doGet(ctx, reqURL)
|
||||||
|
if err != nil {
|
||||||
|
return "", fmt.Errorf("get authenticated user: %w", err)
|
||||||
|
}
|
||||||
|
var result struct {
|
||||||
|
Login string `json:"login"`
|
||||||
|
}
|
||||||
|
if err := json.Unmarshal(body, &result); err != nil {
|
||||||
|
return "", fmt.Errorf("parse user response: %w", err)
|
||||||
|
}
|
||||||
|
return result.Login, nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// RequestReviewer adds a user as a requested reviewer on a pull request.
|
||||||
|
// This is idempotent — requesting an already-requested reviewer is a no-op.
|
||||||
|
func (c *Client) RequestReviewer(ctx context.Context, owner, repo string, number int, reviewer string) error {
|
||||||
|
reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/requested_reviewers",
|
||||||
|
c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
|
||||||
|
|
||||||
|
payload := struct {
|
||||||
|
Reviewers []string `json:"reviewers"`
|
||||||
|
}{Reviewers: []string{reviewer}}
|
||||||
|
data, err := json.Marshal(payload)
|
||||||
|
if err != nil {
|
||||||
|
return fmt.Errorf("marshal reviewer request: %w", err)
|
||||||
|
}
|
||||||
|
|
||||||
|
_, err = c.doRequestWithBody(ctx, http.MethodPost, reqURL, data)
|
||||||
|
if err != nil {
|
||||||
|
return fmt.Errorf("request reviewer: %w", err)
|
||||||
|
}
|
||||||
|
return nil
|
||||||
|
}
|
||||||
|
|
||||||
|
// --- helpers ---
|
||||||
|
|
||||||
|
// escapePath escapes each segment of a relative file path for use in URLs.
|
||||||
|
// Slashes are preserved as path separators; other special characters are escaped.
|
||||||
|
func escapePath(p string) string {
|
||||||
|
parts := strings.Split(p, "/")
|
||||||
|
for i, part := range parts {
|
||||||
|
parts[i] = url.PathEscape(part)
|
||||||
|
}
|
||||||
|
return strings.Join(parts, "/")
|
||||||
|
}
|
||||||
|
|||||||
@@ -2,7 +2,9 @@ package github
|
|||||||
|
|
||||||
import (
|
import (
|
||||||
"context"
|
"context"
|
||||||
|
"encoding/json"
|
||||||
"errors"
|
"errors"
|
||||||
|
"fmt"
|
||||||
"net/http"
|
"net/http"
|
||||||
"net/http/httptest"
|
"net/http/httptest"
|
||||||
"net/url"
|
"net/url"
|
||||||
@@ -656,3 +658,475 @@ func TestRedactURL_UserinfoWithQuery(t *testing.T) {
|
|||||||
t.Errorf("redactURL = %q, want %q", got, want)
|
t.Errorf("redactURL = %q, want %q", got, want)
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// --- Tests for API methods ---
|
||||||
|
|
||||||
|
func TestGetPullRequest_Success(t *testing.T) {
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
if r.URL.Path != "/repos/owner/repo/pulls/42" {
|
||||||
|
t.Errorf("unexpected path: %s", r.URL.Path)
|
||||||
|
}
|
||||||
|
w.WriteHeader(http.StatusOK)
|
||||||
|
w.Write([]byte(`{"title":"Test PR","body":"description","head":{"sha":"abc123","ref":"feature"},"draft":false}`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
pr, err := c.GetPullRequest(context.Background(), "owner", "repo", 42)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if pr.Title != "Test PR" {
|
||||||
|
t.Errorf("Title = %q, want %q", pr.Title, "Test PR")
|
||||||
|
}
|
||||||
|
if pr.Head.Sha != "abc123" {
|
||||||
|
t.Errorf("Head.Sha = %q, want %q", pr.Head.Sha, "abc123")
|
||||||
|
}
|
||||||
|
if pr.Head.Ref != "feature" {
|
||||||
|
t.Errorf("Head.Ref = %q, want %q", pr.Head.Ref, "feature")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestGetPullRequest_NotFound(t *testing.T) {
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
w.WriteHeader(http.StatusNotFound)
|
||||||
|
w.Write([]byte(`{"message":"Not Found"}`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
_, err := c.GetPullRequest(context.Background(), "owner", "repo", 99)
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected error, got nil")
|
||||||
|
}
|
||||||
|
if !IsNotFound(err) {
|
||||||
|
t.Errorf("expected IsNotFound=true, got false for error: %v", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestGetPullRequestDiff_Success(t *testing.T) {
|
||||||
|
const wantDiff = "diff --git a/foo.go b/foo.go\n--- a/foo.go\n+++ b/foo.go\n"
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
if r.Header.Get("Accept") != "application/vnd.github.diff" {
|
||||||
|
t.Errorf("Accept = %q, want application/vnd.github.diff", r.Header.Get("Accept"))
|
||||||
|
}
|
||||||
|
w.WriteHeader(http.StatusOK)
|
||||||
|
w.Write([]byte(wantDiff))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
diff, err := c.GetPullRequestDiff(context.Background(), "owner", "repo", 1)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if diff != wantDiff {
|
||||||
|
t.Errorf("diff = %q, want %q", diff, wantDiff)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestGetPullRequestFiles_Success(t *testing.T) {
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
w.WriteHeader(http.StatusOK)
|
||||||
|
w.Write([]byte(`[{"filename":"foo.go","status":"modified"},{"filename":"bar.go","status":"added"}]`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
files, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if len(files) != 2 {
|
||||||
|
t.Fatalf("len(files) = %d, want 2", len(files))
|
||||||
|
}
|
||||||
|
if files[0].Filename != "foo.go" || files[0].Status != "modified" {
|
||||||
|
t.Errorf("files[0] = %+v, want {foo.go modified}", files[0])
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestGetPullRequestFiles_Paginated(t *testing.T) {
|
||||||
|
page := 0
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
page++
|
||||||
|
if page == 1 {
|
||||||
|
// Return 100 items (page full → expect another request)
|
||||||
|
items := make([]map[string]string, 100)
|
||||||
|
for i := range items {
|
||||||
|
items[i] = map[string]string{"filename": fmt.Sprintf("file%d.go", i), "status": "modified"}
|
||||||
|
}
|
||||||
|
data, _ := json.Marshal(items)
|
||||||
|
w.WriteHeader(http.StatusOK)
|
||||||
|
w.Write(data)
|
||||||
|
return
|
||||||
|
}
|
||||||
|
// Page 2: return fewer than perPage → stop
|
||||||
|
w.WriteHeader(http.StatusOK)
|
||||||
|
w.Write([]byte(`[{"filename":"last.go","status":"added"}]`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
files, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if len(files) != 101 {
|
||||||
|
t.Errorf("len(files) = %d, want 101", len(files))
|
||||||
|
sonnet-review-bot
commented
[NIT] In **[NIT]** In `TestGetFileContent_Base64WithNewlines`, the `encoded` variable is declared and assigned but only used via `_ = encoded` (a suppress-unused comment). The actual test hardcodes the string in the `body` literal. This is confusing — either use the variable to build the response body, or remove the variable entirely.
|
|||||||
|
}
|
||||||
|
if page != 2 {
|
||||||
|
t.Errorf("page = %d, want 2", page)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestGetCommitStatuses_Success(t *testing.T) {
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
w.WriteHeader(http.StatusOK)
|
||||||
|
// GitHub uses "state" field
|
||||||
|
w.Write([]byte(`[{"state":"success","context":"ci/test","description":"Tests pass","target_url":"https://ci.example.com"}]`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
statuses, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "deadbeef")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if len(statuses) != 1 {
|
||||||
|
t.Fatalf("len(statuses) = %d, want 1", len(statuses))
|
||||||
|
}
|
||||||
|
if statuses[0].Status != "success" {
|
||||||
|
t.Errorf("Status = %q, want %q", statuses[0].Status, "success")
|
||||||
|
}
|
||||||
|
if statuses[0].Context != "ci/test" {
|
||||||
|
t.Errorf("Context = %q, want %q", statuses[0].Context, "ci/test")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestGetFileContent_Base64(t *testing.T) {
|
||||||
|
// "hello world\n" base64-encoded with embedded newlines (as GitHub does it)
|
||||||
|
encoded := "aGVsbG8gd29ybGQK"
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
if !strings.HasSuffix(r.URL.Path, "/contents/README.md") {
|
||||||
|
t.Errorf("unexpected path: %s", r.URL.Path)
|
||||||
|
}
|
||||||
|
w.WriteHeader(http.StatusOK)
|
||||||
|
w.Write([]byte(`{"name":"README.md","path":"README.md","type":"file","content":"` + encoded + `","encoding":"base64"}`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
sonnet-review-bot
commented
[NIT] In TestGetFileContent_Base64WithNewlines, the variable **[NIT]** In TestGetFileContent_Base64WithNewlines, the variable `encoded` is declared and then immediately suppressed with `_ = encoded`. The variable is unused because the test hardcodes the JSON string inline. Either remove the `encoded` variable declaration entirely or use it to construct the response body.
|
|||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
content, err := c.GetFileContent(context.Background(), "owner", "repo", "README.md")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if content != "hello world\n" {
|
||||||
|
t.Errorf("content = %q, want %q", content, "hello world\n")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestGetFileContent_Base64WithNewlines(t *testing.T) {
|
||||||
|
// GitHub embeds newlines in base64 content for readability (every 60 chars)
|
||||||
|
// Test that we strip them correctly before decoding
|
||||||
|
// "hello world\n" = aGVsbG8gd29ybGQK — split it with embedded \n
|
||||||
|
encoded := "aGVs\nbG8g\nd29y\nbGQK"
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
w.WriteHeader(http.StatusOK)
|
||||||
|
// JSON-encode the embedded newlines as \n
|
||||||
|
body := `{"name":"README.md","path":"README.md","type":"file","content":"aGVs\nbG8g\nd29y\nbGQK","encoding":"base64"}`
|
||||||
|
_ = encoded // suppress unused warning
|
||||||
|
w.Write([]byte(body))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
content, err := c.GetFileContent(context.Background(), "owner", "repo", "README.md")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if content != "hello world\n" {
|
||||||
|
t.Errorf("content = %q, want %q", content, "hello world\n")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestGetFileContent_IsDirectory(t *testing.T) {
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
w.WriteHeader(http.StatusOK)
|
||||||
|
w.Write([]byte(`{"name":"docs","path":"docs","type":"dir","content":"","encoding":""}`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
_, err := c.GetFileContent(context.Background(), "owner", "repo", "docs")
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected error for directory, got nil")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestGetFileContentRef_Success(t *testing.T) {
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
if r.URL.Query().Get("ref") != "main" {
|
||||||
|
t.Errorf("ref = %q, want %q", r.URL.Query().Get("ref"), "main")
|
||||||
|
}
|
||||||
|
encoded := "dGVzdA==" // "test"
|
||||||
|
w.WriteHeader(http.StatusOK)
|
||||||
|
w.Write([]byte(`{"name":"foo.go","path":"foo.go","type":"file","content":"` + encoded + `","encoding":"base64"}`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
content, err := c.GetFileContentRef(context.Background(), "owner", "repo", "foo.go", "main")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if content != "test" {
|
||||||
|
t.Errorf("content = %q, want %q", content, "test")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestListContents_Directory(t *testing.T) {
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
w.WriteHeader(http.StatusOK)
|
||||||
|
w.Write([]byte(`[{"name":"foo.go","path":"foo.go","type":"file"},{"name":"bar","path":"bar","type":"dir"}]`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
entries, err := c.ListContents(context.Background(), "owner", "repo", "")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if len(entries) != 2 {
|
||||||
|
t.Fatalf("len(entries) = %d, want 2", len(entries))
|
||||||
|
}
|
||||||
|
if entries[0].Name != "foo.go" || entries[0].Type != "file" {
|
||||||
|
t.Errorf("entries[0] = %+v, unexpected", entries[0])
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestListContents_SingleFile(t *testing.T) {
|
||||||
|
// GitHub returns a single object when the path is a file
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
w.WriteHeader(http.StatusOK)
|
||||||
|
w.Write([]byte(`{"name":"README.md","path":"README.md","type":"file","content":"","encoding":""}`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
entries, err := c.ListContents(context.Background(), "owner", "repo", "README.md")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if len(entries) != 1 {
|
||||||
|
t.Fatalf("len(entries) = %d, want 1", len(entries))
|
||||||
|
}
|
||||||
|
if entries[0].Name != "README.md" {
|
||||||
|
t.Errorf("entries[0].Name = %q, want README.md", entries[0].Name)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestPostReview_Success(t *testing.T) {
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
if r.Method != http.MethodPost {
|
||||||
|
t.Errorf("method = %s, want POST", r.Method)
|
||||||
|
}
|
||||||
|
if r.URL.Path != "/repos/owner/repo/pulls/1/reviews" {
|
||||||
|
t.Errorf("path = %s, unexpected", r.URL.Path)
|
||||||
|
}
|
||||||
|
var payload map[string]interface{}
|
||||||
|
if err := json.NewDecoder(r.Body).Decode(&payload); err != nil {
|
||||||
|
t.Errorf("decode body: %v", err)
|
||||||
|
}
|
||||||
|
if payload["event"] != "APPROVE" {
|
||||||
|
t.Errorf("event = %v, want APPROVE", payload["event"])
|
||||||
|
}
|
||||||
|
w.WriteHeader(http.StatusOK)
|
||||||
|
w.Write([]byte(`{"id":99,"body":"looks good","user":{"login":"bot"},"state":"APPROVED"}`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
review, err := c.PostReview(context.Background(), "owner", "repo", 1, "APPROVE", "looks good", "abc", nil)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if review.ID != 99 {
|
||||||
|
t.Errorf("review.ID = %d, want 99", review.ID)
|
||||||
|
}
|
||||||
|
if review.User.Login != "bot" {
|
||||||
|
t.Errorf("review.User.Login = %q, want bot", review.User.Login)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestPostReview_Unauthorized(t *testing.T) {
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
w.WriteHeader(http.StatusUnauthorized)
|
||||||
|
w.Write([]byte(`{"message":"Bad credentials"}`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("bad-tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
_, err := c.PostReview(context.Background(), "owner", "repo", 1, "APPROVE", "body", "", nil)
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected error, got nil")
|
||||||
|
}
|
||||||
|
if !IsUnauthorized(err) {
|
||||||
|
t.Errorf("expected IsUnauthorized=true, got false for error: %v", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestListReviews_Success(t *testing.T) {
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
w.WriteHeader(http.StatusOK)
|
||||||
|
w.Write([]byte(`[{"id":1,"body":"review 1","user":{"login":"alice"},"state":"APPROVED"},{"id":2,"body":"review 2","user":{"login":"bob"},"state":"CHANGES_REQUESTED"}]`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
reviews, err := c.ListReviews(context.Background(), "owner", "repo", 1)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if len(reviews) != 2 {
|
||||||
|
t.Fatalf("len(reviews) = %d, want 2", len(reviews))
|
||||||
|
}
|
||||||
|
if reviews[0].ID != 1 || reviews[0].User.Login != "alice" {
|
||||||
|
t.Errorf("reviews[0] = %+v, unexpected", reviews[0])
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestDeleteReview_Success(t *testing.T) {
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
if r.Method != http.MethodDelete {
|
||||||
|
t.Errorf("method = %s, want DELETE", r.Method)
|
||||||
|
}
|
||||||
|
if r.URL.Path != "/repos/owner/repo/pulls/1/reviews/42" {
|
||||||
|
t.Errorf("path = %s, unexpected", r.URL.Path)
|
||||||
|
}
|
||||||
|
w.WriteHeader(http.StatusNoContent)
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
err := c.DeleteReview(context.Background(), "owner", "repo", 1, 42)
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestDeleteReview_SubmittedReview(t *testing.T) {
|
||||||
|
// GitHub returns 422 for trying to delete a non-pending review
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
w.WriteHeader(http.StatusUnprocessableEntity)
|
||||||
|
w.Write([]byte(`{"message":"Can only delete a pending review"}`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
err := c.DeleteReview(context.Background(), "owner", "repo", 1, 99)
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected error, got nil")
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestGetAuthenticatedUser_Success(t *testing.T) {
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
if r.URL.Path != "/user" {
|
||||||
|
t.Errorf("path = %s, want /user", r.URL.Path)
|
||||||
|
}
|
||||||
|
w.WriteHeader(http.StatusOK)
|
||||||
|
w.Write([]byte(`{"login":"review-bot","id":12345}`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
login, err := c.GetAuthenticatedUser(context.Background())
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
if login != "review-bot" {
|
||||||
|
t.Errorf("login = %q, want review-bot", login)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestRequestReviewer_Success(t *testing.T) {
|
||||||
|
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
|
if r.Method != http.MethodPost {
|
||||||
|
t.Errorf("method = %s, want POST", r.Method)
|
||||||
|
}
|
||||||
|
if r.URL.Path != "/repos/owner/repo/pulls/1/requested_reviewers" {
|
||||||
|
t.Errorf("path = %s, unexpected", r.URL.Path)
|
||||||
|
}
|
||||||
|
var payload map[string]interface{}
|
||||||
|
if err := json.NewDecoder(r.Body).Decode(&payload); err != nil {
|
||||||
|
t.Errorf("decode body: %v", err)
|
||||||
|
}
|
||||||
|
reviewers, ok := payload["reviewers"].([]interface{})
|
||||||
|
if !ok || len(reviewers) != 1 || reviewers[0] != "reviewer1" {
|
||||||
|
t.Errorf("reviewers = %v, unexpected", payload["reviewers"])
|
||||||
|
}
|
||||||
|
w.WriteHeader(http.StatusCreated)
|
||||||
|
w.Write([]byte(`{}`))
|
||||||
|
}))
|
||||||
|
defer srv.Close()
|
||||||
|
|
||||||
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
|
err := c.RequestReviewer(context.Background(), "owner", "repo", 1, "reviewer1")
|
||||||
|
if err != nil {
|
||||||
|
t.Fatalf("unexpected error: %v", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestPostReview_RejectsHTTP(t *testing.T) {
|
||||||
|
// PostReview must reject http:// base URLs — tokens must not be sent in plaintext.
|
||||||
|
c := NewClient("tok", "http://127.0.0.1:1")
|
||||||
|
_, err := c.PostReview(context.Background(), "owner", "repo", 1, "APPROVE", "body", "", nil)
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected error for HTTP base URL in PostReview")
|
||||||
|
}
|
||||||
|
if !strings.Contains(err.Error(), "refusing HTTP request") {
|
||||||
|
t.Errorf("unexpected error message: %v", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestDeleteReview_RejectsHTTP(t *testing.T) {
|
||||||
|
// DeleteReview must reject http:// base URLs — tokens must not be sent in plaintext.
|
||||||
|
c := NewClient("tok", "http://127.0.0.1:1")
|
||||||
|
err := c.DeleteReview(context.Background(), "owner", "repo", 1, 42)
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected error for HTTP base URL in DeleteReview")
|
||||||
|
}
|
||||||
|
if !strings.Contains(err.Error(), "refusing HTTP request") {
|
||||||
|
t.Errorf("unexpected error message: %v", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestRequestReviewer_RejectsHTTP(t *testing.T) {
|
||||||
|
// RequestReviewer must reject http:// base URLs — tokens must not be sent in plaintext.
|
||||||
|
c := NewClient("tok", "http://127.0.0.1:1")
|
||||||
|
err := c.RequestReviewer(context.Background(), "owner", "repo", 1, "reviewer1")
|
||||||
|
if err == nil {
|
||||||
|
t.Fatal("expected error for HTTP base URL in RequestReviewer")
|
||||||
|
}
|
||||||
|
if !strings.Contains(err.Error(), "refusing HTTP request") {
|
||||||
|
t.Errorf("unexpected error message: %v", err)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
func TestEscapePath_SpecialChars(t *testing.T) {
|
||||||
|
tests := []struct {
|
||||||
|
input string
|
||||||
|
want string
|
||||||
|
}{
|
||||||
|
{"README.md", "README.md"},
|
||||||
|
{"docs/guide.md", "docs/guide.md"},
|
||||||
|
{"path with spaces/file.md", "path%20with%20spaces/file.md"},
|
||||||
|
{"path/with [brackets]/file.md", "path/with%20%5Bbrackets%5D/file.md"},
|
||||||
|
}
|
||||||
|
for _, tt := range tests {
|
||||||
|
got := escapePath(tt.input)
|
||||||
|
if got != tt.want {
|
||||||
|
t.Errorf("escapePath(%q) = %q, want %q", tt.input, got, tt.want)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
[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 thangithub.concur.comwon't be detected. The more robust approach would be to rely on the explicitVCS_TYPEenv var for all GHES instances, or detect GHES by checking for/api/v3in the URL, or check the GitHub API endpoint itself. The comment acknowledges the composite action setsVCS_TYPEexplicitly, but this hardcoded fallback will silently mis-route for other GHES deployments.