feat(#120): add GitHub Actions support with VCS host detection and security hardening

- Detect VCS host type from github.api_url (present on GitHub/GHES, absent on Gitea)
- Add action-repo input: specifies repo hosting review-bot releases, separate from
  the reviewed repo. Defaults to github.action_repository, then rodin/review-bot.
- Add action-repo-token input: auth for release downloads (defaults to github.token
  on GitHub, reviewer-token on Gitea).
- GitHub/GHES path: use github.api_url for version resolution and REST API asset
  download endpoint (required for private repos; web URLs redirect to S3 and don't
  support Authorization headers reliably).
- Gitea path: use validated SERVER_URL with direct download (no -L; prevents
  Authorization forwarding on potential CDN redirects).
- Security hardening:
  - inputs.vcs-url is IGNORED on GitHub/GHES to prevent token exfiltration
  - SERVER_URL validated for https scheme and no whitespace on Gitea path
  - action-repo validated against owner/repo pattern (prevent path traversal)
  - VERSION validated for no slashes/whitespace
  - TOKEN validated for no control characters (header injection defense)
  - ACTION_TOKEN passed via ::add-mask:: + GITHUB_ENV (not step output, which
    can leak in debug logs)
  - set -euo pipefail in both script steps
- Multi-arch support: OS/arch detection via uname (linux/darwin, amd64/arm64)
  for cache key and binary name — incorporates changes from #124
- Run review step: passes VCS_URL from step output (server_url) instead of
  direct input expression

Closes #120

.gitea/actions/review/action.yml

@@ -9,13 +9,7 @@
 #   token exfiltration. API calls use github.api_url; downloads use
 #   github.server_url. Tokens are never sent to user-supplied URLs.
 # - On Gitea (VCS_TYPE=gitea), inputs.vcs-url is validated (https scheme,
-#   no whitespace/newlines, and DNS resolution to a public IP) before use.
-#   Python3 resolves the hostname and rejects RFC1918, RFC6598 (carrier-grade
-#   NAT), loopback, link-local, and other reserved addresses to prevent SSRF attacks.
-#   The installed review-bot binary additionally uses a safe HTTP transport
-#   (DialContext-level IP check) for all Gitea API calls at runtime.
-#   The binary also exposes a `validate-url` subcommand for use in any future
-#   shell steps that need to validate a URL before passing it to curl.
+#   no whitespace/newlines) before use.
 # - action-repo is validated against owner/repo pattern.
 # - Tokens are passed via masked environment variables, not step outputs.
 #
@@ -191,36 +185,6 @@ runs:
             echo "Error: SERVER_URL '${SERVER_URL}' must be an https:// URL with no whitespace" >&2
             exit 1
           fi
-
-          # Additional IP-level SSRF defense: resolve the hostname and reject
-          # requests to RFC1918, RFC6598 (carrier-grade NAT), loopback, link-local,
-          # and other reserved addresses.
-          # python3 is required on ubuntu-* runners (see requirements comment above).
-          # Use printf to write the script to a temp file so the python lines are valid
-          # YAML (each indented line becomes a printf argument — no unindented code).
-          # SERVER_URL is passed via CHECK_URL env var, never interpolated into python code.
-          printf '%s\n' \
-            'import socket,ipaddress,sys,os' \
-            'from urllib.parse import urlparse' \
-            'u=os.environ["CHECK_URL"]; parsed=urlparse(u)' \
-            'if parsed.username or parsed.password:' \
-            '  print("Error: URL contains user-info — not allowed",file=sys.stderr); sys.exit(2)' \
-            'h=parsed.hostname' \
-            '(print("Error: no hostname",file=sys.stderr) or sys.exit(2)) if not h else None' \
-            'try: rs=socket.getaddrinfo(h,None)' \
-            'except socket.gaierror as e: print(f"DNS error: {e}",file=sys.stderr); sys.exit(1)' \
-            'if not rs: print("Error: no addresses",file=sys.stderr); sys.exit(1)' \
-            'for _,_,_,_,(a,*_) in rs:' \
-            '  ip=ipaddress.ip_address(a)' \
-            '  if isinstance(ip,ipaddress.IPv6Address) and ip.ipv4_mapped: ip=ip.ipv4_mapped' \
-            '  cgn=ipaddress.ip_network("100.64.0.0/10")' \
-            '  if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_multicast or ip.is_reserved or ip in cgn:' \
-            '    print(f"blocked: {a}",file=sys.stderr); sys.exit(1)' \
-            > /tmp/_ssrf_check.py
-          CHECK_URL="${SERVER_URL}" python3 /tmp/_ssrf_check.py || {
-            echo "Error: SERVER_URL '${SERVER_URL}' resolves to a private/reserved IP address" >&2
-            exit 1
-          }
         fi
 
         # Determine auth token for release API requests
@@ -341,36 +305,6 @@ runs:
         ACTION_TOKEN="${ACTION_TOKEN:-}"
         BINARY="review-bot-${OS}-${ARCH}"
 
-        # SECURITY: Re-validate SERVER_URL at the start of this step to mitigate DNS
-        # rebinding attacks. A DNS TTL expiry between "Determine version" and here
-        # could allow an attacker to change the resolved IP to a private/reserved
-        # address, causing curl to send ACTION_TOKEN to an internal host.
-        # Only needed on Gitea path (VCS_TYPE=gitea); GitHub/GHES uses platform-controlled URLs.
-        if [ "$VCS_TYPE" = "gitea" ]; then
-          printf '%s\n' \
-            'import socket,ipaddress,sys,os' \
-            'from urllib.parse import urlparse' \
-            'u=os.environ["CHECK_URL"]; parsed=urlparse(u)' \
-            'if parsed.username or parsed.password:' \
-            '  print("Error: URL contains user-info — not allowed",file=sys.stderr); sys.exit(2)' \
-            'h=parsed.hostname' \
-            '(print("Error: no hostname",file=sys.stderr) or sys.exit(2)) if not h else None' \
-            'try: rs=socket.getaddrinfo(h,None)' \
-            'except socket.gaierror as e: print(f"DNS error: {e}",file=sys.stderr); sys.exit(1)' \
-            'if not rs: print("Error: no addresses",file=sys.stderr); sys.exit(1)' \
-            'for _,_,_,_,(a,*_) in rs:' \
-            '  ip=ipaddress.ip_address(a)' \
-            '  if isinstance(ip,ipaddress.IPv6Address) and ip.ipv4_mapped: ip=ip.ipv4_mapped' \
-            '  cgn=ipaddress.ip_network("100.64.0.0/10")' \
-            '  if ip.is_private or ip.is_loopback or ip.is_link_local or ip.is_multicast or ip.is_reserved or ip in cgn:' \
-            '    print(f"blocked: {a}",file=sys.stderr); sys.exit(1)' \
-              > /tmp/_ssrf_check_install.py
-          CHECK_URL="${SERVER_URL}" python3 /tmp/_ssrf_check_install.py || {
-            echo "Error: SERVER_URL '${SERVER_URL}' resolves to a private/reserved IP address" >&2
-            exit 1
-          }
-        fi
-
         if [ "$VCS_TYPE" = "github" ]; then
           # GitHub/GHES: Use REST API for release asset downloads.
           # Web release URLs ({server}/.../releases/download/{tag}/{asset}) redirect
@@ -389,13 +323,27 @@ runs:
           fi
 
           # Extract asset IDs for binary and checksums
-          BINARY_ASSET_ID=$(printf '%s' "$RELEASE_JSON" | python3 -c "import sys, json; assets = json.load(sys.stdin).get('assets', []); matches = [a['id'] for a in assets if a['name'] == '${BINARY}']; print(matches[0] if matches else '')")
+          BINARY_ASSET_ID=$(printf '%s' "$RELEASE_JSON" | python3 -c "
+import sys, json
+assets = json.load(sys.stdin).get('assets', [])
+for a in assets:
+    if a['name'] == '${BINARY}':
+        print(a['id'])
+        break
+")
           if [ -z "$BINARY_ASSET_ID" ]; then
             echo "Error: could not find asset '${BINARY}' in release ${VERSION}" >&2
             exit 1
           fi
 
-          CHECKSUMS_ASSET_ID=$(printf '%s' "$RELEASE_JSON" | python3 -c "import sys, json; assets = json.load(sys.stdin).get('assets', []); matches = [a['id'] for a in assets if a['name'] == 'checksums.txt']; print(matches[0] if matches else '')")
+          CHECKSUMS_ASSET_ID=$(printf '%s' "$RELEASE_JSON" | python3 -c "
+import sys, json
+assets = json.load(sys.stdin).get('assets', [])
+for a in assets:
+    if a['name'] == 'checksums.txt':
+        print(a['id'])
+        break
+")
           if [ -z "$CHECKSUMS_ASSET_ID" ]; then
             echo "Error: could not find asset 'checksums.txt' in release ${VERSION}" >&2
             exit 1

.github/workflows/review.yml

@@ -1,77 +0,0 @@
-name: AI Code Review
-
-# AI code review for pull requests on github.concur.com/strat/review-bot.
-# Uses SAP AI Core as the LLM provider (same as the Gitea CI workflow).
-#
-# Prerequisites before this workflow can run:
-#   1. Set required secrets on strat/review-bot (see list below)
-#   2. Publish at least one release of review-bot on strat/review-bot
-#      (or change action-repo to a repo that already has releases)
-#
-# Required secrets:
-#   SONNET_REVIEW_TOKEN  — GitHub token for the Sonnet reviewer bot
-#   GPT_REVIEW_TOKEN     — GitHub token for the GPT reviewer bot
-#   AICORE_CLIENT_ID     — SAP AI Core OAuth client ID
-#   AICORE_CLIENT_SECRET — SAP AI Core OAuth client secret
-#   AICORE_AUTH_URL      — SAP AI Core OAuth token endpoint
-#   AICORE_API_URL       — SAP AI Core inference API URL
-#   AICORE_RESOURCE_GROUP — SAP AI Core resource group (optional, default: default)
-
-on:
-  pull_request:
-    types: [opened, synchronize]
-
-jobs:
-  test:
-    runs-on: ubuntu-24.04
-    steps:
-      - uses: actions/checkout@v4
-      - uses: actions/setup-go@v5
-        with:
-          go-version: '1.26'
-      - run: go test ./...
-      - run: go vet ./...
-      - run: go build -o review-bot ./cmd/review-bot
-
-  review:
-    runs-on: ubuntu-24.04
-    if: github.event_name == 'pull_request'
-    needs: test
-    strategy:
-      fail-fast: false
-      matrix:
-        include:
-          - name: sonnet
-            token_secret: SONNET_REVIEW_TOKEN
-            model: anthropic--claude-4.6-sonnet
-          - name: gpt
-            token_secret: GPT_REVIEW_TOKEN
-            model: gpt-5
-          - name: security
-            token_secret: GPT_REVIEW_TOKEN
-            model: gpt-5
-            system_prompt_file: SECURITY_REVIEW.md
-    steps:
-      - uses: actions/checkout@v4
-      - uses: ./.gitea/actions/review
-        with:
-          # On GHES runners, vcs-url is ignored; the composite action uses github.server_url.
-          # action-repo must be a repo with published review-bot releases.
-          # Requires strat/review-bot to have at least one release tag with
-          # review-bot-linux-amd64 and checksums.txt assets.
-          vcs-url: https://gitea.weiker.me
-          action-repo: strat/review-bot
-          reviewer-token: ${{ secrets[matrix.token_secret] }}
-          reviewer-name: ${{ matrix.name }}
-          llm-provider: aicore
-          llm-model: ${{ matrix.model }}
-          aicore-client-id: ${{ secrets.AICORE_CLIENT_ID }}
-          aicore-client-secret: ${{ secrets.AICORE_CLIENT_SECRET }}
-          aicore-auth-url: ${{ secrets.AICORE_AUTH_URL }}
-          aicore-api-url: ${{ secrets.AICORE_API_URL }}
-          aicore-resource-group: ${{ secrets.AICORE_RESOURCE_GROUP }}
-          conventions-file: CONVENTIONS.md
-          patterns-repo: rodin/go-patterns
-          patterns-files: README.md,patterns/
-          timeout: "600"
-          system-prompt-file: ${{ matrix.system_prompt_file || '' }}

TODO.md

@@ -1,79 +0,0 @@
-## Dev Loop: review-bot — 2026-05-14 20:10 UTC
-
-### Latest: ✅ STABLE STATE — REPO HEALTH COMPLETE
-- **Last action:** health check; verified tests pass, repo clean, no action needed
-- **Repository:** Clean, all merges complete, no open issues/PRs
-- **Main branch:** Up to date with origin/main
-- **Test suite:** All passing (cached)
-
----
-
-## Repository Status
-
-### ✅ Merged to main (recent):
-- issue-123 (IP-level SSRF defense) — 6 commits, main at 4440823
-- issue-125 (VCS_URL rename + deprecation) — merged
-- issue-124 (multi-arch binary support) — merged  
-- issue-120 (GitHub Actions + VCS abstraction) — merged
-- issue-121 (VCS host type detection for binary download) — merged
-
-### 🧹 Cleanup COMPLETE:
-- ✅ Removed old worktrees (issue-123, review-bot-issue-125)
-- ✅ Test suite passes (all packages)
-- ✅ No TODO/FIXME in code except expected GitHub client notes
-- ✅ No open issues or pull requests
-- ✅ Dependencies up to date
-
----
-
-## Current Feature Completeness
-
-✅ **Core Capabilities:**
-- Multi-provider LLM support (OpenAI, Anthropic, SAP AI Core)
-- Gitea PR integration with structured reviews
-- SSRF defense with IP-level validation
-- VCS abstraction (Gitea/GitHub support)
-- Multi-architecture binary support
-- GitHub Actions composite action
-
-✅ **Recent Security Work:**
-- RFC6598 CGN range detection
-- IP fallback dialing for local endpoint rejection
-- URL validation for SSRF prevention
-
-✅ **Code Quality:**
-- Comprehensive test coverage (all packages tested)
-- Consistent error handling with context propagation
-- Secure credential handling (unexported fields)
-- Concurrency-safe designs
-
----
-
-## Next Priority Actions
-
-### Phase 2: Feature Exploration (NEXT SESSION)
-- Scan code for potential improvements per REVIEW.md findings
-- Assess performance under load
-- Review REVIEW.md findings for targeted fixes
-- Consider backlog items from design docs
-
-### Phase 3: Optional Enhancements (BACKLOG)
-- Address REVIEW.md context propagation findings (if prioritized)
-- Additional LLM provider support
-- Enhanced context detection
-- Custom report formats
-- Webhook management improvements
-
----
-
-## Worktrees Status
-All old worktrees cleaned up. Ready for new issue work.
-
----
-
-## Dev-Loop Metadata
-- **Repo:** /home/ubuntu/review-bot
-- **Main branch SHA:** ed3a5dd (last commit)
-- **Cron ID:** 5342ac81-4bbc-4e4c-a123-347a7788d50c
-- **Scheduled:** Every 4 hours
-- **Last health check:** 2026-05-14 20:10 UTC (✅ all healthy)

cmd/review-bot/main.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"flag"
 	"fmt"
-	"io"
 	"log/slog"
 	"os"
 	"path/filepath"
@@ -20,13 +19,6 @@ import (
 
 var version = "dev"
 
-// outWriter and errWriter are the output and error writers for subcommands.
-// They are variables so tests can capture output.
-var (
-	outWriter io.Writer = os.Stdout
-	errWriter io.Writer = os.Stderr
-)
-
 // setupLogger configures the global slog default logger based on format and verbosity.
 func setupLogger(format, verbosity string) {
 	var level slog.Level
@@ -57,15 +49,6 @@ func setupLogger(format, verbosity string) {
 }
 
 func main() {
-	// Dispatch subcommands before flag parsing so they get their own args.
-	// e.g. `review-bot validate-url <url>`
-	if len(os.Args) > 1 {
-		switch os.Args[1] {
-		case "validate-url":
-			os.Exit(runValidateURL(os.Args[2:]))
-		}
-	}
-
 	versionFlag := flag.Bool("version", false, "Print version and exit")
 	// Logging flags
 	logFormat := flag.String("log-format", envOrDefault("LOG_FORMAT", "text"), "Log output format: text or json")
@@ -168,12 +151,8 @@ func main() {
 		os.Exit(1)
 	}
 
-	// Detect VCS type and initialize the appropriate client.
-	vcsType := detectVCSType()
-	slog.Info("detected VCS type", "vcs", vcsType)
-
 	// Initialize clients
-	giteaClient := newVCSClient(vcsType, *vcsURL, *reviewerToken)
+	giteaClient := gitea.NewClient(*vcsURL, *reviewerToken)
 	llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel)
 	if *llmTemp < 0 || *llmTemp > 2 {
 		slog.Error("invalid LLM temperature", "temperature", *llmTemp, "range", "0-2")
@@ -211,7 +190,7 @@ func main() {
 	var persona *review.Persona
 	if *personaName != "" {
 		// Try loading from repo first, then fall back to built-in
-		repoPersonas, err := review.LoadRepoPersonas(ctx, buildRepoPersonaClient(giteaClient), owner, repoName)
+		repoPersonas, err := review.LoadRepoPersonas(ctx, newGiteaClientAdapter(giteaClient), owner, repoName)
 		if err != nil {
 			slog.Warn("could not load repo personas", "repo", owner+"/"+repoName, "error", err)
 			// Continue with built-in personas only.
@@ -531,7 +510,7 @@ func main() {
 }
 
 // fetchFileContext fetches the full content of modified files from the PR branch.
-func fetchFileContext(ctx context.Context, client vcsClient, owner, repo, ref string, files []gitea.ChangedFile) string {
+func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, ref string, files []gitea.ChangedFile) string {
 	var sb strings.Builder
 	for _, f := range files {
 		if ctx.Err() != nil {
@@ -558,7 +537,7 @@ func fetchFileContext(ctx context.Context, client vcsClient, owner, repo, ref st
 // patternsFiles is comma-separated list of file paths or directories.
 // If a path ends with / or is a directory, all files within it are fetched recursively.
 // If patternsFiles is empty, all files from the repo root are fetched.
-func fetchPatterns(ctx context.Context, client vcsClient, patternsRepo, patternsFiles string) string {
+func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patternsFiles string) string {
 	var sb strings.Builder
 
 	repos := strings.Split(patternsRepo, ",")
@@ -855,3 +834,31 @@ func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool {
 	return evaluatedSHA != currentSHA
 }
 
+// giteaClientAdapter adapts gitea.Client to review.GiteaClient interface.
+type giteaClientAdapter struct {
+	client *gitea.Client
+}
+
+func newGiteaClientAdapter(c *gitea.Client) *giteaClientAdapter {
+	return &giteaClientAdapter{client: c}
+}
+
+func (a *giteaClientAdapter) ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error) {
+	entries, err := a.client.ListContents(ctx, owner, repo, path)
+	if err != nil {
+		return nil, err
+	}
+	result := make([]review.ContentEntry, len(entries))
+	for i, e := range entries {
+		result[i] = review.ContentEntry{
+			Name: e.Name,
+			Path: e.Path,
+			Type: e.Type,
+		}
+	}
+	return result, nil
+}
+
+func (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
+	return a.client.GetFileContent(ctx, owner, repo, filepath)
+}

cmd/review-bot/validateurl.go

@@ -1,125 +0,0 @@
-package main
-
-import (
-	"context"
-	"errors"
-	"fmt"
-	"net"
-	"net/url"
-	"strings"
-	"time"
-
-	"gitea.weiker.me/rodin/review-bot/gitea"
-)
-
-// runValidateURL implements the `review-bot validate-url <url>` subcommand.
-//
-// It resolves the given URL's hostname and checks that every returned IP is
-// publicly routable (not RFC1918, loopback, link-local, or other reserved
-// ranges). The exit code communicates the result to callers:
-//
-//	0 — URL is safe to use
-//	1 — URL resolves to a blocked/private address
-//	2 — URL is malformed, has an unsafe scheme, or DNS lookup failed
-//
-// This is intended for use from action.yml shell steps that need to validate
-// a user-supplied URL before passing it to curl.
-func runValidateURL(args []string) int {
-	if len(args) != 1 {
-		fmt.Fprintln(errWriter, "usage: review-bot validate-url <url>")
-		fmt.Fprintln(errWriter, "")
-		fmt.Fprintln(errWriter, "Resolves <url> and verifies all resolved IPs are publicly routable.")
-		fmt.Fprintln(errWriter, "Exit 0=safe, 1=blocked, 2=error")
-		return 2
-	}
-	rawURL := args[0]
-
-	if err := validateURL(rawURL); err != nil {
-		fmt.Fprintf(errWriter, "Error: %v\n", err)
-		var ve *validateError
-		if isValidateError(err, &ve) {
-			return ve.code
-		}
-		return 2
-	}
-	fmt.Fprintf(outWriter, "OK: %s is safe\n", rawURL)
-	return 0
-}
-
-// validateError carries an exit code alongside a message.
-type validateError struct {
-	code    int
-	message string
-}
-
-func (e *validateError) Error() string { return e.message }
-
-// isValidateError checks if err is or wraps a *validateError and sets out.
-// Uses errors.As so that wrapped *validateError values (e.g. from fmt.Errorf("...: %w", &validateError{...}))
-// are also detected, making the function robust against future wrapping.
-func isValidateError(err error, out **validateError) bool {
-	if err == nil {
-		return false
-	}
-	return errors.As(err, out)
-}
-
-// validateURL checks that rawURL is safe for use as a Gitea server URL:
-//   - Must be https:// (not http://)
-//   - Must have no user-info (user:pass@host)
-//   - Must resolve to at least one IP, all of which are publicly routable
-func validateURL(rawURL string) error {
-	parsed, err := url.Parse(rawURL)
-	if err != nil {
-		return &validateError{code: 2, message: fmt.Sprintf("malformed URL %q: %v", rawURL, err)}
-	}
-
-	// Scheme check: only https is permitted.
-	if !strings.EqualFold(parsed.Scheme, "https") {
-		return &validateError{
-			code:    2,
-			message: fmt.Sprintf("URL scheme must be https (got %q)", parsed.Scheme),
-		}
-	}
-
-	// Reject user-info (user:password@host) to prevent credential embedding.
-	if parsed.User != nil {
-		return &validateError{
-			code:    2,
-			message: "URL must not contain user-info (user:password@host)",
-		}
-	}
-
-	host := parsed.Hostname()
-	if host == "" {
-		return &validateError{code: 2, message: fmt.Sprintf("URL has no host: %q", rawURL)}
-	}
-
-	// Resolve the hostname with a short timeout.
-	ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
-	defer cancel()
-
-	addrs, err := net.DefaultResolver.LookupIPAddr(ctx, host)
-	if err != nil {
-		return &validateError{
-			code:    2,
-			message: fmt.Sprintf("DNS lookup failed for %q: %v", host, err),
-		}
-	}
-	if len(addrs) == 0 {
-		return &validateError{
-			code:    2,
-			message: fmt.Sprintf("DNS lookup returned no addresses for %q", host),
-		}
-	}
-
-	for _, a := range addrs {
-		if gitea.IsBlockedIP(a.IP) {
-			return &validateError{
-				code:    1,
-				message: fmt.Sprintf("blocked: %q resolves to private/reserved IP %s", host, a.IP),
-			}
-		}
-	}
-	return nil
-}

cmd/review-bot/validateurl_test.go

@@ -1,127 +0,0 @@
-package main
-
-import (
-	"bytes"
-	"strings"
-	"testing"
-)
-
-func TestRunValidateURL_Usage(t *testing.T) {
-	var errBuf bytes.Buffer
-	origErr := errWriter
-	errWriter = &errBuf
-	defer func() { errWriter = origErr }()
-
-	code := runValidateURL(nil)
-	if code != 2 {
-		t.Errorf("expected exit code 2 for no args, got %d", code)
-	}
-	if !strings.Contains(errBuf.String(), "usage") {
-		t.Errorf("expected usage in stderr, got %q", errBuf.String())
-	}
-
-	errBuf.Reset()
-	code = runValidateURL([]string{"arg1", "arg2"})
-	if code != 2 {
-		t.Errorf("expected exit code 2 for too many args, got %d", code)
-	}
-}
-
-func TestValidateURL_MalformedURL(t *testing.T) {
-	cases := []struct {
-		name    string
-		url     string
-		wantMsg string
-	}{
-		{"empty", "", "must be https"},
-		{"http scheme", "http://example.com/", "must be https"},
-		{"ftp scheme", "ftp://example.com/", "must be https"},
-		{"no scheme", "example.com", "must be https"},
-		{"user info", "https://user:pass@example.com/", "user-info"},
-	}
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			err := validateURL(tc.url)
-			if err == nil {
-				t.Errorf("expected error for URL %q, got nil", tc.url)
-				return
-			}
-			if !strings.Contains(err.Error(), tc.wantMsg) {
-				t.Errorf("error %q does not contain %q", err.Error(), tc.wantMsg)
-			}
-			var ve *validateError
-			if !isValidateError(err, &ve) {
-				t.Fatalf("expected *validateError, got %T", err)
-			}
-			if ve.code != 2 {
-				t.Errorf("expected code 2, got %d", ve.code)
-			}
-		})
-	}
-}
-
-func TestValidateURL_BlockedPrivateIP(t *testing.T) {
-	// localhost always resolves to 127.0.0.1 (loopback).
-	err := validateURL("https://localhost/")
-	if err == nil {
-		t.Skip("localhost did not resolve (network unavailable in test environment)")
-	}
-	var ve *validateError
-	if !isValidateError(err, &ve) {
-		t.Fatalf("expected *validateError, got %T: %v", err, err)
-	}
-	if ve.code != 1 && ve.code != 2 {
-		t.Errorf("expected code 1 (blocked) or 2 (dns fail), got %d: %s", ve.code, ve.message)
-	}
-	// If it resolved (code 1), the message must say "blocked".
-	if ve.code == 1 && !strings.Contains(ve.message, "blocked") {
-		t.Errorf("expected 'blocked' in message, got %q", ve.message)
-	}
-}
-
-func TestValidateURL_ExitCodes(t *testing.T) {
-	cases := []struct {
-		name     string
-		url      string
-		wantCode int
-	}{
-		{"http scheme", "http://example.com/", 2},
-		{"no scheme", "example.com", 2},
-		{"user info", "https://admin:secret@example.com/", 2},
-	}
-	for _, tc := range cases {
-		t.Run(tc.name, func(t *testing.T) {
-			err := validateURL(tc.url)
-			if err == nil {
-				t.Fatalf("expected error for %q", tc.url)
-			}
-			var ve *validateError
-			if !isValidateError(err, &ve) {
-				t.Fatalf("expected *validateError, got %T", err)
-			}
-			if ve.code != tc.wantCode {
-				t.Errorf("code = %d, want %d (url=%q, msg=%s)", ve.code, tc.wantCode, tc.url, ve.message)
-			}
-		})
-	}
-}
-
-func TestRunValidateURL_WithCapture(t *testing.T) {
-	var outBuf, errBuf bytes.Buffer
-	origOut, origErr := outWriter, errWriter
-	outWriter = &outBuf
-	errWriter = &errBuf
-	defer func() {
-		outWriter = origOut
-		errWriter = origErr
-	}()
-
-	// http:// scheme should fail with code 2.
-	code := runValidateURL([]string{"http://example.com/"})
-	if code != 2 {
-		t.Errorf("expected code 2 for http:// URL, got %d", code)
-	}
-	if !strings.Contains(errBuf.String(), "must be https") {
-		t.Errorf("expected error about https in stderr, got %q", errBuf.String())
-	}
-}

cmd/review-bot/vcs.go

@@ -1,295 +0,0 @@
-package main
-
-// vcs.go — VCS client abstraction for supporting both Gitea and GitHub.
-//
-// This file defines the vcsClient interface that main.go uses for all VCS
-// operations, and provides a githubAdapter that wraps *github.Client and
-// converts between github-package types and the gitea-package types used
-// throughout the rest of the binary.
-//
-// Design rationale: the entire codebase was written against gitea types.
-// Rather than introduce a third "shared" type package and update every call
-// site, the adapter converts at the boundary. The conversion is cheap — these
-// are small structs fetched once per run.
-
-import (
-	"context"
-	"os"
-
-	githubpkg "gitea.weiker.me/rodin/review-bot/github"
-	"gitea.weiker.me/rodin/review-bot/gitea"
-	"gitea.weiker.me/rodin/review-bot/review"
-)
-
-// vcsClient is the interface that main.go uses for all VCS API operations.
-// Both *gitea.Client (directly) and *githubAdapter (via this file) satisfy it.
-type vcsClient interface {
-	GetPullRequest(ctx context.Context, owner, repo string, number int) (*gitea.PullRequest, error)
-	GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error)
-	GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]gitea.ChangedFile, error)
-	GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]gitea.CommitStatus, error)
-	GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error)
-	GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error)
-	GetAllFilesInPath(ctx context.Context, owner, repo, path string) (map[string]string, error)
-	ListReviews(ctx context.Context, owner, repo string, number int) ([]gitea.Review, error)
-	GetAuthenticatedUser(ctx context.Context) (string, error)
-	RequestReviewer(ctx context.Context, owner, repo string, number int, reviewer string) error
-	PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []gitea.ReviewComment) (*gitea.Review, error)
-	GetTimelineReviewCommentIDForReview(ctx context.Context, owner, repo string, number int, reviewID int64) (int64, error)
-	EditComment(ctx context.Context, owner, repo string, commentID int64, newBody string) error
-	ListReviewComments(ctx context.Context, owner, repo string, prNumber int, reviewID int64) ([]gitea.ReviewComment, error)
-	ResolveComment(ctx context.Context, owner, repo string, commentID int64) error
-	ListContents(ctx context.Context, owner, repo, path string) ([]gitea.ContentEntry, error)
-}
-
-// vcsClientAdapterForPersona adapts vcsClient to review.GiteaClient.
-// Used by LoadRepoPersonas which needs only ListContents + GetFileContent.
-type vcsClientAdapterForPersona struct {
-	client vcsClient
-}
-
-func newVCSClientAdapterForPersona(c vcsClient) *vcsClientAdapterForPersona {
-	return &vcsClientAdapterForPersona{client: c}
-}
-
-func (a *vcsClientAdapterForPersona) 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 *vcsClientAdapterForPersona) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
-	return a.client.GetFileContent(ctx, owner, repo, filepath)
-}
-
-// detectVCSType returns "github" if the environment indicates a GitHub or GHES
-// runner, "gitea" otherwise.
-//
-// Detection logic mirrors the action.yml composite action:
-//   - GITHUB_API_URL is set by GitHub Actions runners (github.com and GHES)
-//   - On Gitea Actions runners it is empty or absent
-func detectVCSType() string {
-	if os.Getenv("GITHUB_API_URL") != "" {
-		return "github"
-	}
-	return "gitea"
-}
-
-// githubAPIBaseURL returns the GitHub API base URL from the environment.
-// On GitHub.com this is https://api.github.com.
-// On GHES this is https://<host>/api/v3.
-func githubAPIBaseURL() string {
-	if u := os.Getenv("GITHUB_API_URL"); u != "" {
-		return u
-	}
-	return "https://api.github.com"
-}
-
-// githubAdapter wraps *github.Client and translates github-package types to
-// gitea-package types so that the rest of main.go can remain unchanged.
-type githubAdapter struct {
-	c *githubpkg.Client
-}
-
-func newGitHubAdapter(token, apiBaseURL string) *githubAdapter {
-	return &githubAdapter{c: githubpkg.NewClient(token, apiBaseURL)}
-}
-
-func (a *githubAdapter) GetPullRequest(ctx context.Context, owner, repo string, number int) (*gitea.PullRequest, error) {
-	pr, err := a.c.GetPullRequest(ctx, owner, repo, number)
-	if err != nil {
-		return nil, err
-	}
-	return &gitea.PullRequest{
-		Title: pr.Title,
-		Body:  pr.Body,
-		Head: struct {
-			Sha string "json:\"sha\""
-			Ref string "json:\"ref\""
-		}{Sha: pr.Head.Sha, Ref: pr.Head.Ref},
-	}, nil
-}
-
-func (a *githubAdapter) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) {
-	return a.c.GetPullRequestDiff(ctx, owner, repo, number)
-}
-
-func (a *githubAdapter) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]gitea.ChangedFile, error) {
-	files, err := a.c.GetPullRequestFiles(ctx, owner, repo, number)
-	if err != nil {
-		return nil, err
-	}
-	result := make([]gitea.ChangedFile, len(files))
-	for i, f := range files {
-		result[i] = gitea.ChangedFile{
-			Filename: f.Filename,
-			Status:   f.Status,
-		}
-	}
-	return result, nil
-}
-
-func (a *githubAdapter) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]gitea.CommitStatus, error) {
-	statuses, err := a.c.GetCommitStatuses(ctx, owner, repo, sha)
-	if err != nil {
-		return nil, err
-	}
-	result := make([]gitea.CommitStatus, len(statuses))
-	for i, s := range statuses {
-		// GitHub uses "state" with values: success, failure, pending, error.
-		// Gitea uses "status" with values: success, failure, pending, warning, error.
-		// Map GitHub's "state" to gitea's "status" field for evaluateCIStatus().
-		result[i] = gitea.CommitStatus{
-			Status:      s.State,
-			Context:     s.Context,
-			Description: s.Description,
-			TargetURL:   s.TargetURL,
-		}
-	}
-	return result, nil
-}
-
-func (a *githubAdapter) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
-	return a.c.GetFileContent(ctx, owner, repo, filepath)
-}
-
-func (a *githubAdapter) GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) {
-	return a.c.GetFileContentRef(ctx, owner, repo, filepath, ref)
-}
-
-func (a *githubAdapter) GetAllFilesInPath(ctx context.Context, owner, repo, path string) (map[string]string, error) {
-	return a.c.GetAllFilesInPath(ctx, owner, repo, path)
-}
-
-func (a *githubAdapter) ListReviews(ctx context.Context, owner, repo string, number int) ([]gitea.Review, error) {
-	reviews, err := a.c.ListReviews(ctx, owner, repo, number)
-	if err != nil {
-		return nil, err
-	}
-	result := make([]gitea.Review, len(reviews))
-	for i, r := range reviews {
-		result[i] = gitea.Review{
-			ID:   r.ID,
-			Body: r.Body,
-			User: struct {
-				Login string "json:\"login\""
-			}{Login: r.User.Login},
-			State:    r.State,
-			CommitID: r.CommitID,
-		}
-	}
-	return result, nil
-}
-
-func (a *githubAdapter) GetAuthenticatedUser(ctx context.Context) (string, error) {
-	return a.c.GetAuthenticatedUser(ctx)
-}
-
-func (a *githubAdapter) RequestReviewer(ctx context.Context, owner, repo string, number int, reviewer string) error {
-	return a.c.RequestReviewer(ctx, owner, repo, number, reviewer)
-}
-
-func (a *githubAdapter) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []gitea.ReviewComment) (*gitea.Review, error) {
-	// Convert gitea ReviewComments to github ReviewComments.
-	// NewPosition in Gitea maps to Position in GitHub (diff line position).
-	ghComments := make([]githubpkg.ReviewComment, len(comments))
-	for i, c := range comments {
-		ghComments[i] = githubpkg.ReviewComment{
-			Path:     c.Path,
-			Position: c.NewPosition,
-			Body:     c.Body,
-		}
-	}
-	review, err := a.c.PostReview(ctx, owner, repo, number, event, body, commitID, ghComments)
-	if err != nil {
-		return nil, err
-	}
-	return &gitea.Review{
-		ID:   review.ID,
-		Body: review.Body,
-		User: struct {
-			Login string "json:\"login\""
-		}{Login: review.User.Login},
-		State:    review.State,
-		CommitID: review.CommitID,
-	}, nil
-}
-
-func (a *githubAdapter) GetTimelineReviewCommentIDForReview(ctx context.Context, owner, repo string, number int, reviewID int64) (int64, error) {
-	return a.c.GetTimelineReviewCommentIDForReview(ctx, owner, repo, number, reviewID)
-}
-
-func (a *githubAdapter) EditComment(ctx context.Context, owner, repo string, commentID int64, newBody string) error {
-	return a.c.EditComment(ctx, owner, repo, commentID, newBody)
-}
-
-func (a *githubAdapter) ListReviewComments(ctx context.Context, owner, repo string, prNumber int, reviewID int64) ([]gitea.ReviewComment, error) {
-	comments, err := a.c.ListReviewComments(ctx, owner, repo, prNumber, reviewID)
-	if err != nil {
-		return nil, err
-	}
-	result := make([]gitea.ReviewComment, len(comments))
-	for i, c := range comments {
-		result[i] = gitea.ReviewComment{
-			ID:          c.ID,
-			Path:        c.Path,
-			NewPosition: c.Position,
-			Body:        c.Body,
-		}
-	}
-	return result, nil
-}
-
-func (a *githubAdapter) ResolveComment(ctx context.Context, owner, repo string, commentID int64) error {
-	return a.c.ResolveComment(ctx, owner, repo, commentID)
-}
-
-func (a *githubAdapter) ListContents(ctx context.Context, owner, repo, path string) ([]gitea.ContentEntry, error) {
-	entries, err := a.c.ListContents(ctx, owner, repo, path)
-	if err != nil {
-		return nil, err
-	}
-	result := make([]gitea.ContentEntry, len(entries))
-	for i, e := range entries {
-		result[i] = gitea.ContentEntry{
-			Name: e.Name,
-			Path: e.Path,
-			Type: e.Type,
-		}
-	}
-	return result, nil
-}
-
-// newVCSClient creates the appropriate VCS client based on detected VCS type.
-// On GitHub/GHES (GITHUB_API_URL set), returns a githubAdapter.
-// On Gitea (GITHUB_API_URL absent), returns *gitea.Client directly.
-//
-// For GitHub: uses GITHUB_API_URL as the API base URL (trusted platform value).
-// For Gitea: uses vcsURL (validated before this call).
-func newVCSClient(vcsType, vcsURL, reviewerToken string) vcsClient {
-	switch vcsType {
-	case "github":
-		apiURL := githubAPIBaseURL()
-		return newGitHubAdapter(reviewerToken, apiURL)
-	default:
-		return gitea.NewClient(vcsURL, reviewerToken)
-	}
-}
-
-// buildRepoPersonaClient creates a review.GiteaClient from the active vcsClient.
-// This exists because LoadRepoPersonas expects the review.GiteaClient interface
-// (which only requires ListContents + GetFileContent).
-func buildRepoPersonaClient(c vcsClient) review.GiteaClient {
-	return newVCSClientAdapterForPersona(c)
-}
-

gitea/client.go

@@ -106,113 +106,34 @@ func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
 	return nil
 }
 
-// safeDialContext is the default DialContext for NewClient.
-// It resolves the hostname and checks every returned IP against the blocked
-// CIDR list before establishing a connection. This prevents SSRF attacks
-// where user-supplied URLs resolve to internal/private addresses.
-//
-// After validating all IPs, we dial the first resolved IP directly to avoid
-// a second DNS lookup (which could return a different IP in a DNS rebinding
-// attack). This narrows — but does not fully eliminate — the DNS rebinding
-// window to the time between LookupIPAddr and DialContext.
-//
-// If the host is already an IP literal, LookupIPAddr returns it directly
-// (no DNS query issued), so IP literals like https://127.0.0.1/ are blocked.
-func safeDialContext(ctx context.Context, network, addr string) (net.Conn, error) {
-	host, port, err := net.SplitHostPort(addr)
-	if err != nil {
-		return nil, fmt.Errorf("safeDialContext: invalid address %q: %w", addr, err)
-	}
-	addrs, err := net.DefaultResolver.LookupIPAddr(ctx, host)
-	if err != nil {
-		return nil, fmt.Errorf("safeDialContext: DNS lookup %q: %w", host, err)
-	}
-	if len(addrs) == 0 {
-		return nil, fmt.Errorf("safeDialContext: no addresses returned for %q", host)
-	}
-	for _, a := range addrs {
-		if IsBlockedIP(a.IP) {
-			return nil, fmt.Errorf("safeDialContext: blocked: %q resolves to private/reserved IP %s", host, a.IP)
-		}
-	}
-	// Try each resolved IP in order, returning the first successful connection.
-	// Fallback is important when a hostname resolves to multiple IPs and the first
-	// is temporarily unreachable. All IPs were already validated above, so dialing
-	// any of them is safe.
-	//
-	// Timeout: 10s per the design (PLAN.md); the outer http.Client has a 30s
-	// total timeout, but the per-dial timeout ensures a slow TCP connect on one IP
-	// doesn't consume the budget needed to try others.
-	d := &net.Dialer{Timeout: 10 * time.Second}
-	var lastErr error
-	for _, a := range addrs {
-		conn, err := d.DialContext(ctx, network, net.JoinHostPort(a.IP.String(), port))
-		if err == nil {
-			return conn, nil
-		}
-		lastErr = err
-	}
-	return nil, fmt.Errorf("safeDialContext: all %d addresses for %q failed, last error: %w", len(addrs), host, lastErr)
-}
-
-// newSafeHTTPClient returns an *http.Client with the SSRF-blocking safeDialContext
-// transport and the cross-host redirect rejection policy.
-//
-// We clone http.DefaultTransport to preserve its production-ready defaults
-// (ProxyFromEnvironment, TLSHandshakeTimeout, IdleConnTimeout, connection
-// pooling, HTTP/2 support) and override only DialContext with safeDialContext.
-func newSafeHTTPClient() *http.Client {
-	transport := http.DefaultTransport.(*http.Transport).Clone()
-	transport.DialContext = safeDialContext
-	return &http.Client{
-		Timeout:       30 * time.Second,
-		Transport:     transport,
-		CheckRedirect: defaultCheckRedirect,
-	}
-}
-
 // NewClient creates a new Gitea API client.
-//
-// The client uses a safe HTTP transport by default: DNS resolution is performed
-// before connecting and any IP in a private/reserved range is rejected
-// (RFC1918, loopback, link-local, ULA, etc.). Cross-host and HTTPS→HTTP
-// redirects are also rejected.
-//
-// For tests that use httptest.NewServer (which listens on 127.0.0.1), call
-// WithUnsafeDialer() to bypass the IP check.
 func NewClient(baseURL, token string) *Client {
 	return &Client{
 		baseURL: strings.TrimRight(baseURL, "/"),
 		token:   token,
-		http:    newSafeHTTPClient(),
-	}
-}
-
-// WithUnsafeDialer returns the client configured with a plain HTTP client that
-// has no IP-level SSRF protection. It preserves the redirect-rejection policy.
-//
-// This MUST only be used in tests. Production code must never call this method.
-func (c *Client) WithUnsafeDialer() *Client {
-	c.http = &http.Client{
+		http: &http.Client{
 			Timeout:       30 * time.Second,
 			CheckRedirect: defaultCheckRedirect,
+		},
 	}
-	return c
 }
 
 // SetHTTPClient sets the underlying HTTP client used for requests.
 // This is intended for test setup only to inject mock transports; it must be
 // called before any goroutines issue requests.
 //
-// Passing nil restores the default safe client (30s timeout, IP-blocking
-// safeDialContext, and redirect-rejecting CheckRedirect policy matching NewClient).
+// Passing nil restores the default client (30s timeout + redirect-rejecting
+// CheckRedirect policy matching NewClient).
 //
 // Callers providing a non-nil client are responsible for configuring a safe
 // CheckRedirect policy. Without one, the default net/http behavior will follow
 // redirects and may forward the Authorization header to untrusted hosts.
 func (c *Client) SetHTTPClient(hc *http.Client) {
 	if hc == nil {
-		hc = newSafeHTTPClient()
+		hc = &http.Client{
+			Timeout:       30 * time.Second,
+			CheckRedirect: defaultCheckRedirect,
+		}
 	}
 	c.http = hc
 }

gitea/client_test.go

@@ -36,7 +36,7 @@ func TestGetPullRequest(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	got, err := client.GetPullRequest(context.Background(), "owner", "repo", 1)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -63,7 +63,7 @@ func TestGetPullRequestDiff(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	got, err := client.GetPullRequestDiff(context.Background(), "owner", "repo", 5)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -88,7 +88,7 @@ func TestGetCommitStatuses(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	got, err := client.GetCommitStatuses(context.Background(), "owner", "repo", "abc123")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -138,7 +138,7 @@ func TestPostReview(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", "abc123def", nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -158,7 +158,7 @@ func TestGetPullRequest_Non200(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	_, err := client.GetPullRequest(context.Background(), "owner", "repo", 999)
 	if err == nil {
 		t.Fatal("expected error for 404, got nil")
@@ -171,7 +171,7 @@ func TestGetPullRequest_BadJSON(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	_, err := client.GetPullRequest(context.Background(), "owner", "repo", 1)
 	if err == nil {
 		t.Fatal("expected error for bad JSON, got nil")
@@ -185,7 +185,7 @@ func TestPostReview_Non200(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", "", nil)
 	if err == nil {
 		t.Fatal("expected error for 403, got nil")
@@ -208,7 +208,7 @@ func TestPostReview_EmptyCommitID_OmittedFromPayload(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "ok", "", nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -226,7 +226,7 @@ func TestGetFileContent(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	got, err := client.GetFileContent(context.Background(), "owner", "repo", "CONVENTIONS.md")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -246,7 +246,7 @@ func TestGetPullRequestFiles(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	files, err := client.GetPullRequestFiles(context.Background(), "owner", "repo", 1)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -271,7 +271,7 @@ func TestGetFileContentRef(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	content, err := client.GetFileContentRef(context.Background(), "owner", "repo", "main.go", "feature-branch")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -291,7 +291,7 @@ func TestListContents(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	entries, err := client.ListContents(context.Background(), "owner", "repo", "docs")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -318,7 +318,7 @@ func TestListContents_DotPath(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	entries, err := client.ListContents(context.Background(), "owner", "repo", ".")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -343,7 +343,7 @@ func TestListContents_FilePath(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	entries, err := client.ListContents(context.Background(), "owner", "repo", "README.md")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -375,7 +375,7 @@ func TestGetAllFilesInPath_File(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	files, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "README.md")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -428,7 +428,7 @@ func TestListReviews(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	reviews, err := client.ListReviews(context.Background(), "owner", "repo", 5)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -468,7 +468,7 @@ func TestListReviews_Pagination(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	reviews, err := client.ListReviews(context.Background(), "owner", "repo", 5)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -493,7 +493,7 @@ func TestDeleteReview(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	err := client.DeleteReview(context.Background(), "owner", "repo", 5, 10)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -507,7 +507,7 @@ func TestDeleteReview_Forbidden(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	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")
@@ -536,7 +536,7 @@ func TestEditComment(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	err := client.EditComment(context.Background(), "owner", "repo", 42, "updated body")
 	if err != nil {
 		t.Fatalf("EditComment() error = %v", err)
@@ -550,7 +550,7 @@ func TestEditComment_Forbidden(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	err := client.EditComment(context.Background(), "owner", "repo", 42, "new body")
 	if err == nil {
 		t.Fatal("expected error for 403 response")
@@ -570,7 +570,7 @@ func TestGetTimelineReviewCommentID(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	id, err := client.GetTimelineReviewCommentID(context.Background(), "owner", "repo", 5, "<!-- review-bot:sonnet -->")
 	if err != nil {
 		t.Fatalf("GetTimelineReviewCommentID() error = %v", err)
@@ -586,7 +586,7 @@ func TestGetTimelineReviewCommentID_NotFound(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	_, err := client.GetTimelineReviewCommentID(context.Background(), "owner", "repo", 5, "<!-- review-bot:sonnet -->")
 	if err == nil {
 		t.Fatal("expected error when sentinel not found")
@@ -609,7 +609,7 @@ func TestGetAllFilesInPath_404FallsBackToFile(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	files, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "README.md")
 	if err != nil {
 		t.Fatalf("expected fallback to file on 404, got error: %v", err)
@@ -630,7 +630,7 @@ func TestGetAllFilesInPath_500Propagates(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	_, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "somepath")
 	if err == nil {
 		t.Fatal("expected error to propagate for 500, got nil")
@@ -652,7 +652,7 @@ func TestGetAllFilesInPath_403Propagates(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	_, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "private/stuff")
 	if err == nil {
 		t.Fatal("expected error to propagate for 403, got nil")
@@ -704,7 +704,7 @@ func TestGetAuthenticatedUser(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	login, err := client.GetAuthenticatedUser(context.Background())
 	if err != nil {
 		t.Fatalf("GetAuthenticatedUser() error = %v", err)
@@ -729,7 +729,7 @@ func TestRequestReviewer(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	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)
@@ -745,7 +745,7 @@ func TestRequestReviewer_204(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	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)
@@ -759,7 +759,7 @@ func TestRequestReviewer_Error(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	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")
@@ -779,7 +779,7 @@ func TestListReviewComments(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	comments, err := client.ListReviewComments(context.Background(), "owner", "repo", 1, 42)
 	if err != nil {
 		t.Fatalf("ListReviewComments() error = %v", err)
@@ -807,7 +807,7 @@ func TestResolveComment(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	err := client.ResolveComment(context.Background(), "owner", "repo", 99)
 	if err != nil {
 		t.Fatalf("ResolveComment() error = %v", err)
@@ -821,7 +821,7 @@ func TestResolveComment_Error(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	err := client.ResolveComment(context.Background(), "owner", "repo", 99)
 	if err == nil {
 		t.Fatal("expected error for 404 response")
@@ -870,7 +870,7 @@ func TestDoGet_RetriesOn500(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	// Use short backoff for fast tests
 	client.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond}
 
@@ -895,7 +895,7 @@ func TestDoGet_FailsAfterMaxRetries(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	// Use short backoff for fast tests
 	client.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond}
 
@@ -924,7 +924,7 @@ func TestDoGet_NoRetryOn4xx(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	_, err := client.doGet(context.Background(), server.URL+"/test")
 	if err == nil {
 		t.Fatal("expected error for 403")
@@ -952,7 +952,7 @@ func TestDoGet_RespectsContextCancellation(t *testing.T) {
 
 	ctx, cancel := context.WithCancel(context.Background())
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	// Use longer backoff to give us time to cancel during the wait
 	client.RetryBackoff = []time.Duration{100 * time.Millisecond, 100 * time.Millisecond}
 
@@ -1285,137 +1285,3 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
 		t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)")
 	}
 }
-
-// TestSafeDialContextBlocksPrivateIPs verifies that NewClient (which uses
-// safeDialContext by default) refuses to connect to private/reserved IPs.
-func TestSafeDialContextBlocksPrivateIPs(t *testing.T) {
-	// These servers listen on 127.0.0.1, so the safe dialer will block them.
-	// We use NewClient (NOT NewTestClient) to exercise the real safe dialer.
-	privateURLs := []struct {
-		name string
-		url  string
-	}{
-		{"loopback localhost", "http://localhost/"},
-		{"loopback 127.0.0.1", "http://127.0.0.1/"},
-	}
-
-	for _, tc := range privateURLs {
-		t.Run(tc.name, func(t *testing.T) {
-			c := NewClient(tc.url, "token")
-			_, err := c.GetPullRequest(context.Background(), "owner", "repo", 1)
-			if err == nil {
-				t.Errorf("expected error connecting to %s, got nil", tc.url)
-			}
-			// Error must mention SSRF/blocked, not a random network error.
-			if !strings.Contains(err.Error(), "blocked") &&
-				!strings.Contains(err.Error(), "private") &&
-				!strings.Contains(err.Error(), "loopback") &&
-				!strings.Contains(err.Error(), "reserved") {
-				t.Logf("error: %v", err)
-				// Allow other errors (connection refused, DNS) since the point
-				// is that we don't silently succeed — but prefer the explicit block message.
-			}
-		})
-	}
-}
-
-// TestWithUnsafeDialerAllowsLocalhost verifies that WithUnsafeDialer bypasses
-// the IP check, allowing tests to connect to httptest.Server (127.0.0.1).
-func TestWithUnsafeDialerAllowsLocalhost(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(`{"title":"test","body":"","head":{"sha":"abc","ref":"main"}}`))
-	}))
-	defer server.Close()
-
-	// WithUnsafeDialer should allow connecting to 127.0.0.1.
-	c := NewClient(server.URL, "token").WithUnsafeDialer()
-	pr, err := c.GetPullRequest(context.Background(), "owner", "repo", 1)
-	if err != nil {
-		t.Fatalf("unexpected error with unsafe dialer: %v", err)
-	}
-	if pr.Title != "test" {
-		t.Errorf("expected title 'test', got %q", pr.Title)
-	}
-}
-
-// TestNewClient_HasSafeTransport verifies that NewClient installs the
-// SSRF-blocking transport (i.e. Transport is not nil and DialContext is set).
-func TestNewClient_HasSafeTransport(t *testing.T) {
-	c := NewClient("https://gitea.example.com", "token")
-	if c.http.Transport == nil {
-		t.Fatal("expected Transport to be set on NewClient (safe dialer)")
-	}
-	transport, ok := c.http.Transport.(*http.Transport)
-	if !ok {
-		t.Fatalf("expected *http.Transport, got %T", c.http.Transport)
-	}
-	if transport.DialContext == nil {
-		t.Fatal("expected DialContext to be set on transport (safe dialer)")
-	}
-}
-
-// TestSetHTTPClient_NilRestoresSafeTransport verifies that SetHTTPClient(nil)
-// restores the safe transport (not just any client).
-func TestSetHTTPClient_NilRestoresSafeTransport(t *testing.T) {
-	c := NewClient("https://gitea.example.com", "token")
-	c.SetHTTPClient(&http.Client{}) // replace with plain client
-	c.SetHTTPClient(nil)            // restore
-	transport, ok := c.http.Transport.(*http.Transport)
-	if !ok {
-		t.Fatalf("expected *http.Transport after SetHTTPClient(nil), got %T", c.http.Transport)
-	}
-	if transport.DialContext == nil {
-		t.Fatal("expected DialContext to be restored after SetHTTPClient(nil)")
-	}
-}
-
-// TestNewSafeHTTPClient_PreservesDefaultTransportSettings verifies that
-// newSafeHTTPClient clones http.DefaultTransport to retain proxy support,
-// TLS handshake timeout, idle connection limits, and HTTP/2.
-func TestNewSafeHTTPClient_PreservesDefaultTransportSettings(t *testing.T) {
-	c := NewClient("https://gitea.example.com", "token")
-	transport, ok := c.http.Transport.(*http.Transport)
-	if !ok {
-		t.Fatalf("expected *http.Transport, got %T", c.http.Transport)
-	}
-
-	defaults := http.DefaultTransport.(*http.Transport)
-
-	// TLSHandshakeTimeout must be inherited (non-zero), not the zero value
-	// that a bare &http.Transport{} would have.
-	if transport.TLSHandshakeTimeout == 0 {
-		t.Error("TLSHandshakeTimeout is 0; expected inherited value from DefaultTransport")
-	}
-	if transport.TLSHandshakeTimeout != defaults.TLSHandshakeTimeout {
-		t.Errorf("TLSHandshakeTimeout = %v, want %v", transport.TLSHandshakeTimeout, defaults.TLSHandshakeTimeout)
-	}
-
-	// IdleConnTimeout must be inherited.
-	if transport.IdleConnTimeout == 0 {
-		t.Error("IdleConnTimeout is 0; expected inherited value from DefaultTransport")
-	}
-	if transport.IdleConnTimeout != defaults.IdleConnTimeout {
-		t.Errorf("IdleConnTimeout = %v, want %v", transport.IdleConnTimeout, defaults.IdleConnTimeout)
-	}
-
-	// MaxIdleConns must be inherited.
-	if transport.MaxIdleConns == 0 {
-		t.Error("MaxIdleConns is 0; expected inherited value from DefaultTransport")
-	}
-
-	// ForceAttemptHTTP2 must be inherited.
-	if !transport.ForceAttemptHTTP2 {
-		t.Error("ForceAttemptHTTP2 is false; expected true from DefaultTransport")
-	}
-
-	// Proxy must be set (ProxyFromEnvironment).
-	if transport.Proxy == nil {
-		t.Error("Proxy is nil; expected ProxyFromEnvironment from DefaultTransport")
-	}
-
-	// DialContext must be our safe dialer, not the default.
-	if transport.DialContext == nil {
-		t.Error("DialContext is nil; expected safeDialContext")
-	}
-}

gitea/diff_size_test.go

@@ -70,7 +70,7 @@ func TestGetPullRequestDiff_SizeLimits(t *testing.T) {
 			}))
 			defer server.Close()
 
-			client := NewTestClient(server.URL, "test-token")
+			client := NewClient(server.URL, "test-token")
 			client.MaxDiffSize = tt.maxDiffSize
 			client.RetryBackoff = []time.Duration{}
 

gitea/export_test.go

@@ -1,18 +0,0 @@
-// Package gitea — export_test.go exposes test helpers to test files in this
-// package. It uses `package gitea` (not `package gitea_test`) so it can access
-// unexported identifiers; Go only compiles it into the test binary, never into
-// the production binary. This is the idiomatic pattern for white-box testing
-// in Go (see net/http/export_test.go in the stdlib for the same approach).
-package gitea
-
-// NewTestClient creates a Gitea client configured for use in unit tests.
-// It bypasses the IP-level SSRF protection so that tests can connect to
-// httptest.Server instances (which listen on 127.0.0.1).
-//
-// Using the internal package gitea declaration (not gitea_test) means this
-// symbol is available to all _test.go files in this package. It is ONLY
-// compiled into the test binary; production binaries never include it.
-// Production code must use NewClient, which enables the safe dialer.
-func NewTestClient(baseURL, token string) *Client {
-	return NewClient(baseURL, token).WithUnsafeDialer()
-}

gitea/ipcheck.go

@@ -1,91 +0,0 @@
-// Package gitea provides a client for the Gitea API.
-// ipcheck.go implements IP-level SSRF protection by checking resolved addresses
-// against known blocked CIDR ranges (RFC1918, loopback, link-local, etc.).
-package gitea
-
-import (
-	"fmt"
-	"net"
-)
-
-// blockedCIDRStrings is the canonical list of CIDR strings that should never
-// be contacted by review-bot. See IsBlockedIP for the full list of covered
-// address families.
-//
-// These are hard-coded literals: any parse failure is a programming error.
-// Validity is verified by TestBlockedCIDRsValid in ipcheck_test.go.
-var blockedCIDRStrings = []string{
-	// IPv4 loopback
-	"127.0.0.0/8",
-	// IPv4 unspecified / "this network"
-	"0.0.0.0/8",
-	// RFC1918 private ranges
-	"10.0.0.0/8",
-	"172.16.0.0/12",
-	"192.168.0.0/16",
-	// IPv4 link-local (APIPA, also used by AWS instance metadata 169.254.169.254)
-	"169.254.0.0/16",
-	// IPv4 shared address space (RFC6598, carrier-grade NAT)
-	"100.64.0.0/10",
-	// IPv4 multicast
-	"224.0.0.0/4",
-	// IPv4 reserved / broadcast
-	"240.0.0.0/4",
-	// IPv6 loopback
-	"::1/128",
-	// IPv6 unspecified
-	"::/128",
-	// IPv6 link-local
-	"fe80::/10",
-	// IPv6 unique local (ULA) — RFC4193
-	"fc00::/7",
-	// IPv6 multicast
-	"ff00::/8",
-}
-
-// blockedCIDRs is the parsed form of blockedCIDRStrings.
-// Any entry that fails to parse is recorded in blockedCIDRParseErrors instead
-// of panicking; tests verify this slice is always empty via TestBlockedCIDRsValid.
-var (
-	blockedCIDRs           []*net.IPNet
-	blockedCIDRParseErrors []string
-)
-
-func init() {
-	blockedCIDRs = make([]*net.IPNet, 0, len(blockedCIDRStrings))
-	for _, r := range blockedCIDRStrings {
-		_, cidr, err := net.ParseCIDR(r)
-		if err != nil {
-			// Record the error rather than panicking; TestBlockedCIDRsValid
-			// will catch this during tests, and the CI build will fail.
-			blockedCIDRParseErrors = append(blockedCIDRParseErrors,
-				fmt.Sprintf("ipcheck: invalid built-in CIDR %q: %v", r, err))
-			continue
-		}
-		blockedCIDRs = append(blockedCIDRs, cidr)
-	}
-}
-
-// IsBlockedIP reports whether ip is in a blocked address range.
-// It is exported for use by the validate-url subcommand and tests outside
-// this package.
-//
-// IPv6-mapped IPv4 addresses (e.g. ::ffff:192.168.1.1) are normalized to their
-// IPv4 form before checking so that IPv4 CIDRs catch them.
-//
-// Based on:
-//   - RFC1918 private ranges
-//   - RFC5735 / RFC4193 special-use IPv4/IPv6 ranges
-//   - RFC4291 IPv6 link-local / loopback
-func IsBlockedIP(ip net.IP) bool {
-	// Normalize IPv6-mapped IPv4 addresses (::ffff:x.x.x.x) to plain IPv4.
-	if v4 := ip.To4(); v4 != nil {
-		ip = v4
-	}
-	for _, cidr := range blockedCIDRs {
-		if cidr.Contains(ip) {
-			return true
-		}
-	}
-	return false
-}

gitea/ipcheck_test.go

@@ -1,144 +0,0 @@
-package gitea
-
-import (
-	"net"
-	"testing"
-)
-
-func TestIsBlockedIP(t *testing.T) {
-	blocked := []struct {
-		name string
-		ip   string
-	}{
-		// IPv4 loopback
-		{"loopback 127.0.0.1", "127.0.0.1"},
-		{"loopback 127.0.0.2", "127.0.0.2"},
-		{"loopback 127.255.255.255", "127.255.255.255"},
-		// IPv4 unspecified
-		{"unspecified 0.0.0.0", "0.0.0.0"},
-		{"unspecified 0.1.2.3", "0.1.2.3"},
-		// RFC1918
-		{"RFC1918 10.0.0.1", "10.0.0.1"},
-		{"RFC1918 10.255.255.255", "10.255.255.255"},
-		{"RFC1918 172.16.0.1", "172.16.0.1"},
-		{"RFC1918 172.31.255.255", "172.31.255.255"},
-		{"RFC1918 192.168.0.1", "192.168.0.1"},
-		{"RFC1918 192.168.255.255", "192.168.255.255"},
-		// Link-local (APIPA / AWS metadata)
-		{"link-local 169.254.0.1", "169.254.0.1"},
-		{"link-local 169.254.169.254", "169.254.169.254"},
-		// Shared address space (carrier-grade NAT)
-		{"CGN 100.64.0.1", "100.64.0.1"},
-		{"CGN 100.127.255.255", "100.127.255.255"},
-		// Multicast
-		{"multicast 224.0.0.1", "224.0.0.1"},
-		{"multicast 239.255.255.255", "239.255.255.255"},
-		// Reserved
-		{"reserved 240.0.0.1", "240.0.0.1"},
-		{"broadcast 255.255.255.255", "255.255.255.255"},
-		// IPv6 loopback
-		{"IPv6 loopback ::1", "::1"},
-		// IPv6 unspecified
-		{"IPv6 unspecified ::", "::"},
-		// IPv6 link-local
-		{"IPv6 link-local fe80::1", "fe80::1"},
-		{"IPv6 link-local fe80::dead:beef", "fe80::dead:beef"},
-		// IPv6 ULA
-		{"IPv6 ULA fc00::1", "fc00::1"},
-		{"IPv6 ULA fd00::1", "fd00::1"},
-		// IPv6 multicast
-		{"IPv6 multicast ff02::1", "ff02::1"},
-	}
-
-	for _, tc := range blocked {
-		t.Run(tc.name, func(t *testing.T) {
-			ip := net.ParseIP(tc.ip)
-			if ip == nil {
-				t.Fatalf("failed to parse IP %q", tc.ip)
-			}
-			if !IsBlockedIP(ip) {
-				t.Errorf("IsBlockedIP(%q) = false, want true", tc.ip)
-			}
-		})
-	}
-
-	allowed := []struct {
-		name string
-		ip   string
-	}{
-		{"public 8.8.8.8", "8.8.8.8"},
-		{"public 1.1.1.1", "1.1.1.1"},
-		{"public 198.51.100.1", "198.51.100.1"}, // RFC5737 TEST-NET-2 — a documentation-only range;
-					// not assigned to any real host, but intentionally left unblocked here because
-					// it has no special routing treatment (unlike RFC1918/loopback/link-local) and
-					// blocking it would require tracking every RFC5737 range without meaningful
-					// security benefit (no server should ever listen on a TEST-NET address).
-		{"public 151.101.1.1", "151.101.1.1"},        // Fastly
-		{"public IPv6 2001:4860:4860::8888", "2001:4860:4860::8888"}, // Google DNS
-		{"public IPv6 2606:4700:4700::1111", "2606:4700:4700::1111"}, // Cloudflare DNS
-	}
-
-	for _, tc := range allowed {
-		t.Run(tc.name, func(t *testing.T) {
-			ip := net.ParseIP(tc.ip)
-			if ip == nil {
-				t.Fatalf("failed to parse IP %q", tc.ip)
-			}
-			if IsBlockedIP(ip) {
-				t.Errorf("IsBlockedIP(%q) = true, want false", tc.ip)
-			}
-		})
-	}
-}
-
-func TestIsBlockedIPv6MappedIPv4(t *testing.T) {
-	// ::ffff:192.168.1.1 is an IPv6-mapped IPv4 address — should be blocked as RFC1918.
-	// Construct it manually as a 16-byte IP.
-	mapped := net.IP{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff, 192, 168, 1, 1}
-	if !IsBlockedIP(mapped) {
-		t.Errorf("IsBlockedIP(::ffff:192.168.1.1) = false, want true (IPv6-mapped IPv4 must be normalized)")
-	}
-
-	// ::ffff:8.8.8.8 — IPv6-mapped public IP — should be allowed.
-	mappedPublic := net.IP{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff, 8, 8, 8, 8}
-	if IsBlockedIP(mappedPublic) {
-		t.Errorf("IsBlockedIP(::ffff:8.8.8.8) = true, want false")
-	}
-}
-
-func TestIsBlockedIPEdgeCases(t *testing.T) {
-	// The boundary between RFC1918 and public ranges.
-	// 172.15.255.255 is NOT private (just below 172.16.0.0/12).
-	notPrivate := net.ParseIP("172.15.255.255")
-	if IsBlockedIP(notPrivate) {
-		t.Errorf("IsBlockedIP(172.15.255.255) = true, want false (outside 172.16.0.0/12)")
-	}
-	// 172.32.0.0 is NOT private (just above 172.31.255.255).
-	notPrivate2 := net.ParseIP("172.32.0.0")
-	if IsBlockedIP(notPrivate2) {
-		t.Errorf("IsBlockedIP(172.32.0.0) = true, want false (outside 172.16.0.0/12)")
-	}
-	// CGN: 100.63.255.255 is NOT in 100.64.0.0/10.
-	notCGN := net.ParseIP("100.63.255.255")
-	if IsBlockedIP(notCGN) {
-		t.Errorf("IsBlockedIP(100.63.255.255) = true, want false (outside 100.64.0.0/10)")
-	}
-	// CGN: 100.128.0.0 is NOT in 100.64.0.0/10.
-	notCGN2 := net.ParseIP("100.128.0.0")
-	if IsBlockedIP(notCGN2) {
-		t.Errorf("IsBlockedIP(100.128.0.0) = true, want false (outside 100.64.0.0/10)")
-	}
-}
-
-// TestBlockedCIDRsValid verifies that all entries in blockedCIDRStrings parse
-// successfully. This catches programming errors in the CIDR list without
-// requiring a startup panic. The init() function records parse failures in
-// blockedCIDRParseErrors rather than panicking; this test makes those failures
-// visible as test failures during CI.
-func TestBlockedCIDRsValid(t *testing.T) {
-	if len(blockedCIDRParseErrors) > 0 {
-		for _, msg := range blockedCIDRParseErrors {
-			t.Errorf("CIDR parse error: %s", msg)
-		}
-	}
-}

gitea/post_review_comments_test.go

@@ -31,7 +31,7 @@ func TestPostReview_WithComments(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	comments := []ReviewComment{
 		{Path: "main.go", NewPosition: 42, Body: "[MAJOR] Something bad"},
 		{Path: "util.go", NewPosition: 10, Body: "[MINOR] Style issue"},
@@ -71,7 +71,7 @@ func TestPostReview_NilComments(t *testing.T) {
 	}))
 	defer server.Close()
 
-	client := NewTestClient(server.URL, "test-token")
+	client := NewClient(server.URL, "test-token")
 	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "all good", "", nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)

github/methods.go

@@ -1,552 +0,0 @@
-// Package github provides a client for the GitHub API.
-// This file contains the higher-level PR/review methods built on top of the
-// HTTP client in client.go. All methods use GitHub REST API v3 paths.
-package github
-
-import (
-	"bytes"
-	"context"
-	"encoding/base64"
-	"encoding/json"
-	"fmt"
-	"io"
-	"log/slog"
-	"net/http"
-	"net/url"
-	"strings"
-)
-
-// PullRequest holds relevant PR metadata.
-type PullRequest struct {
-	Title string `json:"title"`
-	Body  string `json:"body"`
-	Head  struct {
-		Sha string `json:"sha"`
-		Ref string `json:"ref"`
-	} `json:"head"`
-}
-
-// CommitStatus represents a single CI status entry.
-// GitHub uses "state" (success/failure/pending/error) unlike Gitea's "status".
-type CommitStatus struct {
-	State       string `json:"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 "path" + "position" or "line" for positioning.
-type ReviewComment struct {
-	ID   int64  `json:"id,omitempty"`
-	Path string `json:"path"`
-	// Position is the line position in the diff (used when submitting).
-	// Side+Line is an alternative for GitHub (line in the file), but
-	// we mirror the Gitea interface using NewPosition mapped to position.
-	Position int64  `json:"position,omitempty"`
-	Body     string `json:"body"`
-}
-
-// 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"
-}
-
-// Review represents a pull request review.
-type Review struct {
-	ID   int64  `json:"id"`
-	Body string `json:"body"`
-	User struct {
-		Login string `json:"login"`
-	} `json:"user"`
-	State    string `json:"state"`
-	CommitID string `json:"commit_id"`
-}
-
-// 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.v3.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 at 30 files/page (max 3000 files total).
-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's combined status endpoint returns the most-relevant state per context.
-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: %w", err)
-		}
-		var batch []CommitStatus
-		if err := json.Unmarshal(body, &batch); err != nil {
-			return nil, fmt.Errorf("parse statuses JSON: %w", err)
-		}
-		all = append(all, batch...)
-		if len(batch) < perPage {
-			break
-		}
-	}
-	return all, nil
-}
-
-// GetFileContent fetches a file from the default branch of a repo.
-// GitHub's contents API returns base64-encoded content.
-func (c *Client) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
-	return c.GetFileContentRef(ctx, owner, repo, filepath, "")
-}
-
-// GetFileContentRef fetches a file from a specific ref (branch/tag/sha) in a repo.
-func (c *Client) GetFileContentRef(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)
-	}
-	// GitHub returns JSON with base64-encoded content
-	var result struct {
-		Content  string `json:"content"`
-		Encoding string `json:"encoding"`
-	}
-	if err := json.Unmarshal(body, &result); err != nil {
-		return "", fmt.Errorf("parse file content JSON: %w", err)
-	}
-	if result.Encoding != "base64" {
-		return "", fmt.Errorf("unexpected encoding %q for file %s", result.Encoding, filepath)
-	}
-	// GitHub wraps base64 content in newlines — strip them before decoding
-	cleaned := strings.ReplaceAll(result.Content, "\n", "")
-	decoded, err := base64.StdEncoding.DecodeString(cleaned)
-	if err != nil {
-		return "", fmt.Errorf("decode file content: %w", err)
-	}
-	return string(decoded), nil
-}
-
-// ListContents lists files and directories at a given path in a repo.
-// Pass an empty path to list the repository root.
-func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) {
-	if path == "." {
-		path = ""
-	}
-	var reqURL string
-	if 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 also returns a single object when path is a file
-		var single ContentEntry
-		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{single}
-	}
-	return entries, nil
-}
-
-// GetAllFilesInPath recursively fetches all file contents under a path.
-// If the path is a file, returns just that file's content.
-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) {
-			// Try fetching as a file 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
-		}
-		return nil, fmt.Errorf("list contents %q: %w", path, err)
-	}
-
-	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
-}
-
-// PostReview submits a review to a PR and returns the created review.
-// event should be "APPROVE", "REQUEST_CHANGES", or "COMMENT".
-// commitID anchors the review to a specific commit SHA.
-// comments are optional inline comments.
-//
-// Note: GitHub uses "APPROVE" (not "APPROVED") for the event name.
-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)
-
-	// GitHub uses "APPROVE" not "APPROVED", "REQUEST_CHANGES" and "COMMENT" match
-	ghEvent := event
-	if event == "APPROVED" {
-		ghEvent = "APPROVE"
-	}
-
-	payload := struct {
-		Body     string          `json:"body"`
-		Event    string          `json:"event"`
-		CommitID string          `json:"commit_id,omitempty"`
-		Comments []ReviewComment `json:"comments,omitempty"`
-	}{
-		Body:     body,
-		Event:    ghEvent,
-		CommitID: commitID,
-		Comments: comments,
-	}
-
-	data, err := json.Marshal(payload)
-	if err != nil {
-		return nil, fmt.Errorf("marshal review payload: %w", err)
-	}
-
-	req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data))
-	if err != nil {
-		return nil, fmt.Errorf("create review request: %w", err)
-	}
-	req.Header.Set("Authorization", "Bearer "+c.token)
-	req.Header.Set("Content-Type", "application/json")
-	req.Header.Set("Accept", "application/vnd.github+json")
-
-	resp, err := c.httpClient.Do(req)
-	if err != nil {
-		return nil, fmt.Errorf("post review: %w", err)
-	}
-	defer resp.Body.Close()
-
-	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		respBody, _ := io.ReadAll(io.LimitReader(resp.Body, 64*1024))
-		return nil, fmt.Errorf("post review failed (status %d): %s", resp.StatusCode, string(respBody))
-	}
-
-	respBody, err := io.ReadAll(io.LimitReader(resp.Body, 10*1024*1024))
-	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
-}
-
-// ListReviews returns all reviews on a pull request.
-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 deletes a review by ID.
-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)
-
-	req, err := http.NewRequestWithContext(ctx, http.MethodDelete, reqURL, nil)
-	if err != nil {
-		return fmt.Errorf("create delete request: %w", err)
-	}
-	req.Header.Set("Authorization", "Bearer "+c.token)
-	req.Header.Set("Accept", "application/vnd.github+json")
-
-	resp, err := c.httpClient.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(io.LimitReader(resp.Body, 256))
-		return fmt.Errorf("delete review failed (status %d): %s", resp.StatusCode, string(respBody))
-	}
-	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/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 on GitHub — requesting an already-requested reviewer succeats.
-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)
-	}
-
-	req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data))
-	if err != nil {
-		return fmt.Errorf("create reviewer request: %w", err)
-	}
-	req.Header.Set("Authorization", "Bearer "+c.token)
-	req.Header.Set("Content-Type", "application/json")
-	req.Header.Set("Accept", "application/vnd.github+json")
-
-	resp, err := c.httpClient.Do(req)
-	if err != nil {
-		return fmt.Errorf("request reviewer: %w", err)
-	}
-	defer resp.Body.Close()
-
-	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		respBody, _ := io.ReadAll(io.LimitReader(resp.Body, 256))
-		return fmt.Errorf("request reviewer failed (status %d): %s", resp.StatusCode, string(respBody))
-	}
-	return nil
-}
-
-// EditComment updates the body of a PR review comment.
-// GitHub uses PATCH /repos/{owner}/{repo}/pulls/comments/{comment_id}.
-func (c *Client) EditComment(ctx context.Context, owner, repo string, commentID int64, newBody string) error {
-	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/comments/%d",
-		c.baseURL,
-		url.PathEscape(owner),
-		url.PathEscape(repo),
-		commentID)
-
-	payload := struct {
-		Body string `json:"body"`
-	}{Body: newBody}
-	data, err := json.Marshal(payload)
-	if err != nil {
-		return fmt.Errorf("marshal edit payload: %w", err)
-	}
-
-	req, err := http.NewRequestWithContext(ctx, http.MethodPatch, reqURL, bytes.NewReader(data))
-	if err != nil {
-		return fmt.Errorf("create edit request: %w", err)
-	}
-	req.Header.Set("Authorization", "Bearer "+c.token)
-	req.Header.Set("Content-Type", "application/json")
-	req.Header.Set("Accept", "application/vnd.github+json")
-
-	resp, err := c.httpClient.Do(req)
-	if err != nil {
-		return fmt.Errorf("edit comment: %w", err)
-	}
-	defer resp.Body.Close()
-
-	if resp.StatusCode != http.StatusOK {
-		body, _ := io.ReadAll(io.LimitReader(resp.Body, 256))
-		return fmt.Errorf("edit comment failed (status %d): %s", resp.StatusCode, body)
-	}
-	return nil
-}
-
-// ListReviewComments returns the inline comments attached to a specific review.
-func (c *Client) ListReviewComments(ctx context.Context, owner, repo string, prNumber int, reviewID int64) ([]ReviewComment, error) {
-	const perPage = 100
-	var all []ReviewComment
-	for page := 1; ; page++ {
-		reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d/comments?per_page=%d&page=%d",
-			c.baseURL,
-			url.PathEscape(owner),
-			url.PathEscape(repo),
-			prNumber,
-			reviewID,
-			perPage,
-			page)
-		body, err := c.doGet(ctx, reqURL)
-		if err != nil {
-			return nil, fmt.Errorf("list review comments (page %d): %w", page, err)
-		}
-		var batch []ReviewComment
-		if err := json.Unmarshal(body, &batch); err != nil {
-			return nil, fmt.Errorf("parse review comments (page %d): %w", page, err)
-		}
-		all = append(all, batch...)
-		if len(batch) < perPage {
-			break
-		}
-	}
-	return all, nil
-}
-
-// ResolveComment is a no-op on GitHub. GitHub does not support resolving
-// individual review comments via the REST API (only via the GraphQL API).
-// This method exists to satisfy the VCSClient interface.
-func (c *Client) ResolveComment(_ context.Context, _, _ string, _ int64) error {
-	return nil
-}
-
-// GetTimelineReviewCommentIDForReview finds the timeline comment ID for a review.
-// GitHub doesn't have a direct timeline event endpoint for reviews the way Gitea does.
-// This is primarily used by the supersede path (EditComment + ResolveComment). On GitHub,
-// we return the review ID itself. Note that EditComment on GitHub uses the
-// /pulls/comments/{id} endpoint (for inline review comments), which does not
-// apply to review bodies — the supersede EditComment call will 404 and be
-// logged as a warning. This is a known limitation; the review is still posted
-// correctly regardless.
-func (c *Client) GetTimelineReviewCommentIDForReview(_ context.Context, _, _ string, _ int, reviewID int64) (int64, error) {
-	return reviewID, nil
-}
-
-// escapePath escapes each path segment individually while preserving slashes.
-// This avoids double-escaping the forward slash separator in file paths.
-// NOTE: Intentionally duplicated from gitea/client.go to keep the packages independent.
-func escapePath(p string) string {
-	parts := strings.Split(p, "/")
-	escaped := make([]string, len(parts))
-	for i, part := range parts {
-		escaped[i] = url.PathEscape(part)
-	}
-	return strings.Join(escaped, "/")
-}

github/methods_test.go

@@ -1,518 +0,0 @@
-package github
-
-import (
-	"context"
-	"encoding/base64"
-	"encoding/json"
-	"fmt"
-	"net/http"
-	"net/http/httptest"
-	"testing"
-)
-
-// newTestClient creates a Client pointed at the test server.
-func newTestClient(srv *httptest.Server) *Client {
-	return NewClient("test-token", srv.URL, AllowInsecureHTTPForTest())
-}
-
-func TestGetPullRequest(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.Method != http.MethodGet || r.URL.Path != "/repos/owner/repo/pulls/42" {
-			http.Error(w, "unexpected", http.StatusNotFound)
-			return
-		}
-		if got := r.Header.Get("Authorization"); got != "Bearer test-token" {
-			http.Error(w, "unauthorized", http.StatusUnauthorized)
-			return
-		}
-		w.Header().Set("Content-Type", "application/json")
-		fmt.Fprintln(w, `{"title":"Fix bug","body":"Body text","head":{"sha":"abc1234","ref":"fix/bug"}}`)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	pr, err := c.GetPullRequest(context.Background(), "owner", "repo", 42)
-	if err != nil {
-		t.Fatalf("GetPullRequest: %v", err)
-	}
-	if pr.Title != "Fix bug" {
-		t.Errorf("Title = %q, want %q", pr.Title, "Fix bug")
-	}
-	if pr.Head.Sha != "abc1234" {
-		t.Errorf("Head.Sha = %q, want %q", pr.Head.Sha, "abc1234")
-	}
-	if pr.Head.Ref != "fix/bug" {
-		t.Errorf("Head.Ref = %q, want %q", pr.Head.Ref, "fix/bug")
-	}
-}
-
-func TestGetPullRequest_NotFound(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		http.Error(w, `{"message":"Not Found"}`, http.StatusNotFound)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	_, err := c.GetPullRequest(context.Background(), "owner", "repo", 99)
-	if err == nil {
-		t.Fatal("expected error for 404, got nil")
-	}
-	if !IsNotFound(err) {
-		t.Errorf("expected IsNotFound error, got %v", err)
-	}
-}
-
-func TestGetPullRequestDiff(t *testing.T) {
-	diffText := "diff --git a/foo.go b/foo.go\n@@ -1,1 +1,2 @@\n+added"
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path != "/repos/owner/repo/pulls/1" {
-			http.Error(w, "unexpected", http.StatusNotFound)
-			return
-		}
-		if r.Header.Get("Accept") != "application/vnd.github.v3.diff" {
-			http.Error(w, "wrong accept", http.StatusNotAcceptable)
-			return
-		}
-		w.Header().Set("Content-Type", "text/plain")
-		fmt.Fprint(w, diffText)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	got, err := c.GetPullRequestDiff(context.Background(), "owner", "repo", 1)
-	if err != nil {
-		t.Fatalf("GetPullRequestDiff: %v", err)
-	}
-	if got != diffText {
-		t.Errorf("diff = %q, want %q", got, diffText)
-	}
-}
-
-func TestGetPullRequestFiles(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path != "/repos/owner/repo/pulls/5/files" {
-			http.Error(w, "unexpected", http.StatusNotFound)
-			return
-		}
-		w.Header().Set("Content-Type", "application/json")
-		fmt.Fprintln(w, `[{"filename":"foo.go","status":"added"},{"filename":"bar.go","status":"modified"}]`)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	files, err := c.GetPullRequestFiles(context.Background(), "owner", "repo", 5)
-	if err != nil {
-		t.Fatalf("GetPullRequestFiles: %v", err)
-	}
-	if len(files) != 2 {
-		t.Fatalf("len(files) = %d, want 2", len(files))
-	}
-	if files[0].Filename != "foo.go" || files[0].Status != "added" {
-		t.Errorf("files[0] = %+v", files[0])
-	}
-}
-
-func TestGetCommitStatuses(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path != "/repos/owner/repo/commits/deadbeef/statuses" {
-			http.Error(w, "unexpected", http.StatusNotFound)
-			return
-		}
-		w.Header().Set("Content-Type", "application/json")
-		fmt.Fprintln(w, `[{"state":"success","context":"ci/test","description":"Tests passed","target_url":"https://ci.example.com"}]`)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	statuses, err := c.GetCommitStatuses(context.Background(), "owner", "repo", "deadbeef")
-	if err != nil {
-		t.Fatalf("GetCommitStatuses: %v", err)
-	}
-	if len(statuses) != 1 {
-		t.Fatalf("len(statuses) = %d, want 1", len(statuses))
-	}
-	if statuses[0].State != "success" {
-		t.Errorf("State = %q, want success", statuses[0].State)
-	}
-	if statuses[0].Context != "ci/test" {
-		t.Errorf("Context = %q, want ci/test", statuses[0].Context)
-	}
-}
-
-func TestGetFileContent(t *testing.T) {
-	content := "package main\nfunc main() {}\n"
-	encoded := base64.StdEncoding.EncodeToString([]byte(content))
-	// GitHub wraps base64 in newlines every 60 chars
-	var chunked string
-	for i := 0; i < len(encoded); i += 60 {
-		end := i + 60
-		if end > len(encoded) {
-			end = len(encoded)
-		}
-		chunked += encoded[i:end] + "\n"
-	}
-
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path != "/repos/owner/repo/contents/main.go" {
-			http.Error(w, "unexpected path: "+r.URL.Path, http.StatusNotFound)
-			return
-		}
-		w.Header().Set("Content-Type", "application/json")
-		resp := map[string]string{
-			"content":  chunked,
-			"encoding": "base64",
-		}
-		if err := json.NewEncoder(w).Encode(resp); err != nil {
-			http.Error(w, err.Error(), http.StatusInternalServerError)
-		}
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	got, err := c.GetFileContent(context.Background(), "owner", "repo", "main.go")
-	if err != nil {
-		t.Fatalf("GetFileContent: %v", err)
-	}
-	if got != content {
-		t.Errorf("content = %q, want %q", got, content)
-	}
-}
-
-func TestGetFileContentRef(t *testing.T) {
-	content := "hello world"
-	encoded := base64.StdEncoding.EncodeToString([]byte(content))
-
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path != "/repos/owner/repo/contents/README.md" {
-			http.Error(w, "unexpected", http.StatusNotFound)
-			return
-		}
-		if r.URL.Query().Get("ref") != "abc123" {
-			http.Error(w, "missing ref", http.StatusBadRequest)
-			return
-		}
-		w.Header().Set("Content-Type", "application/json")
-		resp := map[string]string{"content": encoded + "\n", "encoding": "base64"}
-		if err := json.NewEncoder(w).Encode(resp); err != nil {
-			http.Error(w, err.Error(), http.StatusInternalServerError)
-		}
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	got, err := c.GetFileContentRef(context.Background(), "owner", "repo", "README.md", "abc123")
-	if err != nil {
-		t.Fatalf("GetFileContentRef: %v", err)
-	}
-	if got != content {
-		t.Errorf("content = %q, want %q", got, content)
-	}
-}
-
-func TestListContents(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path == "/repos/owner/repo/contents" {
-			w.Header().Set("Content-Type", "application/json")
-			fmt.Fprintln(w, `[{"name":"README.md","path":"README.md","type":"file"},{"name":"src","path":"src","type":"dir"}]`)
-			return
-		}
-		http.Error(w, "unexpected: "+r.URL.Path, http.StatusNotFound)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	entries, err := c.ListContents(context.Background(), "owner", "repo", "")
-	if err != nil {
-		t.Fatalf("ListContents: %v", err)
-	}
-	if len(entries) != 2 {
-		t.Fatalf("len(entries) = %d, want 2", len(entries))
-	}
-	if entries[0].Name != "README.md" || entries[0].Type != "file" {
-		t.Errorf("entries[0] = %+v", entries[0])
-	}
-	if entries[1].Name != "src" || entries[1].Type != "dir" {
-		t.Errorf("entries[1] = %+v", entries[1])
-	}
-}
-
-func TestListContents_Dot(t *testing.T) {
-	// "." should be treated as "" (root)
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path == "/repos/owner/repo/contents" {
-			w.Header().Set("Content-Type", "application/json")
-			fmt.Fprintln(w, `[]`)
-			return
-		}
-		http.Error(w, "unexpected: "+r.URL.Path, http.StatusNotFound)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	entries, err := c.ListContents(context.Background(), "owner", "repo", ".")
-	if err != nil {
-		t.Fatalf("ListContents: %v", err)
-	}
-	if len(entries) != 0 {
-		t.Errorf("expected empty entries, got %d", len(entries))
-	}
-}
-
-func TestPostReview(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.Method != http.MethodPost || r.URL.Path != "/repos/owner/repo/pulls/10/reviews" {
-			http.Error(w, "unexpected", http.StatusNotFound)
-			return
-		}
-		var payload struct {
-			Body     string `json:"body"`
-			Event    string `json:"event"`
-			CommitID string `json:"commit_id"`
-		}
-		if err := json.NewDecoder(r.Body).Decode(&payload); err != nil {
-			http.Error(w, "bad body", http.StatusBadRequest)
-			return
-		}
-		// Verify APPROVED is normalized to APPROVE
-		if payload.Event != "APPROVE" {
-			http.Error(w, fmt.Sprintf("expected APPROVE, got %s", payload.Event), http.StatusBadRequest)
-			return
-		}
-		w.Header().Set("Content-Type", "application/json")
-		w.WriteHeader(http.StatusOK)
-		fmt.Fprintf(w, `{"id":99,"body":%q,"user":{"login":"bot"},"state":"APPROVED","commit_id":%q}`, payload.Body, payload.CommitID)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	// Pass "APPROVED" (Gitea-style) — should be normalized to APPROVE
-	review, err := c.PostReview(context.Background(), "owner", "repo", 10, "APPROVED", "Looks good", "abc123", nil)
-	if err != nil {
-		t.Fatalf("PostReview: %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 TestListReviews(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path != "/repos/owner/repo/pulls/7/reviews" {
-			http.Error(w, "unexpected", http.StatusNotFound)
-			return
-		}
-		w.Header().Set("Content-Type", "application/json")
-		fmt.Fprintln(w, `[{"id":1,"body":"LGTM","user":{"login":"alice"},"state":"APPROVED","commit_id":"abc"}]`)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	reviews, err := c.ListReviews(context.Background(), "owner", "repo", 7)
-	if err != nil {
-		t.Fatalf("ListReviews: %v", err)
-	}
-	if len(reviews) != 1 {
-		t.Fatalf("len(reviews) = %d, want 1", len(reviews))
-	}
-	if reviews[0].User.Login != "alice" {
-		t.Errorf("User.Login = %q, want alice", reviews[0].User.Login)
-	}
-}
-
-func TestGetAuthenticatedUser(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path != "/user" {
-			http.Error(w, "unexpected", http.StatusNotFound)
-			return
-		}
-		w.Header().Set("Content-Type", "application/json")
-		fmt.Fprintln(w, `{"login":"sonnet-review"}`)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	login, err := c.GetAuthenticatedUser(context.Background())
-	if err != nil {
-		t.Fatalf("GetAuthenticatedUser: %v", err)
-	}
-	if login != "sonnet-review" {
-		t.Errorf("login = %q, want sonnet-review", login)
-	}
-}
-
-func TestResolveComment_NoOp(t *testing.T) {
-	// ResolveComment is a no-op on GitHub — should not make any HTTP call.
-	callCount := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		callCount++
-		http.Error(w, "unexpected call", http.StatusInternalServerError)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	if err := c.ResolveComment(context.Background(), "owner", "repo", 123); err != nil {
-		t.Errorf("ResolveComment: %v (expected no-op)", err)
-	}
-	if callCount != 0 {
-		t.Errorf("expected no HTTP calls, got %d", callCount)
-	}
-}
-
-func TestGetTimelineReviewCommentIDForReview(t *testing.T) {
-	// Should return reviewID unchanged without making HTTP calls.
-	callCount := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		callCount++
-		http.Error(w, "unexpected", http.StatusInternalServerError)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	got, err := c.GetTimelineReviewCommentIDForReview(context.Background(), "owner", "repo", 5, 42)
-	if err != nil {
-		t.Fatalf("GetTimelineReviewCommentIDForReview: %v", err)
-	}
-	if got != 42 {
-		t.Errorf("got %d, want 42", got)
-	}
-	if callCount != 0 {
-		t.Errorf("expected no HTTP calls, got %d", callCount)
-	}
-}
-
-func TestRequestReviewer(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.Method != http.MethodPost || r.URL.Path != "/repos/owner/repo/pulls/3/requested_reviewers" {
-			http.Error(w, "unexpected", http.StatusNotFound)
-			return
-		}
-		var payload struct {
-			Reviewers []string `json:"reviewers"`
-		}
-		if err := json.NewDecoder(r.Body).Decode(&payload); err != nil || len(payload.Reviewers) == 0 {
-			http.Error(w, "bad body", http.StatusBadRequest)
-			return
-		}
-		if payload.Reviewers[0] != "bot-user" {
-			http.Error(w, fmt.Sprintf("unexpected reviewer %q", payload.Reviewers[0]), http.StatusBadRequest)
-			return
-		}
-		w.WriteHeader(http.StatusCreated)
-		fmt.Fprintln(w, `{}`)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	if err := c.RequestReviewer(context.Background(), "owner", "repo", 3, "bot-user"); err != nil {
-		t.Errorf("RequestReviewer: %v", err)
-	}
-}
-
-func TestEditComment(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.Method != http.MethodPatch || r.URL.Path != "/repos/owner/repo/pulls/comments/55" {
-			http.Error(w, "unexpected", http.StatusNotFound)
-			return
-		}
-		var payload struct {
-			Body string `json:"body"`
-		}
-		if err := json.NewDecoder(r.Body).Decode(&payload); err != nil {
-			http.Error(w, "bad body", http.StatusBadRequest)
-			return
-		}
-		if payload.Body != "updated body" {
-			http.Error(w, "wrong body", http.StatusBadRequest)
-			return
-		}
-		w.Header().Set("Content-Type", "application/json")
-		w.WriteHeader(http.StatusOK)
-		fmt.Fprintln(w, `{"id":55,"body":"updated body"}`)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	if err := c.EditComment(context.Background(), "owner", "repo", 55, "updated body"); err != nil {
-		t.Errorf("EditComment: %v", err)
-	}
-}
-
-func TestListReviewComments(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.URL.Path != "/repos/owner/repo/pulls/9/reviews/20/comments" {
-			http.Error(w, "unexpected", http.StatusNotFound)
-			return
-		}
-		w.Header().Set("Content-Type", "application/json")
-		fmt.Fprintln(w, `[{"id":100,"path":"main.go","position":5,"body":"Needs fix"}]`)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	comments, err := c.ListReviewComments(context.Background(), "owner", "repo", 9, 20)
-	if err != nil {
-		t.Fatalf("ListReviewComments: %v", err)
-	}
-	if len(comments) != 1 {
-		t.Fatalf("len(comments) = %d, want 1", len(comments))
-	}
-	if comments[0].Path != "main.go" {
-		t.Errorf("Path = %q, want main.go", comments[0].Path)
-	}
-	if comments[0].Position != 5 {
-		t.Errorf("Position = %d, want 5", comments[0].Position)
-	}
-}
-
-func TestDeleteReview(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if r.Method != http.MethodDelete || r.URL.Path != "/repos/owner/repo/pulls/7/reviews/11" {
-			http.Error(w, "unexpected", http.StatusNotFound)
-			return
-		}
-		w.WriteHeader(http.StatusNoContent)
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	if err := c.DeleteReview(context.Background(), "owner", "repo", 7, 11); err != nil {
-		t.Errorf("DeleteReview: %v", err)
-	}
-}
-
-func TestGetAllFilesInPath(t *testing.T) {
-	content := "file content"
-	encoded := base64.StdEncoding.EncodeToString([]byte(content))
-
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		switch r.URL.Path {
-		case "/repos/owner/repo/contents/patterns":
-			w.Header().Set("Content-Type", "application/json")
-			fmt.Fprintln(w, `[{"name":"patterns.md","path":"patterns/patterns.md","type":"file"}]`)
-		case "/repos/owner/repo/contents/patterns/patterns.md":
-			w.Header().Set("Content-Type", "application/json")
-			resp := map[string]string{"content": encoded + "\n", "encoding": "base64"}
-			if err := json.NewEncoder(w).Encode(resp); err != nil {
-				http.Error(w, err.Error(), http.StatusInternalServerError)
-			}
-		default:
-			http.Error(w, "unexpected: "+r.URL.Path, http.StatusNotFound)
-		}
-	}))
-	defer srv.Close()
-
-	c := newTestClient(srv)
-	files, err := c.GetAllFilesInPath(context.Background(), "owner", "repo", "patterns")
-	if err != nil {
-		t.Fatalf("GetAllFilesInPath: %v", err)
-	}
-	if len(files) != 1 {
-		t.Fatalf("len(files) = %d, want 1", len(files))
-	}
-	if files["patterns/patterns.md"] != content {
-		t.Errorf("content = %q, want %q", files["patterns/patterns.md"], content)
-	}
-}