From 0d417e068e4ade5b598e9c5910ad5be25b3d88ed Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 20:17:01 -0700 Subject: [PATCH 1/9] feat: delete previous review before posting new one (#6) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before posting a review, the bot now: 1. Calls GET /api/v1/user to identify its own login 2. Lists all reviews on the PR 3. Deletes any existing reviews from itself 4. Posts the fresh review This keeps PR threads clean — one review per bot at any time. New Gitea client methods: - GetAuthenticatedUser() — token self-identification - ListReviews() — fetch reviews on a PR - DeleteReview() — delete a review by ID Flag: --update-existing / UPDATE_EXISTING (default true) Set to false to preserve old behavior (stack reviews). All delete failures are non-fatal (logged as warnings). Closes #6 --- .gitea/actions/review/action.yml | 5 ++ cmd/review-bot/main.go | 32 +++++++++++ gitea/client.go | 75 +++++++++++++++++++++++++ gitea/client_test.go | 96 ++++++++++++++++++++++++++++++++ 4 files changed, 208 insertions(+) diff --git a/.gitea/actions/review/action.yml b/.gitea/actions/review/action.yml index f52307c..d005697 100644 --- a/.gitea/actions/review/action.yml +++ b/.gitea/actions/review/action.yml @@ -66,6 +66,10 @@ inputs: description: 'Print review to stdout instead of posting' required: false default: 'false' + update-existing: + description: 'Delete previous review from same bot before posting (default true)' + required: false + default: 'true' runs: using: 'composite' @@ -145,6 +149,7 @@ runs: LLM_TEMPERATURE: ${{ inputs.temperature }} LLM_TIMEOUT: ${{ inputs.timeout }} LLM_PROVIDER: ${{ inputs.llm-provider }} + UPDATE_EXISTING: ${{ inputs.update-existing }} run: | ARGS="" if [ "${{ inputs.dry-run }}" = "true" ]; then diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 065afca..39865b4 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -33,6 +33,7 @@ func main() { patternsRepo := flag.String("patterns-repo", envOrDefault("PATTERNS_REPO", ""), "Repo with language patterns (e.g. rodin/elixir-patterns)") patternsFiles := flag.String("patterns-files", envOrDefault("PATTERNS_FILES", "README.md"), "Comma-separated file paths to fetch from patterns repo") dryRun := flag.Bool("dry-run", false, "Print review to stdout instead of posting") + updateExisting := flag.Bool("update-existing", envOrDefaultBool("UPDATE_EXISTING", true), "Delete previous review from same bot before posting (default true)") llmTemp := flag.Float64("llm-temperature", envOrDefaultFloat("LLM_TEMPERATURE", 0), "LLM temperature (0 = server default)") llmTimeout := flag.Int("llm-timeout", envOrDefaultInt("LLM_TIMEOUT", 300), "LLM request timeout in seconds (default 300)") llmProvider := flag.String("llm-provider", envOrDefault("LLM_PROVIDER", "openai"), "LLM API provider: openai or anthropic") @@ -195,6 +196,29 @@ func main() { return } + // Delete previous reviews from this bot if update-existing is enabled + if *updateExisting { + login, err := giteaClient.GetAuthenticatedUser(ctx) + if err != nil { + log.Printf("Warning: could not determine reviewer identity: %v", err) + } else { + reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) + if err != nil { + log.Printf("Warning: could not list existing reviews: %v", err) + } else { + for _, r := range reviews { + if r.User.Login == login { + if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, r.ID); err != nil { + log.Printf("Warning: could not delete old review %d: %v", r.ID, err) + } else { + log.Printf("Deleted previous review %d", r.ID) + } + } + } + } + } + } + log.Printf("Posting review (event=%s)...", event) if err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody); err != nil { log.Fatalf("Failed to post review: %v", err) @@ -333,3 +357,11 @@ func envOrDefaultInt(key string, defaultVal int) int { } return defaultVal } + +func envOrDefaultBool(key string, defaultVal bool) bool { + v := os.Getenv(key) + if v == "" { + return defaultVal + } + return v == "true" || v == "1" || v == "yes" +} diff --git a/gitea/client.go b/gitea/client.go index 73a4abf..f21b1d5 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -266,3 +266,78 @@ func (c *Client) GetAllFilesInPath(ctx context.Context, owner, repo, path string } return results, nil } + +// Review represents a pull request review from the Gitea API. +type Review struct { + ID int64 `json:"id"` + User struct { + Login string `json:"login"` + } `json:"user"` + State string `json:"state"` + Stale bool `json:"stale"` +} + +// GetAuthenticatedUser returns the login name of the token's owner. +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 user struct { + Login string `json:"login"` + } + if err := json.Unmarshal(body, &user); err != nil { + return "", fmt.Errorf("parse user response: %w", err) + } + if user.Login == "" { + return "", fmt.Errorf("empty login in user response") + } + return user.Login, nil +} + +// ListReviews returns all reviews on a pull request. +func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error) { + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", + c.baseURL, + url.PathEscape(owner), + url.PathEscape(repo), + number) + body, err := c.doGet(ctx, reqURL) + if err != nil { + return nil, fmt.Errorf("list reviews: %w", err) + } + var reviews []Review + if err := json.Unmarshal(body, &reviews); err != nil { + return nil, fmt.Errorf("parse reviews: %w", err) + } + return reviews, nil +} + +// DeleteReview deletes a review by ID. The token must belong to the review author. +func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error { + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews/%d", + c.baseURL, + url.PathEscape(owner), + url.PathEscape(repo), + number, + reviewID) + + req, err := http.NewRequestWithContext(ctx, http.MethodDelete, reqURL, nil) + if err != nil { + return fmt.Errorf("create delete request: %w", err) + } + req.Header.Set("Authorization", "token "+c.token) + + resp, err := c.http.Do(req) + if err != nil { + return fmt.Errorf("delete review: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + respBody, _ := io.ReadAll(resp.Body) + return fmt.Errorf("delete review failed (status %d): %s", resp.StatusCode, string(respBody)) + } + return nil +} diff --git a/gitea/client_test.go b/gitea/client_test.go index 24f60f8..fc93473 100644 --- a/gitea/client_test.go +++ b/gitea/client_test.go @@ -318,3 +318,99 @@ func TestEscapePath(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) + } + if r.Method != "GET" { + t.Errorf("expected GET, got %s", r.Method) + } + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"login":"review-bot","id":42}`)) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + login, err := client.GetAuthenticatedUser(context.Background()) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if login != "review-bot" { + t.Errorf("expected login %q, got %q", "review-bot", login) + } +} + +func TestGetAuthenticatedUser_EmptyLogin(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`{"login":"","id":0}`)) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + _, err := client.GetAuthenticatedUser(context.Background()) + if err == nil { + t.Fatal("expected error for empty login, got nil") + } +} + +func TestListReviews(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v1/repos/owner/repo/pulls/5/reviews" { + t.Errorf("unexpected path: %s", r.URL.Path) + } + w.Header().Set("Content-Type", "application/json") + w.Write([]byte(`[{"id":10,"user":{"login":"bot-a"},"state":"APPROVED","stale":false},{"id":11,"user":{"login":"bot-b"},"state":"REQUEST_CHANGES","stale":true}]`)) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + reviews, err := client.ListReviews(context.Background(), "owner", "repo", 5) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reviews) != 2 { + t.Fatalf("expected 2 reviews, got %d", len(reviews)) + } + if reviews[0].User.Login != "bot-a" { + t.Errorf("expected bot-a, got %s", reviews[0].User.Login) + } + if reviews[1].ID != 11 { + t.Errorf("expected id 11, got %d", reviews[1].ID) + } +} + +func TestDeleteReview(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v1/repos/owner/repo/pulls/5/reviews/10" { + t.Errorf("unexpected path: %s", r.URL.Path) + } + if r.Method != "DELETE" { + t.Errorf("expected DELETE, got %s", r.Method) + } + w.WriteHeader(http.StatusNoContent) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + err := client.DeleteReview(context.Background(), "owner", "repo", 5, 10) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestDeleteReview_Forbidden(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + w.Write([]byte(`{"message":"forbidden"}`)) + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + err := client.DeleteReview(context.Background(), "owner", "repo", 5, 10) + if err == nil { + t.Fatal("expected error for 403, got nil") + } +} -- 2.47.3 From 41c670b44b774f110adb321f47980e3b96a8cc2c Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 20:22:03 -0700 Subject: [PATCH 2/9] fix: post-then-cleanup flow, remove dead code, pagination - PostReview now returns *Review (id + user login from response) - Delete flow: post first, then delete stale reviews by same user - No read:user scope needed (identity from POST response) - Removed GetAuthenticatedUser (requires scope we lack) - ListReviews: full pagination (loops until partial page) - envOrDefaultBool: case-insensitive, whitespace-trimmed - action.yml: document accepted boolean values - Tests updated for new PostReview signature --- .gitea/actions/review/action.yml | 2 +- cmd/review-bot/main.go | 40 ++++++------- gitea/client.go | 79 +++++++++++++------------- gitea/client_test.go | 96 ++++++++++++++++++-------------- 4 files changed, 113 insertions(+), 104 deletions(-) diff --git a/.gitea/actions/review/action.yml b/.gitea/actions/review/action.yml index d005697..0fdccdc 100644 --- a/.gitea/actions/review/action.yml +++ b/.gitea/actions/review/action.yml @@ -67,7 +67,7 @@ inputs: required: false default: 'false' update-existing: - description: 'Delete previous review from same bot before posting (default true)' + description: 'Delete previous review from same bot before posting. Accepts: true/1/yes or false/0/no (default true)' required: false default: 'true' diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 39865b4..098a2da 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -196,34 +196,30 @@ func main() { return } - // Delete previous reviews from this bot if update-existing is enabled - if *updateExisting { - login, err := giteaClient.GetAuthenticatedUser(ctx) + log.Printf("Posting review (event=%s)...", event) + posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody) + if err != nil { + log.Fatalf("Failed to post review: %v", err) + } + log.Printf("Review posted (id=%d, user=%s)", posted.ID, posted.User.Login) + + // Delete stale reviews from this bot if update-existing is enabled + if *updateExisting && posted.User.Login != "" { + reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) if err != nil { - log.Printf("Warning: could not determine reviewer identity: %v", err) + log.Printf("Warning: could not list existing reviews: %v", err) } else { - reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) - if err != nil { - log.Printf("Warning: could not list existing reviews: %v", err) - } else { - for _, r := range reviews { - if r.User.Login == login { - if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, r.ID); err != nil { - log.Printf("Warning: could not delete old review %d: %v", r.ID, err) - } else { - log.Printf("Deleted previous review %d", r.ID) - } + for _, r := range reviews { + if r.User.Login == posted.User.Login && r.ID != posted.ID { + if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, r.ID); err != nil { + log.Printf("Warning: could not delete old review %d: %v", r.ID, err) + } else { + log.Printf("Deleted stale review %d", r.ID) } } } } } - - log.Printf("Posting review (event=%s)...", event) - if err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody); err != nil { - log.Fatalf("Failed to post review: %v", err) - } - log.Printf("Review posted successfully!") } // fetchFileContext fetches the full content of modified files from the PR branch. @@ -359,7 +355,7 @@ func envOrDefaultInt(key string, defaultVal int) int { } func envOrDefaultBool(key string, defaultVal bool) bool { - v := os.Getenv(key) + v := strings.TrimSpace(strings.ToLower(os.Getenv(key))) if v == "" { return defaultVal } diff --git a/gitea/client.go b/gitea/client.go index f21b1d5..75bd096 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -129,9 +129,9 @@ func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, r return string(body), nil } -// PostReview submits a review to a PR. +// PostReview submits a review to a PR and returns the created review. // event should be "APPROVED" or "REQUEST_CHANGES". -func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string) error { +func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string) (*Review, error) { reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, owner, repo, number) payload := struct { @@ -144,27 +144,36 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, data, err := json.Marshal(payload) if err != nil { - return fmt.Errorf("marshal review payload: %w", err) + return nil, fmt.Errorf("marshal review payload: %w", err) } req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data)) if err != nil { - return fmt.Errorf("create review request: %w", err) + return nil, fmt.Errorf("create review 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("post review: %w", err) + return nil, fmt.Errorf("post review: %w", err) } defer resp.Body.Close() if resp.StatusCode < 200 || resp.StatusCode >= 300 { respBody, _ := io.ReadAll(resp.Body) - return fmt.Errorf("post review failed (status %d): %s", resp.StatusCode, string(respBody)) + return nil, fmt.Errorf("post review failed (status %d): %s", resp.StatusCode, string(respBody)) } - return nil + + respBody, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("read review response: %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 } func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) { @@ -277,41 +286,33 @@ type Review struct { Stale bool `json:"stale"` } -// GetAuthenticatedUser returns the login name of the token's owner. -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 user struct { - Login string `json:"login"` - } - if err := json.Unmarshal(body, &user); err != nil { - return "", fmt.Errorf("parse user response: %w", err) - } - if user.Login == "" { - return "", fmt.Errorf("empty login in user response") - } - return user.Login, nil -} - // ListReviews returns all reviews on a pull request. +// Paginates through all pages to ensure no reviews are missed. func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error) { - reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", - c.baseURL, - url.PathEscape(owner), - url.PathEscape(repo), - number) - body, err := c.doGet(ctx, reqURL) - if err != nil { - return nil, fmt.Errorf("list reviews: %w", err) + const pageSize = 50 + var all []Review + for page := 1; ; page++ { + reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews?limit=%d&page=%d", + c.baseURL, + url.PathEscape(owner), + url.PathEscape(repo), + number, + pageSize, + 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) < pageSize { + break + } } - var reviews []Review - if err := json.Unmarshal(body, &reviews); err != nil { - return nil, fmt.Errorf("parse reviews: %w", err) - } - return reviews, nil + return all, nil } // DeleteReview deletes a review by ID. The token must belong to the review author. diff --git a/gitea/client_test.go b/gitea/client_test.go index fc93473..eabdd9e 100644 --- a/gitea/client_test.go +++ b/gitea/client_test.go @@ -123,15 +123,21 @@ func TestPostReview(t *testing.T) { } w.WriteHeader(http.StatusOK) - w.Write([]byte(`{}`)) + w.Write([]byte(`{"id":100,"user":{"login":"review-bot"},"state":"APPROVED","stale":false}`)) })) defer server.Close() client := NewClient(server.URL, "test-token") - err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM") + review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM") if err != nil { t.Fatalf("unexpected error: %v", err) } + if review.ID != 100 { + t.Errorf("expected review ID 100, got %d", review.ID) + } + if review.User.Login != "review-bot" { + t.Errorf("expected user login %q, got %q", "review-bot", review.User.Login) + } } func TestGetPullRequest_Non200(t *testing.T) { @@ -169,7 +175,7 @@ func TestPostReview_Non200(t *testing.T) { defer server.Close() client := NewClient(server.URL, "test-token") - err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test") + _, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test") if err == nil { t.Fatal("expected error for 403, got nil") } @@ -319,49 +325,18 @@ func TestEscapePath(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) - } - if r.Method != "GET" { - t.Errorf("expected GET, got %s", r.Method) - } - w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"login":"review-bot","id":42}`)) - })) - defer server.Close() - - client := NewClient(server.URL, "test-token") - login, err := client.GetAuthenticatedUser(context.Background()) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if login != "review-bot" { - t.Errorf("expected login %q, got %q", "review-bot", login) - } -} - -func TestGetAuthenticatedUser_EmptyLogin(t *testing.T) { - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - w.Header().Set("Content-Type", "application/json") - w.Write([]byte(`{"login":"","id":0}`)) - })) - defer server.Close() - - client := NewClient(server.URL, "test-token") - _, err := client.GetAuthenticatedUser(context.Background()) - if err == nil { - t.Fatal("expected error for empty login, got nil") - } -} - func TestListReviews(t *testing.T) { + pageCount := 0 server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path != "/api/v1/repos/owner/repo/pulls/5/reviews" { t.Errorf("unexpected path: %s", r.URL.Path) } + if r.URL.Query().Get("limit") != "50" { + t.Errorf("expected limit=50, got %s", r.URL.Query().Get("limit")) + } + pageCount++ w.Header().Set("Content-Type", "application/json") + // Return 2 results (less than page size) to signal end w.Write([]byte(`[{"id":10,"user":{"login":"bot-a"},"state":"APPROVED","stale":false},{"id":11,"user":{"login":"bot-b"},"state":"REQUEST_CHANGES","stale":true}]`)) })) defer server.Close() @@ -377,8 +352,45 @@ func TestListReviews(t *testing.T) { if reviews[0].User.Login != "bot-a" { t.Errorf("expected bot-a, got %s", reviews[0].User.Login) } - if reviews[1].ID != 11 { - t.Errorf("expected id 11, got %d", reviews[1].ID) + if pageCount != 1 { + t.Errorf("expected 1 page fetch (results < page size), got %d", pageCount) + } +} + +func TestListReviews_Pagination(t *testing.T) { + pageCount := 0 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + pageCount++ + page := r.URL.Query().Get("page") + w.Header().Set("Content-Type", "application/json") + if page == "1" { + // Return exactly 50 items to trigger next page fetch + items := "[" + for i := 0; i < 50; i++ { + if i > 0 { + items += "," + } + items += fmt.Sprintf(`{"id":%d,"user":{"login":"bot"},"state":"APPROVED","stale":false}`, i+1) + } + items += "]" + w.Write([]byte(items)) + } else { + // Page 2: return fewer than 50 to signal end + w.Write([]byte(`[{"id":51,"user":{"login":"bot"},"state":"APPROVED","stale":false}]`)) + } + })) + defer server.Close() + + client := NewClient(server.URL, "test-token") + reviews, err := client.ListReviews(context.Background(), "owner", "repo", 5) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(reviews) != 51 { + t.Fatalf("expected 51 reviews across 2 pages, got %d", len(reviews)) + } + if pageCount != 2 { + t.Errorf("expected 2 page fetches, got %d", pageCount) } } -- 2.47.3 From 69e0a459c3b12de34151f4a97d06518c76fe8d46 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 20:55:09 -0700 Subject: [PATCH 3/9] feat: sentinel-based review cleanup + system prompt file + security review Sentinel-based cleanup: - Reviews embed in body (hidden HTML comment) - Cleanup matches by sentinel, not token identity - Each reviewer-name is a logical identity (sonnet, gpt, security) - Same token can run multiple review types without conflict - No extra API scopes needed System prompt file (--system-prompt-file / SYSTEM_PROMPT_FILE): - Loads a local file with additional review instructions - Appended to system base as "Additional Review Instructions" - Enables specialized reviews (security, performance, etc.) - Partially addresses #5 Security review: - SECURITY_REVIEW.md prompt focused on vulnerabilities - 3rd CI matrix entry using same token, different prompt - Focus: injection, auth, secrets, input validation, crypto, races CI changes: - REVIEWER_NAME passed from matrix.name - SYSTEM_PROMPT_FILE passed from matrix (empty for standard reviews) - 3 reviewers: sonnet (general), gpt (general), security (focused) --- .gitea/actions/review/action.yml | 5 +++++ .gitea/workflows/ci.yml | 6 ++++++ SECURITY_REVIEW.md | 18 ++++++++++++++++++ cmd/review-bot/main.go | 25 +++++++++++++++++++++---- gitea/client.go | 1 + review/formatter.go | 2 ++ review/formatter_test.go | 18 ++++++++++++++++++ 7 files changed, 71 insertions(+), 4 deletions(-) create mode 100644 SECURITY_REVIEW.md diff --git a/.gitea/actions/review/action.yml b/.gitea/actions/review/action.yml index 0fdccdc..912c7da 100644 --- a/.gitea/actions/review/action.yml +++ b/.gitea/actions/review/action.yml @@ -70,6 +70,10 @@ inputs: description: 'Delete previous review from same bot before posting. Accepts: true/1/yes or false/0/no (default true)' required: false default: 'true' + system-prompt-file: + description: 'Local file with additional system prompt instructions (e.g. security review focus)' + required: false + default: '' runs: using: 'composite' @@ -150,6 +154,7 @@ runs: LLM_TIMEOUT: ${{ inputs.timeout }} LLM_PROVIDER: ${{ inputs.llm-provider }} UPDATE_EXISTING: ${{ inputs.update-existing }} + SYSTEM_PROMPT_FILE: ${{ inputs.system-prompt-file }} run: | ARGS="" if [ "${{ inputs.dry-run }}" = "true" ]; then diff --git a/.gitea/workflows/ci.yml b/.gitea/workflows/ci.yml index 4ada9a6..b429c56 100644 --- a/.gitea/workflows/ci.yml +++ b/.gitea/workflows/ci.yml @@ -32,6 +32,10 @@ jobs: - name: gpt token_secret: GPT_REVIEW_TOKEN model: gpt-4.1 + - name: security + token_secret: SONNET_REVIEW_TOKEN + model: gpt-5 + system_prompt_file: SECURITY_REVIEW.md steps: - uses: actions/checkout@v4 - uses: actions/setup-go@v5 @@ -44,6 +48,7 @@ jobs: GITEA_REPO: ${{ github.repository }} PR_NUMBER: ${{ github.event.pull_request.number }} REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }} + REVIEWER_NAME: ${{ matrix.name }} LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }} LLM_API_KEY: ${{ secrets.LLM_API_KEY }} LLM_MODEL: ${{ matrix.model }} @@ -51,4 +56,5 @@ jobs: PATTERNS_REPO: "rodin/go-patterns" PATTERNS_FILES: "README.md,patterns/" LLM_TIMEOUT: "600" + SYSTEM_PROMPT_FILE: ${{ matrix.system_prompt_file }} run: ./review-bot diff --git a/SECURITY_REVIEW.md b/SECURITY_REVIEW.md new file mode 100644 index 0000000..4b2566f --- /dev/null +++ b/SECURITY_REVIEW.md @@ -0,0 +1,18 @@ +You are performing a security-focused code review. Your primary concern is identifying vulnerabilities, not general code quality. + +Focus areas: +- **Injection attacks**: SQL injection, command injection, path traversal, template injection +- **Authentication/Authorization**: Missing auth checks, privilege escalation, IDOR +- **Secrets exposure**: Hardcoded credentials, API keys in code, tokens in logs +- **Input validation**: Untrusted input used without sanitization, unsafe deserialization +- **Cryptography**: Weak algorithms, predictable randomness, improper key management +- **Error handling**: Information leakage in error messages, stack traces exposed +- **Dependencies**: Known vulnerable patterns, unsafe use of external libraries +- **Race conditions**: TOCTOU bugs, unsynchronized shared state +- **Resource exhaustion**: Unbounded allocations, missing timeouts, denial-of-service vectors + +Rules for this review: +- Only report findings with actual security implications. Ignore style, naming, and general code quality. +- Severity mapping: MAJOR = exploitable vulnerability or data exposure. MINOR = defense-in-depth improvement or hardening opportunity. NIT = theoretical concern with low practical risk. +- If the code has no security-relevant changes, APPROVE with an empty findings list. +- Do not duplicate findings that a standard code review would catch (logic bugs, missing error checks) unless they have a security dimension. diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 098a2da..a99270e 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -30,6 +30,7 @@ func main() { llmAPIKey := flag.String("llm-api-key", envOrDefault("LLM_API_KEY", ""), "LLM API key") llmModel := flag.String("llm-model", envOrDefault("LLM_MODEL", ""), "LLM model name") conventionsFile := flag.String("conventions-file", envOrDefault("CONVENTIONS_FILE", ""), "Conventions file path in repo (e.g. CLAUDE.md)") + systemPromptFile := flag.String("system-prompt-file", envOrDefault("SYSTEM_PROMPT_FILE", ""), "Local file with additional system prompt instructions") patternsRepo := flag.String("patterns-repo", envOrDefault("PATTERNS_REPO", ""), "Repo with language patterns (e.g. rodin/elixir-patterns)") patternsFiles := flag.String("patterns-files", envOrDefault("PATTERNS_FILES", "README.md"), "Comma-separated file paths to fetch from patterns repo") dryRun := flag.Bool("dry-run", false, "Print review to stdout instead of posting") @@ -150,9 +151,24 @@ func main() { log.Printf("Loaded patterns from %s (%d bytes)", *patternsRepo, len(patterns)) } + // Step 6b: Load additional system prompt if specified + additionalPrompt := "" + if *systemPromptFile != "" { + data, err := os.ReadFile(*systemPromptFile) + if err != nil { + log.Fatalf("Failed to read system prompt file %q: %v", *systemPromptFile, err) + } + additionalPrompt = string(data) + log.Printf("Loaded system prompt file: %s (%d bytes)", *systemPromptFile, len(additionalPrompt)) + } + // Step 7: Budget-aware prompt assembly + systemBase := review.BuildSystemBase() + if additionalPrompt != "" { + systemBase += "\n\n## Additional Review Instructions\n\n" + additionalPrompt + } sections := budget.Sections{ - SystemBase: review.BuildSystemBase(), + SystemBase: systemBase, Patterns: patterns, Conventions: conventions, FileContext: fileContext, @@ -203,14 +219,15 @@ func main() { } log.Printf("Review posted (id=%d, user=%s)", posted.ID, posted.User.Login) - // Delete stale reviews from this bot if update-existing is enabled - if *updateExisting && posted.User.Login != "" { + // Delete stale reviews from this bot using sentinel matching + sentinel := fmt.Sprintf("", *reviewerName) + if *updateExisting && *reviewerName != "" { reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) if err != nil { log.Printf("Warning: could not list existing reviews: %v", err) } else { for _, r := range reviews { - if r.User.Login == posted.User.Login && r.ID != posted.ID { + if r.ID != posted.ID && strings.Contains(r.Body, sentinel) { if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, r.ID); err != nil { log.Printf("Warning: could not delete old review %d: %v", r.ID, err) } else { diff --git a/gitea/client.go b/gitea/client.go index 75bd096..1b4a472 100644 --- a/gitea/client.go +++ b/gitea/client.go @@ -279,6 +279,7 @@ func (c *Client) GetAllFilesInPath(ctx context.Context, owner, repo, path string // Review represents a pull request review from the Gitea API. type Review struct { ID int64 `json:"id"` + Body string `json:"body"` User struct { Login string `json:"login"` } `json:"user"` diff --git a/review/formatter.go b/review/formatter.go index 4048b58..f0af56d 100644 --- a/review/formatter.go +++ b/review/formatter.go @@ -30,6 +30,8 @@ func FormatMarkdown(result *ReviewResult, reviewerName string) string { if reviewerName != "" { sb.WriteString(fmt.Sprintf("\n---\n*Review by %s*\n", reviewerName)) + // Hidden sentinel for identifying this bot's reviews during cleanup + sb.WriteString(fmt.Sprintf("\n\n", reviewerName)) } return sb.String() diff --git a/review/formatter_test.go b/review/formatter_test.go index f15dc2c..dbc95db 100644 --- a/review/formatter_test.go +++ b/review/formatter_test.go @@ -116,3 +116,21 @@ func TestGiteaEvent(t *testing.T) { } } } + +func TestFormatMarkdown_Sentinel(t *testing.T) { + result := &ReviewResult{ + Verdict: "APPROVE", + Summary: "All good.", + Recommendation: "Merge it.", + } + output := FormatMarkdown(result, "security") + if !strings.Contains(output, "") { + t.Error("expected sentinel comment in output") + } + + // Empty reviewer name should NOT have sentinel + output2 := FormatMarkdown(result, "") + if strings.Contains(output2, "` ensures the security review only replaces previous security reviews, never the code-quality or performance reviews. + +### With language patterns from another repo + +```yaml +- uses: https://gitea.weiker.me/rodin/review-bot/.gitea/actions/review@v0.1.0 + with: + reviewer-token: ${{ secrets.REVIEW_TOKEN }} + reviewer-name: reviewer + llm-base-url: ${{ secrets.LLM_BASE_URL }} + llm-api-key: ${{ secrets.LLM_API_KEY }} + llm-model: gpt-4.1 + conventions-file: CLAUDE.md + patterns-repo: rodin/go-patterns,rodin/kubernetes-conventions + patterns-files: "README.md,patterns/" +``` + +Pattern repos are fetched at review time. The reviewer uses them as criteria for idiomatic code. + +### Dry run (test without posting) + +```yaml +- uses: https://gitea.weiker.me/rodin/review-bot/.gitea/actions/review@v0.1.0 + with: + reviewer-token: ${{ secrets.REVIEW_TOKEN }} + reviewer-name: test + llm-base-url: ${{ secrets.LLM_BASE_URL }} + llm-api-key: ${{ secrets.LLM_API_KEY }} + llm-model: gpt-4.1 + dry-run: 'true' +``` + +Prints the review to CI logs without posting to the PR. Useful for testing prompt changes. + +### Using Anthropic directly + +```yaml +- uses: https://gitea.weiker.me/rodin/review-bot/.gitea/actions/review@v0.1.0 + with: + reviewer-token: ${{ secrets.REVIEW_TOKEN }} + reviewer-name: claude + llm-base-url: https://api.anthropic.com + llm-api-key: ${{ secrets.ANTHROPIC_API_KEY }} + llm-model: claude-sonnet-4-20250514 + llm-provider: anthropic +``` + +## Action Inputs + +| Input | Required | Default | Description | +|-------|----------|---------|-------------| +| `reviewer-token` | Yes | — | Gitea token for posting reviews (needs `write:issue`, `write:repository`) | +| `reviewer-name` | No | `""` | Logical identity for this reviewer. Used as sentinel for idempotent cleanup. Set this when running multiple review bots on the same PR. | +| `llm-base-url` | Yes | — | LLM API base URL | +| `llm-api-key` | Yes | — | LLM API key | +| `llm-model` | Yes | — | Model name | +| `llm-provider` | No | `openai` | API provider: `openai` or `anthropic` | +| `conventions-file` | No | `""` | Path to coding conventions file in the repo | +| `patterns-repo` | No | `""` | Comma-separated repos with language patterns (e.g. `rodin/go-patterns`) | +| `patterns-files` | No | `README.md` | Files/directories to fetch from pattern repos | +| `system-prompt-file` | No | `""` | Local file with additional system prompt instructions | +| `temperature` | No | `0` | LLM temperature (0 = server default) | +| `timeout` | No | `300` | LLM request timeout in seconds | +| `dry-run` | No | `false` | Print review to stdout instead of posting | +| `update-existing` | No | `true` | Delete previous review from same bot before posting. Accepts: true/1/yes or false/0/no | +| `version` | No | `latest` | review-bot version to install | + +## How Review Cleanup Works + +When `reviewer-name` is set, the bot embeds a hidden sentinel in each review: + +```html + +``` + +On the next run, it finds and deletes any review containing its own sentinel (except the one it just posted). This means: + +- **One review per bot per PR** — no clutter from repeated pushes +- **Multiple bots coexist** — each only cleans up its own reviews +- **Same token, different roles** — a single bot account can post "code-review" and "security" reviews without conflict +- **No extra permissions** — identity comes from the sentinel, not the API + +If `reviewer-name` is empty, cleanup is skipped (reviews stack like before). + +## Custom Review Prompts + +Use `system-prompt-file` to specialize the review focus. The file contents are appended to the base system prompt as "Additional Review Instructions." + +Example `SECURITY_REVIEW.md`: + +```markdown +You are performing a security-focused code review. + +Focus areas: +- Injection attacks (SQL, command, path traversal, template) +- Authentication/Authorization (missing checks, privilege escalation) +- Secrets exposure (hardcoded credentials, tokens in logs) +- Input validation (unsanitized input, unsafe deserialization) +- Race conditions (TOCTOU, unsynchronized shared state) + +Rules: +- Only report findings with security implications +- Ignore style, naming, and general code quality +- MAJOR = exploitable vulnerability, MINOR = hardening opportunity, NIT = theoretical risk +- If no security-relevant changes exist, APPROVE with empty findings +``` + +## CLI Usage ```bash review-bot \ @@ -19,71 +236,74 @@ review-bot \ --repo owner/name \ --pr 42 \ --reviewer-token "$GITEA_TOKEN" \ + --reviewer-name "code-review" \ --llm-base-url https://api.openai.com/v1 \ --llm-api-key "$OPENAI_API_KEY" \ - --llm-model gpt-4 \ - --reviewer-name "Sonnet" \ - --conventions-file CONVENTIONS.md \ - --dry-run + --llm-model gpt-4.1 \ + --conventions-file CONVENTIONS.md ``` ## Environment Variables -All flags can be set via environment variables: +All flags have environment variable equivalents: -| Flag | Env Var | Required | Description | -|------|---------|----------|-------------| -| `--gitea-url` | `GITEA_URL` | Yes | Gitea instance base URL | -| `--repo` | `GITEA_REPO` | Yes | Repository in `owner/name` format | -| `--pr` | `PR_NUMBER` | Yes | Pull request number | -| `--reviewer-token` | `REVIEWER_TOKEN` | Yes | Gitea API token for posting reviews | -| `--llm-base-url` | `LLM_BASE_URL` | Yes | OpenAI-compatible API base URL | -| `--llm-api-key` | `LLM_API_KEY` | Yes | LLM API key | -| `--llm-model` | `LLM_MODEL` | Yes | Model identifier | -| `--reviewer-name` | `REVIEWER_NAME` | No | Display name in review footer | -| `--conventions-file` | `CONVENTIONS_FILE` | No | Path to conventions file in repo | -| `--dry-run` | — | No | Print review to stdout instead of posting | +| Flag | Env Var | +|------|---------| +| `--gitea-url` | `GITEA_URL` | +| `--repo` | `GITEA_REPO` | +| `--pr` | `PR_NUMBER` | +| `--reviewer-token` | `REVIEWER_TOKEN` | +| `--reviewer-name` | `REVIEWER_NAME` | +| `--llm-base-url` | `LLM_BASE_URL` | +| `--llm-api-key` | `LLM_API_KEY` | +| `--llm-model` | `LLM_MODEL` | +| `--llm-provider` | `LLM_PROVIDER` | +| `--conventions-file` | `CONVENTIONS_FILE` | +| `--patterns-repo` | `PATTERNS_REPO` | +| `--patterns-files` | `PATTERNS_FILES` | +| `--system-prompt-file` | `SYSTEM_PROMPT_FILE` | +| `--llm-temperature` | `LLM_TEMPERATURE` | +| `--llm-timeout` | `LLM_TIMEOUT` | +| `--update-existing` | `UPDATE_EXISTING` | -## Adding to a Gitea Repository +## Setup -1. Build the binary or use the CI workflow approach (build in CI). +1. **Create a Gitea bot account** (e.g. `review-bot`) +2. **Generate a token** with scopes: `write:issue`, `write:repository` +3. **Add secrets** to your Gitea repo (Settings → Actions → Secrets): + - `REVIEW_TOKEN` — the bot's Gitea token + - `LLM_BASE_URL` — your LLM endpoint + - `LLM_API_KEY` — your LLM key +4. **Add the workflow** (see Quick Start above) -2. Add secrets to your Gitea repo (Settings → Actions → Secrets): - - `SONNET_REVIEW_TOKEN` — Gitea token for the Sonnet reviewer account - - `GPT_REVIEW_TOKEN` — Gitea token for the GPT reviewer account - - `LLM_BASE_URL` — Your LLM API endpoint - - `LLM_API_KEY` — Your LLM API key +### Token Scopes Required -3. Copy `.gitea/workflows/ci.yml` to your repo (or adapt it). +| Scope | Purpose | +|-------|---------| +| `write:issue` | Post and delete reviews | +| `write:repository` | Read PR diffs, file content, commit statuses | -4. On every PR, the bot will: - - Run tests and vet - - Build review-bot - - Post reviews from each configured LLM reviewer +No `read:user` scope needed — the bot identifies itself from the review response. ## Development ```bash -# Run tests -go test ./... - -# Run vet -go vet ./... - -# Build +go test ./... # Unit tests +go vet ./... # Static analysis go build -o review-bot ./cmd/review-bot -# Integration tests (requires env vars) +# Integration tests (requires env vars set) go test -tags=integration ./... ``` ## Architecture ``` -cmd/review-bot/ CLI entrypoint -gitea/ Gitea API client -llm/ OpenAI-compatible LLM client +cmd/review-bot/ CLI entrypoint + orchestration +gitea/ Gitea API client (reviews, PRs, files) +llm/ Multi-provider LLM client (OpenAI + Anthropic) review/ Prompt building, response parsing, formatting +budget/ Token estimation + context trimming ``` ## License -- 2.47.3 From 6a3c81327943916d7b8c15868273b9a549b0845d Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 21:03:41 -0700 Subject: [PATCH 5/9] fix: address review findings (path restriction, login cross-check, README) - system-prompt-file: reject absolute paths and paths containing ".." Prevents reading arbitrary files outside the workspace on shared runners. - Cleanup: cross-check r.User.Login == posted.User.Login before deletion Defense-in-depth: only attempt to delete reviews from same author. Flagged by both sonnet and security reviewers. - README: fix wording (cleanup happens after posting, not before) Issues filed for deferred work: - #24: Consistent url.PathEscape across all client endpoints - #25: Binary signature verification for supply-chain hardening --- README.md | 2 +- cmd/review-bot/main.go | 20 +++++++++++++++++--- 2 files changed, 18 insertions(+), 4 deletions(-) diff --git a/README.md b/README.md index 6512eec..ee43bd4 100644 --- a/README.md +++ b/README.md @@ -7,7 +7,7 @@ AI-powered code review bot for Gitea pull requests. Fetches diff + context, send - **Multi-provider**: OpenAI-compatible and Anthropic Messages API - **Context-aware**: Fetches full file content, conventions, language patterns, CI status - **Smart budget**: Automatically trims context to fit model token limits -- **Idempotent reviews**: Deletes previous review before posting new one (one review per bot) +- **Idempotent reviews**: Posts new review, then cleans up stale ones (one review per bot) - **Custom prompts**: Load additional instructions from a file (e.g. security-focused review) - **Zero dependencies**: Go stdlib only diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index a99270e..8684cac 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -6,6 +6,7 @@ import ( "fmt" "log" "os" + "path/filepath" "strconv" "strings" "time" @@ -154,9 +155,22 @@ func main() { // Step 6b: Load additional system prompt if specified additionalPrompt := "" if *systemPromptFile != "" { - data, err := os.ReadFile(*systemPromptFile) + workspace := os.Getenv("GITHUB_WORKSPACE") + if workspace == "" { + workspace, _ = os.Getwd() + } + absWorkspace, err := filepath.Abs(workspace) if err != nil { - log.Fatalf("Failed to read system prompt file %q: %v", *systemPromptFile, err) + log.Fatalf("Failed to resolve workspace path: %v", err) + } + promptPath := filepath.Join(absWorkspace, *systemPromptFile) + promptPath = filepath.Clean(promptPath) + if !strings.HasPrefix(promptPath, absWorkspace+string(filepath.Separator)) && promptPath != absWorkspace { + log.Fatalf("system-prompt-file resolves outside workspace (got %q, workspace %q)", promptPath, absWorkspace) + } + data, err := os.ReadFile(promptPath) + if err != nil { + log.Fatalf("Failed to read system prompt file %q: %v", promptPath, err) } additionalPrompt = string(data) log.Printf("Loaded system prompt file: %s (%d bytes)", *systemPromptFile, len(additionalPrompt)) @@ -227,7 +241,7 @@ func main() { log.Printf("Warning: could not list existing reviews: %v", err) } else { for _, r := range reviews { - if r.ID != posted.ID && strings.Contains(r.Body, sentinel) { + if r.ID != posted.ID && r.User.Login == posted.User.Login && strings.Contains(r.Body, sentinel) { if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, r.ID); err != nil { log.Printf("Warning: could not delete old review %d: %v", r.ID, err) } else { -- 2.47.3 From 687005d982bd3b8845d43d4c8a4b62ef936f0f3d Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 21:10:39 -0700 Subject: [PATCH 6/9] feat: worst-wins reconciliation for shared-token review types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When multiple review types share a Gitea bot account, Gitea uses the latest review to determine the user's approval state. This creates a race: if security finds issues but code-quality finishes last with APPROVE, the PR appears approved. Now before posting, each job checks if any sibling review from the same user has REQUEST_CHANGES. If so and we would post APPROVE, we downgrade to COMMENT instead — the review is still visible but won't override the blocking state. Documented in README under "Shared Token: Worst-Wins." --- README.md | 8 ++++++++ cmd/review-bot/main.go | 21 +++++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/README.md b/README.md index ee43bd4..3822a3e 100644 --- a/README.md +++ b/README.md @@ -205,6 +205,14 @@ On the next run, it finds and deletes any review containing its own sentinel (ex If `reviewer-name` is empty, cleanup is skipped (reviews stack like before). +### Shared Token: Worst-Wins Behavior + +When multiple review types share the same Gitea bot account (e.g. code-quality and security), Gitea determines the user's approval state from their **most recent review**. This creates a race condition: if security finds issues (REQUEST_CHANGES) but code-quality finishes last (APPROVE), the PR appears approved. + +review-bot handles this automatically with **worst-wins reconciliation**: before posting, each job checks whether any sibling review from the same user already has REQUEST_CHANGES. If so and this job would post APPROVE, it posts as REQUEST_CHANGES instead — maintaining the block. This ensures the PR stays blocked until all checks pass, regardless of execution order. + +**If you need independent approval/block per review type**, use separate Gitea bot accounts with their own tokens. + ## Custom Review Prompts Use `system-prompt-file` to specialize the review focus. The file contents are appended to the base system prompt as "Additional Review Instructions." diff --git a/cmd/review-bot/main.go b/cmd/review-bot/main.go index 8684cac..109436f 100644 --- a/cmd/review-bot/main.go +++ b/cmd/review-bot/main.go @@ -226,6 +226,26 @@ func main() { return } + // Worst-wins: if we're about to APPROVE but a sibling review from the same + // user already has REQUEST_CHANGES, post as REQUEST_CHANGES too so we don't + // override the blocking state. + if event == "APPROVED" && *reviewerName != "" { + existing, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) + if err == nil { + for _, r := range existing { + if !r.Stale && r.State == "REQUEST_CHANGES" { + // Check it's from the same user (same token) but a different role + sentinelCheck := fmt.Sprintf("", *reviewerName) + if !strings.Contains(r.Body, sentinelCheck) { + log.Printf("Sibling review %d has REQUEST_CHANGES; escalating to REQUEST_CHANGES", r.ID) + event = "REQUEST_CHANGES" + break + } + } + } + } + } + log.Printf("Posting review (event=%s)...", event) posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody) if err != nil { @@ -249,6 +269,7 @@ func main() { } } } + } } } -- 2.47.3 From 436e6a88246795e600ff76029a3ed1f98f1992c8 Mon Sep 17 00:00:00 2001 From: Rodin Date: Fri, 1 May 2026 21:16:16 -0700 Subject: [PATCH 7/9] fix: symlink traversal + worst-wins pre-check + user scoping MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security (MAJOR): - Add filepath.EvalSymlinks after Clean for system-prompt-file - Re-validate resolved path is still within workspace - Prevents symlink → /etc/shadow exfiltration via malicious repo Worst-wins: - Check BEFORE posting (not after) — no delete+repost dance - Identify sibling bots by ", *reviewerName) - if !strings.Contains(r.Body, sentinelCheck) { - log.Printf("Sibling review %d has REQUEST_CHANGES; escalating to REQUEST_CHANGES", r.ID) - event = "REQUEST_CHANGES" - break - } - } - } - } + // Validate reviewer-name: only safe characters allowed in sentinel + if err := validateReviewerName(*reviewerName); err != nil { + log.Fatalf("%v", err) } + sentinel := fmt.Sprintf("", *reviewerName) log.Printf("Posting review (event=%s)...", event) posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody) @@ -254,7 +248,6 @@ func main() { log.Printf("Review posted (id=%d, user=%s)", posted.ID, posted.User.Login) // Delete stale reviews from this bot using sentinel matching - sentinel := fmt.Sprintf("", *reviewerName) if *updateExisting && *reviewerName != "" { reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber) if err != nil { @@ -270,6 +263,22 @@ func main() { } } + // Worst-wins: if we posted APPROVE but a sibling review from the + // same user (same token, different role) has REQUEST_CHANGES, + // delete ours and re-post as REQUEST_CHANGES to maintain the block. + if event == "APPROVED" && shouldEscalate(reviews, posted.ID, posted.User.Login, sentinel) { + log.Printf("Sibling review has REQUEST_CHANGES; escalating") + if err := giteaClient.DeleteReview(ctx, owner, repoName, prNumber, posted.ID); err != nil { + log.Printf("Warning: could not delete review for escalation: %v", err) + } else { + _, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "REQUEST_CHANGES", reviewBody) + if err != nil { + log.Printf("Warning: could not re-post as REQUEST_CHANGES: %v", err) + } else { + log.Printf("Review escalated to REQUEST_CHANGES") + } + } + } } } } @@ -413,3 +422,29 @@ func envOrDefaultBool(key string, defaultVal bool) bool { } return v == "true" || v == "1" || v == "yes" } + +// validateReviewerName checks that the name contains only safe characters +// for embedding in an HTML comment sentinel ([a-zA-Z0-9_-]). +func validateReviewerName(name string) error { + if name == "" { + return nil + } + for _, ch := range name { + if !((ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z') || (ch >= '0' && ch <= '9') || ch == '-' || ch == '_') { + return fmt.Errorf("reviewer-name must contain only [a-zA-Z0-9_-] (got %q)", name) + } + } + return nil +} + +// shouldEscalate checks if the current APPROVED review should be escalated +// to REQUEST_CHANGES because a sibling bot review (same user, different role) +// already has REQUEST_CHANGES. +func shouldEscalate(reviews []gitea.Review, postedID int64, postedLogin, ownSentinel string) bool { + for _, r := range reviews { + if r.ID != postedID && !r.Stale && r.User.Login == postedLogin && r.State == "REQUEST_CHANGES" && strings.Contains(r.Body, "", true}, + {"invalid space", "my bot", true}, + {"invalid dot", "my.bot", true}, + {"invalid slash", "my/bot", true}, + {"invalid angle", "bot