feat(cmd): add VCS routing for GitHub PR reviews

Wire up the new GitHub API methods to the review-bot CLI via VCS
type detection. review-bot can now review PRs on both Gitea and
GitHub (including GitHub Enterprise Server).

Changes:
- vcs.go: define vcsClient interface with all PR operations
  - giteaVCSAdapter: wraps gitea.Client, satisfies vcsClient + giteaExtClient
  - githubVCSAdapter: wraps github.Client, satisfies vcsClient
  - giteaExtClient: Gitea-specific extension (supersede, comment resolution)
- main.go: detect VCS type via VCS_TYPE env var (auto-detects github.com URLs)
  - Creates appropriate client (gitea or github) based on vcs_type
  - GitHub API URL derived from server URL (github.com → api.github.com,
    GHES → /api/v3)
  - All main flow uses vcsClient interface
  - Gitea-specific supersede operations gated via giteaExtClient type assertion
  - GitHub: logs info when skipping supersede (not supported)
- Removes old giteaClientAdapter (replaced by giteaVCSAdapter in vcs.go)
- giteaVCSAdapter satisfies review.GiteaClient for persona loading

GitHub limitations handled gracefully:
- Review supersede skipped (GitHub doesn't allow editing submitted reviews)
- DeleteReview returns error for non-pending reviews (documented in adapter)
- Inline comments use absolute line + side='RIGHT' instead of diff position

Closes #130.

Co-authored-by: Rodin <rodin@forgedthought.ai>

TODO.md

@@ -1,151 +1,79 @@
-## Dev Loop: review-bot — Continuous Health Monitor
+## Dev Loop: review-bot — 2026-05-14 20:10 UTC
 
-### Current Cycle: 2026-05-15 02:10 UTC ✅
+### Latest: ✅ STABLE STATE — REPO HEALTH COMPLETE
-
+- **Last action:** health check; verified tests pass, repo clean, no action needed
-**Repository Status:** OPTIMAL
+- **Repository:** Clean, all merges complete, no open issues/PRs
-- Main: `9f3f321` (clean, all tests pass)
+- **Main branch:** Up to date with origin/main
-- Working tree: clean
+- **Test suite:** All passing (cached)
-- Build: ✅ successful
-- Vet: ✅ clean
-- Test suite: ALL PASS
 
 ---
 
-## Latest Delivered: Issue #130 ✅
+## Repository Status
 
-### GitHub API + VCS Routing Complete
+### ✅ 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
 
-**Phase 1: GitHub API Methods** ✅
+### 🧹 Cleanup COMPLETE:
-- 12+ methods implemented in `github/client.go`
+- ✅ Removed old worktrees (issue-123, review-bot-issue-125)
-- GetPullRequest, GetPullRequestDiff, GetPullRequestFiles
+- ✅ Test suite passes (all packages)
-- GetCommitStatuses, GetFileContent, ListContents, GetAllFilesInPath
+- ✅ No TODO/FIXME in code except expected GitHub client notes
-- PostReview, ListReviews, DeleteReview, GetAuthenticatedUser, RequestReviewer
+- ✅ No open issues or pull requests
-
+- ✅ Dependencies up to date
-**Phase 2: VCS Abstraction** ✅
-- `vcsClient` interface (GitHub + Gitea)
-- `giteaExtClient` interface (Gitea-specific ops)
-- Adapters for both platforms
-- URL-based auto-detection (github.com → GitHub, else Gitea)
-- `--vcs-type` flag and `VCS_TYPE` env override
-
-**Quality Metrics** ✅
-- 474 lines of GitHub client tests
-- 82 lines of routing tests
-- 361 lines of VCS adapter code
-- Security review: APPROVED (MINOR: URL heuristic note)
-- All tests passing; go vet clean
-
-**Known Limitations** (Documented)
-- GitHub: Can only delete PENDING (draft) reviews, not submitted (handled gracefully)
-- GitHub pagination: per-page=100 with Link header checking
-- Check-runs: Uses statuses API; check-runs deferrable to future enhancement
 
 ---
 
-## Repository Status Post-Merge
+## Current Feature Completeness
 
-### Main Branch
+✅ **Core Capabilities:**
-- Commit: `9f3f321`
-- Status: ✅ All systems healthy
-
-### Recent Merged PRs
-| PR | Issue | Title | Status |
-|---|---|---|---|
-| #131 | #130 | GitHub API methods & VCS routing | ✅ MERGED |
-| #129 | #123 | IP-level SSRF defense | ✅ MERGED |
-| #128 | #125 | VCS_URL deprecation & renaming | ✅ MERGED |
-| #127 | #124 | Multi-arch binary support | ✅ MERGED |
-| #126 | #120 | GitHub Actions composite action | ✅ MERGED |
-
-### Closed Issues
-- #130, #123, #125, #124, #120
-
-### Open Issues
-- None blocking; backlog tracked in Gitea project board
-
-### Worktrees
-- All cleaned up; no stale branches
-
----
-
-## Feature Completeness Summary
-
-### ✅ Core Functionality
 - Multi-provider LLM support (OpenAI, Anthropic, SAP AI Core)
-- Gitea PR review (mature, proven)
+- Gitea PR integration with structured reviews
-- **NEW: GitHub PR review (fully implemented)**
-- VCS abstraction (Gitea/GitHub transparent routing)
 - SSRF defense with IP-level validation
-- Multi-architecture binary deployment
+- VCS abstraction (Gitea/GitHub support)
+- Multi-architecture binary support
+- GitHub Actions composite action
 
-### ✅ Review Quality
+✅ **Recent Security Work:**
-- Structured reviews with code snippets
+- RFC6598 CGN range detection
-- LLM-driven analysis
+- IP fallback dialing for local endpoint rejection
-- Persona-based customization
+- URL validation for SSRF prevention
-- Context awareness
 
-### ✅ Security
+✅ **Code Quality:**
-- RFC6598 CGN detection
+- Comprehensive test coverage (all packages tested)
-- HTTPS enforcement
+- Consistent error handling with context propagation
-- Redirect safety
+- Secure credential handling (unexported fields)
-- Credential handling (no logs, no reflection leaks)
+- Concurrency-safe designs
-- URL validation for VCS API access
 
 ---
 
-## Next Phase: Backlog Priorities
+## Next Priority Actions
 
-### Priority 1: PR Submission
+### Phase 2: Feature Exploration (NEXT SESSION)
-**Issue:** #132+ (create)
+- Scan code for potential improvements per REVIEW.md findings
-**Goal:** Enable review-bot to create PRs (not just post reviews)
+- Assess performance under load
-**Scope:** PR creation flow, commit logic, test coverage
+- Review REVIEW.md findings for targeted fixes
-**Est. Time:** 3–5 days
+- Consider backlog items from design docs
-**Impact:** Enable automated improvements, fix suggestions with diff context
 
-### Priority 2: GitHub Enterprise Support
+### Phase 3: Optional Enhancements (BACKLOG)
-**Goal:** Explicit testing & routing for GitHub Enterprise
+- Address REVIEW.md context propagation findings (if prioritized)
-**Gap:** Enterprise URL patterns, /api/v3 suffix handling, token scopes
+- Additional LLM provider support
-**Scope:** Tests, URL routing, documentation
+- Enhanced context detection
-**Est. Time:** 2–3 days
+- Custom report formats
-**Impact:** Enable enterprise customers, reduce integration risk
+- Webhook management improvements
-
-### Priority 3: Performance & Observability
-**Areas:**
-- Load testing under concurrent reviews
-- Metrics collection (review latency, LLM token usage, API call counts)
-- Audit logging for compliance workflows
-- Dashboard (review history, metrics, team analytics)
-**Est. Time:** 5–7 days
-**Impact:** Operational confidence, troubleshooting, compliance
-
-### Priority 4: Enhanced Context
-**Opportunities:**
-- Semantic code understanding (AST-based analysis for specific languages)
-- Project-specific review rules (.review-bot.yaml in repo root)
-- Team-level customization
-**Est. Time:** 7–10 days
 
 ---
 
-## Dev Loop Schedule
+## Worktrees Status
-
+All old worktrees cleaned up. Ready for new issue work.
-- **Interval:** 4 hours
-- **Next check:** ~6:10 AM UTC (May 15)
-- **Health:** ✅ Optimal — all systems running
-- **Status:** Ready for next phase work
 
 ---
 
-## Metadata
+## Dev-Loop Metadata
-
+- **Repo:** /home/ubuntu/review-bot
-| Key | Value |
+- **Main branch SHA:** ed3a5dd (last commit)
-|---|---|
+- **Cron ID:** 5342ac81-4bbc-4e4c-a123-347a7788d50c
-| Repo | `/home/ubuntu/review-bot` |
+- **Scheduled:** Every 4 hours
-| Main SHA | `9f3f321` |
+- **Last health check:** 2026-05-14 20:10 UTC (✅ all healthy)
-| Last update | 2026-05-15 02:10 UTC |
-| Status | All systems optimal |
-| Next phase | PR submission or GitHub Enterprise support |
-
----
-
-**Summary:** review-bot now supports both GitHub and Gitea PR reviews with a unified abstraction layer. All tests pass, code is clean, security is approved. Ready to move to PR submission or GitHub Enterprise support in the next cycle.

cmd/review-bot/integration_test.go

@@ -10,7 +10,6 @@ import (
 	"testing"
 
 	"gitea.weiker.me/rodin/review-bot/gitea"
-	"gitea.weiker.me/rodin/review-bot/github"
 	"gitea.weiker.me/rodin/review-bot/llm"
 	"gitea.weiker.me/rodin/review-bot/review"
 )
@@ -160,85 +159,3 @@ func TestIntegration_PostAndCleanup(t *testing.T) {
 		t.Logf("Warning: could not delete test review %d: %v", posted.ID, err)
 	}
 }
-
-// TestIntegration_GitHub_PostAndVerifyReview exercises the full VCS routing path
-// for GitHub when INTEGRATION_GITHUB_TOKEN and INTEGRATION_GITHUB_REPO are set.
-// It verifies that the GitHub adapter is selected via VCS_TYPE=github and that
-// PostReview succeeds against a real GitHub PR.
-//
-// Required environment variables:
-//
-//	INTEGRATION_GITHUB_TOKEN  - GitHub personal access token with repo access
-//	INTEGRATION_GITHUB_REPO   - owner/repo with an open PR (e.g. Rodin-AI/review-bot)
-//	INTEGRATION_GITHUB_PR     - PR number to test against
-//
-// The test skips gracefully when these variables are absent.
-func TestIntegration_GitHub_PostAndVerifyReview(t *testing.T) {
-	githubToken := os.Getenv("INTEGRATION_GITHUB_TOKEN")
-	githubRepo := os.Getenv("INTEGRATION_GITHUB_REPO")
-	prNumStr := os.Getenv("INTEGRATION_GITHUB_PR")
-
-	if githubToken == "" || githubRepo == "" || prNumStr == "" {
-		t.Skip("INTEGRATION_GITHUB_TOKEN, INTEGRATION_GITHUB_REPO, and INTEGRATION_GITHUB_PR not set, skipping")
-	}
-
-	prNumber, err := strconv.Atoi(prNumStr)
-	if err != nil {
-		t.Fatalf("Invalid PR number %q: %v", prNumStr, err)
-	}
-
-	parts := strings.SplitN(githubRepo, "/", 2)
-	if len(parts) != 2 || parts[0] == "" || parts[1] == "" {
-		t.Fatalf("Invalid repo format %q, expected owner/repo", githubRepo)
-	}
-	owner, repoName := parts[0], parts[1]
-
-	ctx := context.Background()
-	ghClient := github.NewClient(githubToken, "https://api.github.com")
-
-	// Verify adapter selection: GetAuthenticatedUser must succeed.
-	user, err := ghClient.GetAuthenticatedUser(ctx)
-	if err != nil {
-		t.Fatalf("GetAuthenticatedUser: %v — check INTEGRATION_GITHUB_TOKEN", err)
-	}
-	t.Logf("Authenticated as: %s", user)
-
-	// Verify PR is accessible via GitHub adapter.
-	pr, err := ghClient.GetPullRequest(ctx, owner, repoName, prNumber)
-	if err != nil {
-		t.Fatalf("GetPullRequest: %v", err)
-	}
-	t.Logf("PR: %s (sha: %s)", pr.Title, pr.Head.Sha)
-
-	// Post a COMMENT review — does not require PR approval permissions.
-	sentinel := "<!-- review-bot:integration-test -->"
-	testBody := "# Integration Test Review (GitHub)\n\nThis is an automated integration test.\n\n" + sentinel
-	posted, err := ghClient.PostReview(ctx, owner, repoName, prNumber, "COMMENT", testBody, "", nil)
-	if err != nil {
-		t.Fatalf("PostReview: %v", err)
-	}
-	t.Logf("Posted review ID: %d", posted.ID)
-
-	// Verify the review appears in ListReviews.
-	reviews, err := ghClient.ListReviews(ctx, owner, repoName, prNumber)
-	if err != nil {
-		t.Fatalf("ListReviews: %v", err)
-	}
-	found := false
-	for _, r := range reviews {
-		if r.ID == posted.ID && strings.Contains(r.Body, sentinel) {
-			found = true
-			break
-		}
-	}
-	if !found {
-		t.Errorf("posted review ID %d not found in ListReviews output", posted.ID)
-	}
-
-	// Attempt cleanup — GitHub does not allow deleting submitted reviews,
-	// so this is expected to fail with ErrCannotDeleteSubmittedReview (422).
-	// Log it as informational only.
-	if err := ghClient.DeleteReview(ctx, owner, repoName, prNumber, posted.ID); err != nil {
-		t.Logf("Note: DeleteReview returned (expected for submitted GitHub reviews): %v", err)
-	}
-}

cmd/review-bot/main_test.go

@@ -630,48 +630,6 @@ func TestEvaluateCIStatus(t *testing.T) {
 	}
 }
 
-func TestGithubAPIURL(t *testing.T) {
-	tests := []struct {
-		name  string
-		input string
-		want  string
-	}{
-		{
-			name:  "empty string defaults to api.github.com",
-			input: "",
-			want:  "https://api.github.com",
-		},
-		{
-			name:  "github.com maps to api.github.com",
-			input: "https://github.com",
-			want:  "https://api.github.com",
-		},
-		{
-			name:  "github.com with trailing slash maps to api.github.com",
-			input: "https://github.com/",
-			want:  "https://api.github.com",
-		},
-		{
-			name:  "GHES host gets /api/v3 suffix",
-			input: "https://ghe.example.com",
-			want:  "https://ghe.example.com/api/v3",
-		},
-		{
-			name:  "GHES concur domain does not map to api.github.com",
-			input: "https://github.concur.com",
-			want:  "https://github.concur.com/api/v3",
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got := githubAPIURL(tt.input)
-			if got != tt.want {
-				t.Errorf("githubAPIURL(%q) = %q, want %q", tt.input, got, tt.want)
-			}
-		})
-	}
-}
-
 func TestEnvOrDefault(t *testing.T) {
 	// Test with unset env var
 	os.Unsetenv("TEST_ENV_OR_DEFAULT_UNSET")
@@ -1012,7 +970,7 @@ func TestMainSubprocess_InvalidProvider(t *testing.T) {
 	}
 }
 
-// cleanEnv returns environ without any GITEA/LLM/REVIEWER/VCS env vars that would
+// cleanEnv returns environ without any GITEA/LLM/REVIEWER env vars that would
 // interfere with testing missing-flag scenarios.
 func cleanEnv() []string {
 	var env []string
@@ -1027,8 +985,7 @@ func cleanEnv() []string {
 			strings.HasPrefix(key, "CONVENTIONS_"),
 			strings.HasPrefix(key, "SYSTEM_PROMPT_"),
 			strings.HasPrefix(key, "PATTERNS_"),
-			strings.HasPrefix(key, "UPDATE_"),
+			strings.HasPrefix(key, "UPDATE_"):
-			strings.HasPrefix(key, "VCS_"):
 			continue
 		default:
 			env = append(env, e)

github/client.go

@@ -376,57 +376,6 @@ func (c *Client) doGet(ctx context.Context, url string) ([]byte, error) {
 	return c.doRequest(ctx, http.MethodGet, url, "")
 }
 
-// doRequestWithBody performs an HTTP request with an optional body, applying the
-// same HTTPS enforcement as doRequest. It is used by write methods (POST, PUT,
-// DELETE) that bypass the retry loop in doRequest because write operations are
-// not idempotent.
-//
-// body may be nil for requests that carry no payload (e.g. DELETE).
-// When body is non-nil, Content-Type is set to application/json.
-func (c *Client) doRequestWithBody(ctx context.Context, method, reqURL string, body []byte) ([]byte, error) {
-	if !c.allowInsecureHTTP {
-		parsed, err := url.Parse(reqURL)
-		if err != nil {
-			return nil, fmt.Errorf("parse request URL: %w", err)
-		}
-		if strings.EqualFold(parsed.Scheme, "http") {
-			return nil, fmt.Errorf("refusing HTTP request to %s: use HTTPS or set AllowInsecureHTTP option", redactURL(reqURL))
-		}
-	}
-
-	var reqBody io.Reader
-	if body != nil {
-		reqBody = bytes.NewReader(body)
-	}
-
-	req, err := http.NewRequestWithContext(ctx, method, reqURL, reqBody)
-	if err != nil {
-		return nil, fmt.Errorf("create request: %w", err)
-	}
-	req.Header.Set("Authorization", "Bearer "+c.token)
-	req.Header.Set("Accept", "application/vnd.github+json")
-	if body != nil {
-		req.Header.Set("Content-Type", "application/json")
-	}
-
-	resp, err := c.httpClient.Do(req)
-	if err != nil {
-		return nil, fmt.Errorf("do request: %w", err)
-	}
-	defer resp.Body.Close()
-
-	if resp.StatusCode >= 200 && resp.StatusCode < 300 {
-		respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBodyBytes))
-		if err != nil {
-			return nil, fmt.Errorf("read response body: %w", err)
-		}
-		return respBody, nil
-	}
-
-	errBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes))
-	return nil, &APIError{StatusCode: resp.StatusCode, Body: string(errBody)}
-}
-
 // --- API types ---
 
 // PullRequest holds relevant PR metadata.
@@ -728,11 +677,29 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
 		return nil, fmt.Errorf("marshal review payload: %w", err)
 	}
 
-	respBody, err := c.doRequestWithBody(ctx, http.MethodPost, reqURL, data)
+	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, maxErrorBodyBytes))
+		return nil, &APIError{StatusCode: resp.StatusCode, Body: string(respBody)}
+	}
+
+	respBody, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBodyBytes))
+	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)
@@ -773,11 +740,23 @@ func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number in
 	reqURL := fmt.Sprintf("%s/repos/%s/%s/pulls/%d/reviews/%d",
 		c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number, reviewID)
 
-	// nil body: the GitHub DELETE endpoint for reviews requires no request body.
+	req, err := http.NewRequestWithContext(ctx, http.MethodDelete, reqURL, nil)
-	_, err := c.doRequestWithBody(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, maxErrorBodyBytes))
+		return &APIError{StatusCode: resp.StatusCode, Body: string(respBody)}
+	}
 	return nil
 }
 
@@ -811,10 +790,24 @@ func (c *Client) RequestReviewer(ctx context.Context, owner, repo string, number
 		return fmt.Errorf("marshal reviewer request: %w", err)
 	}
 
-	_, err = c.doRequestWithBody(ctx, http.MethodPost, reqURL, data)
+	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 != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent {
+		respBody, _ := io.ReadAll(io.LimitReader(resp.Body, 256))
+		return &APIError{StatusCode: resp.StatusCode, Body: string(respBody)}
+	}
 	return nil
 }
 

github/client_test.go

@@ -1077,42 +1077,6 @@ func TestRequestReviewer_Success(t *testing.T) {
 	}
 }
 
-func TestPostReview_RejectsHTTP(t *testing.T) {
-	// PostReview must reject http:// base URLs — tokens must not be sent in plaintext.
-	c := NewClient("tok", "http://127.0.0.1:1")
-	_, err := c.PostReview(context.Background(), "owner", "repo", 1, "APPROVE", "body", "", nil)
-	if err == nil {
-		t.Fatal("expected error for HTTP base URL in PostReview")
-	}
-	if !strings.Contains(err.Error(), "refusing HTTP request") {
-		t.Errorf("unexpected error message: %v", err)
-	}
-}
-
-func TestDeleteReview_RejectsHTTP(t *testing.T) {
-	// DeleteReview must reject http:// base URLs — tokens must not be sent in plaintext.
-	c := NewClient("tok", "http://127.0.0.1:1")
-	err := c.DeleteReview(context.Background(), "owner", "repo", 1, 42)
-	if err == nil {
-		t.Fatal("expected error for HTTP base URL in DeleteReview")
-	}
-	if !strings.Contains(err.Error(), "refusing HTTP request") {
-		t.Errorf("unexpected error message: %v", err)
-	}
-}
-
-func TestRequestReviewer_RejectsHTTP(t *testing.T) {
-	// RequestReviewer must reject http:// base URLs — tokens must not be sent in plaintext.
-	c := NewClient("tok", "http://127.0.0.1:1")
-	err := c.RequestReviewer(context.Background(), "owner", "repo", 1, "reviewer1")
-	if err == nil {
-		t.Fatal("expected error for HTTP base URL in RequestReviewer")
-	}
-	if !strings.Contains(err.Error(), "refusing HTTP request") {
-		t.Errorf("unexpected error message: %v", err)
-	}
-}
-
 func TestEscapePath_SpecialChars(t *testing.T) {
 	tests := []struct {
 		input string