feat: self-request as reviewer before posting #41

Merged
rodin merged 2 commits from feat/35-self-request-reviewer into main 2026-05-02 19:11:15 +00:00
3 changed files with 147 additions and 0 deletions
+12
View File
@@ -344,6 +344,18 @@ func main() {
}
}
// Self-request as reviewer (ensures we appear in required-reviewer checks)
authUser, err := giteaClient.GetAuthenticatedUser(ctx)
if err != nil {
slog.Warn("could not determine authenticated user for reviewer self-request", "error", err)
} else if authUser != "" {
if err := giteaClient.RequestReviewer(ctx, owner, repoName, prNumber, authUser); err != nil {
slog.Warn("could not self-request as reviewer", "user", authUser, "error", err)
} else {
slog.Debug("self-requested as reviewer", "user", authUser, "pr", prNumber)
}
}
// POST new review
slog.Info("posting review", "event", event, "pr", prNumber)
posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)
+53
View File
@@ -460,3 +460,56 @@ func (c *Client) EditComment(ctx context.Context, owner, repo string, commentID
}
Review

[MINOR] New exported methods (GetAuthenticatedUser and RequestReviewer) were added without accompanying tests. Repository conventions state to test every exported function; adding unit tests would improve coverage and guard against API regressions.

**[MINOR]** New exported methods (GetAuthenticatedUser and RequestReviewer) were added without accompanying tests. Repository conventions state to test every exported function; adding unit tests would improve coverage and guard against API regressions.
Review

[MINOR] RequestReviewer treats only 200/201/204 as success. For robustness and future compatibility, consider accepting any 2xx status (or explicitly include 202 Accepted) to avoid false negatives if the server returns a different success code.

**[MINOR]** RequestReviewer treats only 200/201/204 as success. For robustness and future compatibility, consider accepting any 2xx status (or explicitly include 202 Accepted) to avoid false negatives if the server returns a different success code.
Review

[NIT] The comment states the RequestReviewer call is idempotent, but if the API returned a non-2xx for 'already requested' (e.g., 409/422), the method would currently return an error. Consider broadening accepted statuses or documenting expected server behavior more precisely.

**[NIT]** The comment states the RequestReviewer call is idempotent, but if the API returned a non-2xx for 'already requested' (e.g., 409/422), the method would currently return an error. Consider broadening accepted statuses or documenting expected server behavior more precisely.
return nil
}
// GetAuthenticatedUser returns the login of the user authenticated by the token.
func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) {
reqURL := fmt.Sprintf("%s/api/v1/user", c.baseURL)
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 the given 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/api/v1/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)
}
req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data))
if err != nil {
Review

[MINOR] On reviewer request failure, the full HTTP response body is included in the error message, which may leak internal server details into logs. Consider truncating or redacting the body before logging to reduce potential information exposure.

**[MINOR]** On reviewer request failure, the full HTTP response body is included in the error message, which may leak internal server details into logs. Consider truncating or redacting the body before logging to reduce potential information exposure.
return fmt.Errorf("create reviewer request: %w", err)
}
req.Header.Set("Authorization", "token "+c.token)
req.Header.Set("Content-Type", "application/json")
resp, err := c.http.Do(req)
if err != nil {
return fmt.Errorf("request reviewer: %w", err)
Review

[MINOR] RequestReviewer treats only 200 OK and 201 Created as success. Consider accepting 204 No Content as success as well, since some APIs return 204 for idempotent operations.

**[MINOR]** RequestReviewer treats only 200 OK and 201 Created as success. Consider accepting 204 No Content as success as well, since some APIs return 204 for idempotent operations.
}
defer resp.Body.Close()
if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent {
body, _ := io.ReadAll(io.LimitReader(resp.Body, 256))
return fmt.Errorf("request reviewer failed (status %d): %s", resp.StatusCode, body)
}
return nil
}
+82
View File
@@ -5,8 +5,10 @@ import (
"encoding/json"
"errors"
"fmt"
"io"
"net/http"
"net/http/httptest"
"strings"
"testing"
)
@@ -602,3 +604,83 @@ func TestIsNotFound(t *testing.T) {
})
}
}
func TestGetAuthenticatedUser(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path != "/api/v1/user" {
t.Errorf("unexpected path: %s", r.URL.Path)
http.NotFound(w, r)
return
}
if r.Header.Get("Authorization") != "token test-token" {
t.Error("missing or wrong auth header")
}
w.Header().Set("Content-Type", "application/json")
fmt.Fprint(w, `{"login":"my-bot","id":42}`)
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
login, err := client.GetAuthenticatedUser(context.Background())
if err != nil {
t.Fatalf("GetAuthenticatedUser() error = %v", err)
}
if login != "my-bot" {
t.Errorf("login = %q, want %q", login, "my-bot")
}
}
func TestRequestReviewer(t *testing.T) {
var gotBody []byte
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.Method != http.MethodPost {
t.Errorf("expected POST, got %s", r.Method)
}
expected := "/api/v1/repos/owner/repo/pulls/7/requested_reviewers"
if r.URL.Path != expected {
t.Errorf("path = %q, want %q", r.URL.Path, expected)
}
gotBody, _ = io.ReadAll(r.Body)
w.WriteHeader(http.StatusCreated)
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
err := client.RequestReviewer(context.Background(), "owner", "repo", 7, "bot-user")
if err != nil {
t.Fatalf("RequestReviewer() error = %v", err)
}
if !strings.Contains(string(gotBody), `"bot-user"`) {
t.Errorf("body = %s, want to contain bot-user", gotBody)
}
}
func TestRequestReviewer_204(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusNoContent)
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
err := client.RequestReviewer(context.Background(), "owner", "repo", 1, "user")
if err != nil {
t.Fatalf("RequestReviewer() should accept 204, got error = %v", err)
}
}
func TestRequestReviewer_Error(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
w.WriteHeader(http.StatusForbidden)
fmt.Fprint(w, "no permission")
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
err := client.RequestReviewer(context.Background(), "owner", "repo", 1, "user")
if err == nil {
t.Fatal("expected error for 403 response")
}
if !strings.Contains(err.Error(), "403") {
t.Errorf("error should mention status code: %v", err)
}
}