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

@@ -1,17 +1,37 @@
-# This composite action is designed for Gitea Actions runners.
-# Gitea Actions supports GitHub Actions syntax including $GITHUB_OUTPUT,
-# actions/cache, and actions/checkout.
+# This composite action supports both Gitea Actions and GitHub Actions runners.
+# It detects the VCS host type by checking whether github.api_url is set
+# (present on GitHub.com and GHES runners, absent on Gitea runners) and uses
+# the appropriate releases API for version resolution and binary download
+# (REST API on GitHub, direct URLs on Gitea).
+#
+# Security notes:
+# - On GitHub/GHES (VCS_TYPE=github), inputs.vcs-url is IGNORED to prevent
+#   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) before use.
+# - action-repo is validated against owner/repo pattern.
+# - Tokens are passed via masked environment variables, not step outputs.
+#
 # Requirements: python3, sha256sum, curl (all present on ubuntu-* runners).
 name: 'AI Code Review'
 description: 'Run AI-powered code review on a pull request using review-bot'
 
 inputs:
-  gitea-url:
-    description: 'Gitea instance URL (defaults to server_url)'
+  vcs-url:
+    description: 'VCS server URL (only used on Gitea runners; ignored on GitHub/GHES). Defaults to server_url.'
     required: false
     default: ''
   repo:
-    description: 'Repository (owner/name, defaults to current)'
+    description: 'Repository to review (owner/name, defaults to current)'
+    required: false
+    default: ''
+  action-repo:
+    description: 'Repository hosting review-bot releases (owner/name). Defaults to github.action_repository or rodin/review-bot.'
+    required: false
+    default: ''
+  action-repo-token:
+    description: 'Token for downloading release assets from action-repo (defaults to github.token on GitHub, reviewer-token on Gitea). Required for private repos.'
     required: false
     default: ''
   pr-number:
@@ -19,7 +39,7 @@ inputs:
     required: false
     default: ''
   reviewer-token:
-    description: 'Gitea token for posting the review'
+    description: 'Token for posting the review'
     required: true
   reviewer-name:
     description: 'Display name for the reviewer'
@@ -112,45 +132,265 @@ runs:
       id: version
       shell: bash
       run: |
-        GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
-        REPO="${{ inputs.repo || 'rodin/review-bot' }}"
+        set -euo pipefail
+
+        # --- Input Validation ---
+
+        # Determine the repo hosting review-bot releases (not the repo being reviewed)
+        ACTION_REPO="${{ inputs.action-repo }}"
+        if [ -z "$ACTION_REPO" ]; then
+          # github.action_repository is the repo containing the running action
+          ACTION_REPO="${{ github.action_repository }}"
+        fi
+        if [ -z "$ACTION_REPO" ]; then
+          # Final fallback for Gitea (which may not set action_repository)
+          ACTION_REPO="rodin/review-bot"
+          echo "::notice::action-repo not specified and github.action_repository is empty; falling back to rodin/review-bot"
+        fi
+
+        # Validate ACTION_REPO matches owner/repo pattern (prevent path traversal)
+        if ! printf '%s' "$ACTION_REPO" | grep -qE '^[a-zA-Z0-9._-]+/[a-zA-Z0-9._-]+$'; then
+          echo "Error: action-repo '${ACTION_REPO}' does not match expected owner/repo format" >&2
+          exit 1
+        fi
+
+        # Detect VCS host type using github.api_url context.
+        # github.api_url is set on GitHub.com (https://api.github.com) and GHES
+        # (https://<host>/api/v3). It is empty/unset on Gitea Actions runners.
+        GITHUB_API_URL="${{ github.api_url }}"
+        if [ -n "$GITHUB_API_URL" ]; then
+          VCS_TYPE="github"
+        else
+          VCS_TYPE="gitea"
+        fi
+
+        # Determine SERVER_URL based on VCS type.
+        # SECURITY: On GitHub/GHES, ALWAYS use github.server_url — never trust
+        # inputs.vcs-url to prevent token exfiltration to attacker-controlled hosts.
+        if [ "$VCS_TYPE" = "github" ]; then
+          SERVER_URL="${{ github.server_url }}"
+          if [ -n "${{ inputs.vcs-url }}" ]; then
+            echo "::warning::inputs.vcs-url is ignored on GitHub/GHES runners (VCS_TYPE=github). Using github.server_url instead."
+          fi
+        else
+          SERVER_URL="${{ inputs.vcs-url || github.server_url }}"
+        fi
+        # Strip trailing slash if present
+        SERVER_URL="${SERVER_URL%/}"
+
+        # Validate SERVER_URL for Gitea path: must be https, no whitespace/newlines.
+        # The [^[:space:]] class already rejects newlines, so no separate newline check needed.
+        if [ "$VCS_TYPE" = "gitea" ]; then
+          if ! printf '%s' "$SERVER_URL" | grep -qE '^https://[^[:space:]]+$'; then
+            echo "Error: SERVER_URL '${SERVER_URL}' must be an https:// URL with no whitespace" >&2
+            exit 1
+          fi
+        fi
+
+        # Determine auth token for release API requests
+        ACTION_TOKEN="${{ inputs.action-repo-token }}"
+        if [ -z "$ACTION_TOKEN" ]; then
+          if [ "$VCS_TYPE" = "github" ]; then
+            ACTION_TOKEN="${{ github.token }}"
+          else
+            ACTION_TOKEN="${{ inputs.reviewer-token }}"
+          fi
+        fi
+
+        # Validate token contains no control characters (defense-in-depth against header injection)
+        if [ -n "$ACTION_TOKEN" ]; then
+          if printf '%s' "$ACTION_TOKEN" | LC_ALL=C grep -q '[^[:print:]]'; then
+            echo "Error: ACTION_TOKEN contains control characters" >&2
+            exit 1
+          fi
+        fi
+
         if [ "${{ inputs.version }}" = "latest" ]; then
-          VERSION=$(curl -sSf "${GITEA_URL}/api/v1/repos/${REPO}/releases?limit=1" \
+          if [ "$VCS_TYPE" = "github" ]; then
+            # SECURITY: Use github.api_url which is a trusted platform-provided value.
+            # Never construct API URLs from user-supplied inputs on GitHub.
+            API_URL="${GITHUB_API_URL}/repos/${ACTION_REPO}/releases?per_page=1"
+          else
+            # Gitea API — SERVER_URL was validated above
+            API_URL="${SERVER_URL}/api/v1/repos/${ACTION_REPO}/releases?limit=1"
+          fi
+
+          # Fetch latest version with inline auth header (no intermediate variable)
+          if [ -n "$ACTION_TOKEN" ]; then
+            if [ "$VCS_TYPE" = "github" ]; then
+              VERSION=$(curl -sSf --connect-timeout 10 --max-time 30 \
+                -H "Authorization: Bearer ${ACTION_TOKEN}" "$API_URL" \
                 | python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')")
+            else
+              VERSION=$(curl -sSf --connect-timeout 10 --max-time 30 \
+                -H "Authorization: token ${ACTION_TOKEN}" "$API_URL" \
+                | python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')")
+            fi
+          else
+            VERSION=$(curl -sSf --connect-timeout 10 --max-time 30 "$API_URL" \
+              | python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')")
+          fi
+
           if [ -z "$VERSION" ]; then
-            echo "Failed to determine latest version" >&2
+            echo "Failed to determine latest version from ${API_URL}" >&2
             exit 1
           fi
         else
           VERSION="${{ inputs.version }}"
         fi
+
+        # Validate VERSION: no slashes or whitespace (prevent path traversal).
+        # [:space:] includes newlines and carriage returns in POSIX.
+        if printf '%s' "$VERSION" | grep -qE '[/[:space:]]'; then
+          echo "Error: VERSION '${VERSION}' contains invalid characters (newline, slash, or whitespace)" >&2
+          exit 1
+        fi
+
+        # Detect OS and architecture for platform-specific binary download
+        OS_RAW=$(uname -s | tr '[:upper:]' '[:lower:]')
+        case "$OS_RAW" in
+          linux)  OS="linux" ;;
+          darwin) OS="darwin" ;;
+          *)
+            echo "Error: unsupported OS: $(uname -s)" >&2
+            exit 1
+            ;;
+        esac
+
+        RAW_ARCH=$(uname -m)
+        case "$RAW_ARCH" in
+          x86_64)           ARCH="amd64" ;;
+          aarch64 | arm64)  ARCH="arm64" ;;
+          *)
+            echo "Error: unsupported architecture: $RAW_ARCH" >&2
+            exit 1
+            ;;
+        esac
+
         echo "version=${VERSION}" >> "$GITHUB_OUTPUT"
+        echo "os=${OS}" >> "$GITHUB_OUTPUT"
+        echo "arch=${ARCH}" >> "$GITHUB_OUTPUT"
+        echo "action_repo=${ACTION_REPO}" >> "$GITHUB_OUTPUT"
+        echo "server_url=${SERVER_URL}" >> "$GITHUB_OUTPUT"
+        echo "vcs_type=${VCS_TYPE}" >> "$GITHUB_OUTPUT"
+
+        # SECURITY: Pass token via masked environment variable instead of step output.
+        # Step outputs can leak in debug logs; GITHUB_ENV with masking is safer.
+        if [ -n "$ACTION_TOKEN" ]; then
+          echo "::add-mask::${ACTION_TOKEN}"
+          echo "ACTION_TOKEN=${ACTION_TOKEN}" >> "$GITHUB_ENV"
+        fi
 
     - name: Cache review-bot binary
       id: cache
       uses: actions/cache@v4
       with:
         path: ${{ runner.temp }}/review-bot
-        key: review-bot-linux-amd64-${{ steps.version.outputs.version }}
+        key: review-bot-${{ steps.version.outputs.os }}-${{ steps.version.outputs.arch }}-${{ steps.version.outputs.version }}
 
     - name: Install review-bot
       if: steps.cache.outputs.cache-hit != 'true'
       shell: bash
       run: |
-        GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
-        REPO="${{ inputs.repo || 'rodin/review-bot' }}"
-        VERSION="${{ steps.version.outputs.version }}"
-        BINARY="review-bot-linux-amd64"
+        set -euo pipefail
 
-        curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/${BINARY}" \
+        SERVER_URL="${{ steps.version.outputs.server_url }}"
+        ACTION_REPO="${{ steps.version.outputs.action_repo }}"
+        VERSION="${{ steps.version.outputs.version }}"
+        VCS_TYPE="${{ steps.version.outputs.vcs_type }}"
+        OS="${{ steps.version.outputs.os }}"
+        ARCH="${{ steps.version.outputs.arch }}"
+        # Read token from masked environment variable (set in Determine version step)
+        # Falls back to empty if not set (public repos don't need auth)
+        ACTION_TOKEN="${ACTION_TOKEN:-}"
+        BINARY="review-bot-${OS}-${ARCH}"
+
+        if [ "$VCS_TYPE" = "github" ]; then
+          # GitHub/GHES: Use REST API for release asset downloads.
+          # Web release URLs ({server}/.../releases/download/{tag}/{asset}) redirect
+          # to S3 and don't reliably support Authorization headers for private repos.
+          # The REST API endpoint with Accept: application/octet-stream is required.
+          # GITHUB_API_URL: trusted platform value, same as detected in "Determine version" step.
+          GITHUB_API_URL="${{ github.api_url }}"
+
+          if [ -n "$ACTION_TOKEN" ]; then
+            RELEASE_JSON=$(curl -sSf --connect-timeout 10 --max-time 30 \
+              -H "Authorization: Bearer ${ACTION_TOKEN}" \
+              "${GITHUB_API_URL}/repos/${ACTION_REPO}/releases/tags/${VERSION}")
+          else
+            RELEASE_JSON=$(curl -sSf --connect-timeout 10 --max-time 30 \
+              "${GITHUB_API_URL}/repos/${ACTION_REPO}/releases/tags/${VERSION}")
+          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 '')")
+          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 '')")
+          if [ -z "$CHECKSUMS_ASSET_ID" ]; then
+            echo "Error: could not find asset 'checksums.txt' in release ${VERSION}" >&2
+            exit 1
+          fi
+
+          # Download assets via REST API with Accept: application/octet-stream
+          if [ -n "$ACTION_TOKEN" ]; then
+            curl -sSfL --connect-timeout 10 --max-time 120 \
+              -H "Authorization: Bearer ${ACTION_TOKEN}" \
+              -H "Accept: application/octet-stream" \
+              "${GITHUB_API_URL}/repos/${ACTION_REPO}/releases/assets/${BINARY_ASSET_ID}" \
               -o "${{ runner.temp }}/review-bot"
-        curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/checksums.txt" \
+            curl -sSfL --connect-timeout 10 --max-time 30 \
+              -H "Authorization: Bearer ${ACTION_TOKEN}" \
+              -H "Accept: application/octet-stream" \
+              "${GITHUB_API_URL}/repos/${ACTION_REPO}/releases/assets/${CHECKSUMS_ASSET_ID}" \
               -o "${{ runner.temp }}/checksums.txt"
+          else
+            curl -sSfL --connect-timeout 10 --max-time 120 \
+              -H "Accept: application/octet-stream" \
+              "${GITHUB_API_URL}/repos/${ACTION_REPO}/releases/assets/${BINARY_ASSET_ID}" \
+              -o "${{ runner.temp }}/review-bot"
+            curl -sSfL --connect-timeout 10 --max-time 30 \
+              -H "Accept: application/octet-stream" \
+              "${GITHUB_API_URL}/repos/${ACTION_REPO}/releases/assets/${CHECKSUMS_ASSET_ID}" \
+              -o "${{ runner.temp }}/checksums.txt"
+          fi
+        else
+          # Gitea: Direct download via web release URLs (Gitea serves assets
+          # directly without redirects — no -L needed).
+          # SECURITY: Omitting -L prevents forwarding Authorization header to
+          # unexpected hosts if Gitea ever introduces CDN redirects.
+          DOWNLOAD_URL="${SERVER_URL}/${ACTION_REPO}/releases/download/${VERSION}"
+
+          if [ -n "$ACTION_TOKEN" ]; then
+            curl -sSf --connect-timeout 10 --max-time 120 \
+              -H "Authorization: token ${ACTION_TOKEN}" \
+              "${DOWNLOAD_URL}/${BINARY}" -o "${{ runner.temp }}/review-bot"
+            curl -sSf --connect-timeout 10 --max-time 30 \
+              -H "Authorization: token ${ACTION_TOKEN}" \
+              "${DOWNLOAD_URL}/checksums.txt" -o "${{ runner.temp }}/checksums.txt"
+          else
+            curl -sSf --connect-timeout 10 --max-time 120 \
+              "${DOWNLOAD_URL}/${BINARY}" -o "${{ runner.temp }}/review-bot"
+            curl -sSf --connect-timeout 10 --max-time 30 \
+              "${DOWNLOAD_URL}/checksums.txt" -o "${{ runner.temp }}/checksums.txt"
+          fi
+        fi
 
         # Verify SHA-256 checksum
+        # NOTE: This verifies integrity (download wasn't corrupted) but not
+        # authenticity — both binary and checksums come from the same server.
+        # For stronger guarantees, consider GPG signature verification.
         cd "${{ runner.temp }}"
-        EXPECTED=$(grep "${BINARY}" checksums.txt | awk '{print $1}')
+        EXPECTED=$(grep -E "^[0-9a-f]+[[:space:]]+\*?${BINARY}$" checksums.txt | awk '{print $1}')
+        # sha256sum (GNU) is not available on macOS; use shasum -a 256 on darwin.
+        if [ "${OS}" = "darwin" ]; then
+          ACTUAL=$(shasum -a 256 review-bot | awk '{print $1}')
+        else
           ACTUAL=$(sha256sum review-bot | awk '{print $1}')
+        fi
 
         if [ -z "$EXPECTED" ]; then
           echo "Error: no checksum found for ${BINARY}" >&2
@@ -164,12 +404,12 @@ runs:
         fi
 
         chmod +x "${{ runner.temp }}/review-bot"
-        echo "Installed review-bot ${VERSION} (checksum verified)"
+        echo "Installed review-bot-${OS}-${ARCH} ${VERSION} (checksum verified)"
 
     - name: Run review
       shell: bash
       env:
-        GITEA_URL: ${{ inputs.gitea-url || github.server_url }}
+        VCS_URL: ${{ steps.version.outputs.server_url }}
         GITEA_REPO: ${{ inputs.repo || github.repository }}
         PR_NUMBER: ${{ inputs.pr-number || github.event.pull_request.number }}
         REVIEWER_TOKEN: ${{ inputs.reviewer-token }}

.gitea/workflows/ci.yml

@@ -49,7 +49,7 @@ jobs:
       - run: go build -o review-bot ./cmd/review-bot
       - name: Run ${{ matrix.name }} review
         env:
-          GITEA_URL: ${{ github.server_url }}
+          VCS_URL: ${{ github.server_url }}
           GITEA_REPO: ${{ github.repository }}
           PR_NUMBER: ${{ github.event.pull_request.number }}
           REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }}

CONVENTIONS.md

@@ -9,7 +9,7 @@
 
 | Package | Use Case | Scope |
 |---------|----------|-------|
-| `github.com/goccy/go-yaml` | YAML parsing (persona files, config) | production |
+| `github.com/goccy/go-yaml` | YAML parsing and AST inspection (subpkgs: `ast`, `parser`) | production |
 | `github.com/google/go-cmp` | Test comparisons (`cmp.Diff`) | test only |
 
 **Any import not in this table or the Go standard library is forbidden.**
@@ -21,8 +21,6 @@ To request a new dependency:
 2. Requires explicit approval from Aaron
 3. After merge, a separate PR may use the package
 
-<!-- Deviation from step 1+3 for go-yaml migration: see #91 for rationale. -->
-
 *Enforcement: `scripts/check-deps.sh` parses this table — update only here.*
 
 ## Error Handling

PLAN-125.md

@@ -0,0 +1,175 @@
+# Plan: Issue #125 — Rename GITEA_URL → VCS_URL
+
+## Problem
+
+The `GITEA_URL` environment variable (and `--gitea-url` flag) implies the binary only works with Gitea.
+Now that review-bot supports both Gitea and GitHub/GHES, this name is misleading.
+Renaming to `VCS_URL` makes the binary platform-agnostic in its interface.
+
+## Constraints
+
+- Must not break existing users who already use `GITEA_URL` — need a fallback
+- The CLI flag `--gitea-url` should also be updated to `--vcs-url` for consistency
+- `INTEGRATION_GITEA_URL` in integration tests is a test-only env var, not the binary's interface; but should be updated for clarity
+- The action YAML uses `GITEA_URL` as an internal shell variable in bash scripts — distinct from the env var passed to the binary
+- All changes must compile and pass existing tests
+
+## Files Affected
+
+### Binary / Go source
+| File | Change |
+|------|--------|
+| `cmd/review-bot/main.go` | Rename `--gitea-url` → `--vcs-url`, add `VCS_URL` as primary, keep `GITEA_URL` fallback |
+| `cmd/review-bot/integration_test.go` | Rename `INTEGRATION_GITEA_URL` → `INTEGRATION_VCS_URL` (test-only, no external compat concern) |
+| `integration_test.go` | Same — rename `INTEGRATION_GITEA_URL` → `INTEGRATION_VCS_URL` |
+
+### Action YAML
+| File | Change |
+|------|--------|
+| `.gitea/actions/review/action.yml` | Rename input `gitea-url` → `vcs-url`; update env var passed to binary: `VCS_URL` instead of `GITEA_URL`; keep internal bash var as `GITEA_URL` (only used for release download, not passed to binary) |
+| `.gitea/workflows/ci.yml` | Rename `GITEA_URL` env var to `VCS_URL` in Run review step |
+
+### Documentation
+| File | Change |
+|------|--------|
+| `README.md` | Update CLI example, env var table entry |
+
+## Proposed Approach
+
+### 1. Backward-compatible env var lookup in main.go
+
+Replace:
+```go
+giteaURL := flag.String("gitea-url", envOrDefault("GITEA_URL", ""), "Gitea instance URL")
+```
+
+With:
+```go
+giteaURL := flag.String("vcs-url", envOrDefaultFallback("VCS_URL", "GITEA_URL", ""), "VCS server URL (e.g. https://gitea.example.com)")
+```
+
+Add a helper:
+```go
+// envOrDefaultFallback reads primary env var; if empty, falls back to deprecated env var.
+func envOrDefaultFallback(primary, deprecated, defaultVal string) string {
+    if v := os.Getenv(primary); v != "" {
+        return v
+    }
+    if v := os.Getenv(deprecated); v != "" {
+        slog.Warn("deprecated env var in use; rename to " + primary, "old", deprecated, "new", primary)
+        return v
+    }
+    return defaultVal
+}
+```
+
+**Note:** This must be called AFTER `setupLogger` conceptually, but the flag default is evaluated at flag registration time. Since `setupLogger` runs before `flag.Parse()`, the slog.Warn will print correctly at runtime. We use `log.Printf` as a fallback if this proves problematic.
+
+Actually — flag defaults are evaluated at registration (line 57), before `setupLogger`. The warning won't go through slog. Two options:
+- Use `log.Printf` for the deprecation warning (always visible)
+- Move the fallback lookup to after `flag.Parse()`, checking if the parsed value is still empty
+
+**Decision:** Move fallback to a post-parse check. This is cleaner:
+```go
+vcsURL := flag.String("vcs-url", os.Getenv("VCS_URL"), "VCS server URL")
+flag.Parse()
+// Backward compat: fall back to deprecated GITEA_URL
+if *vcsURL == "" {
+    if v := os.Getenv("GITEA_URL"); v != "" {
+        slog.Warn("GITEA_URL is deprecated; use VCS_URL instead")
+        *vcsURL = v
+    }
+}
+```
+
+This is clean, idiomatic, and the warning goes through slog correctly.
+
+### 2. Keep `--gitea-url` as deprecated alias
+
+Add a hidden flag for backward compat:
+```go
+giteaURLAlias := flag.String("gitea-url", "", "Deprecated: use --vcs-url")
+```
+
+Post-parse:
+```go
+if *vcsURL == "" && *giteaURLAlias != "" {
+    slog.Warn("--gitea-url is deprecated; use --vcs-url instead")
+    *vcsURL = *giteaURLAlias
+}
+```
+
+### 3. Internal variable rename
+
+Rename `giteaURL` local variable → `vcsURL` throughout `main.go` for consistency.
+
+### 4. Error message update
+
+```go
+fmt.Fprintf(os.Stderr, "Required: --vcs-url, --repo, --pr, --reviewer-token, --llm-model\n")
+```
+
+### 5. Action YAML changes
+
+In `.gitea/actions/review/action.yml`:
+- Input `gitea-url` → `vcs-url` (with same description, `required: false`, `default: ''`)
+- Line 172: `GITEA_URL: ${{ inputs.gitea-url || github.server_url }}` → `VCS_URL: ${{ inputs.vcs-url || github.server_url }}`
+- Lines 115, 140: internal bash vars `GITEA_URL=` are used for downloading binaries — NOT passed to the review-bot binary. Leave them as internal bash vars (they're scope-local in bash). These could be renamed to `SERVER_URL` or `BASE_URL` for local clarity, but renaming them isn't strictly required.
+
+In `.gitea/workflows/ci.yml`:
+- Line 52: `GITEA_URL: ${{ github.server_url }}` → `VCS_URL: ${{ github.server_url }}`
+
+### 6. Integration test updates
+
+`INTEGRATION_GITEA_URL` → `INTEGRATION_VCS_URL` in both test files.
+
+### 7. README
+
+- CLI example: `--gitea-url` → `--vcs-url`
+- Env var table: `GITEA_URL` → `VCS_URL`, add note about `GITEA_URL` fallback
+
+## Backward Compatibility Summary
+
+| Old | New | Fallback? |
+|-----|-----|-----------|
+| `GITEA_URL` env var | `VCS_URL` | ✅ with deprecation warning |
+| `--gitea-url` flag | `--vcs-url` | ✅ with deprecation warning |
+| `gitea-url` action input | `vcs-url` | ⚠️ No (action version bump handles this) |
+| `INTEGRATION_GITEA_URL` | `INTEGRATION_VCS_URL` | N/A (test-only) |
+
+## Error Cases
+
+- Both `VCS_URL` and `GITEA_URL` set: `VCS_URL` wins (primary takes precedence)
+- Both `--vcs-url` and `--gitea-url` provided: `--vcs-url` wins
+- Neither set: existing "missing required flags" error unchanged
+
+## Edge Cases
+
+- `os.Getenv` returns "" for unset AND set-to-empty — consistent with existing behavior
+- The `envOrDefault` helper is unchanged; we add `envOrDefaultFallback` for the one renamed var
+
+## Testing Strategy
+
+- Existing unit tests pass unchanged (they don't test env var parsing directly)
+- Integration tests updated to use new env var name
+- Manual: `GITEA_URL=https://example.com ./review-bot --repo x --pr 1 ...` should print deprecation warning and proceed
+- Manual: `VCS_URL=https://example.com ./review-bot ...` should work silently
+
+## Completion Checklist
+
+1. `VCS_URL` is read first; `GITEA_URL` is fallback with deprecation warning
+2. `--vcs-url` flag is primary; `--gitea-url` is deprecated alias with warning
+3. Error message references `--vcs-url` not `--gitea-url`
+4. `action.yml` passes `VCS_URL` (not `GITEA_URL`) to the binary
+5. `ci.yml` passes `VCS_URL` (not `GITEA_URL`) to the binary
+6. README updated in CLI example and env var table
+7. Integration tests use `INTEGRATION_VCS_URL`
+8. `go test ./...` passes
+9. `go vet ./...` passes
+10. `go build ./cmd/review-bot` succeeds
+
+## Open Questions
+
+- Should the CLI flag `--gitea-url` be completely hidden from `--help` or just deprecated with a note? The issue doesn't specify. Decision: keep it visible but add "(deprecated: use --vcs-url)" to the description.
+- Should action.yml also add `gitea-url` as a deprecated input alias? The issue says "Update the action to pass the new env var name" — no mention of backward compat for the action input. Decision: rename only, no alias (action users pin a version anyway).
+- The bash-internal `GITEA_URL` variable in action.yml scripts (used for release download, not passed to binary) — rename for clarity? Decision: yes, rename to `BASE_URL` to avoid confusion with the env var.

README.md

@@ -282,7 +282,7 @@ Rules:
 
 ```bash
 review-bot \
-  --gitea-url https://gitea.example.com \
+  --vcs-url https://gitea.example.com \
   --repo owner/name \
   --pr 42 \
   --reviewer-token "$GITEA_TOKEN" \
@@ -299,7 +299,7 @@ All flags have environment variable equivalents:
 
 | Flag | Env Var |
 |------|---------|
-| `--gitea-url` | `GITEA_URL` |
+| `--vcs-url` | `VCS_URL` (fallback: `GITEA_URL`) |
 | `--repo` | `GITEA_REPO` |
 | `--pr` | `PR_NUMBER` |
 | `--reviewer-token` | `REVIEWER_TOKEN` |

cmd/review-bot/integration_test.go

@@ -17,7 +17,7 @@ import (
 // Integration test requires a running Gitea instance and LLM endpoint.
 // Set environment variables:
 //
-//	INTEGRATION_GITEA_URL     - Gitea base URL
+//	INTEGRATION_VCS_URL       - VCS base URL
 //	INTEGRATION_GITEA_TOKEN   - Gitea API token with repo access
 //	INTEGRATION_GITEA_REPO    - owner/repo with an open PR
 //	INTEGRATION_PR_NUMBER     - PR number to test against
@@ -25,7 +25,7 @@ import (
 //	INTEGRATION_LLM_API_KEY   - LLM API key
 //	INTEGRATION_LLM_MODEL     - Model name
 func TestIntegration_FullReviewFlow(t *testing.T) {
-	giteaURL := os.Getenv("INTEGRATION_GITEA_URL")
+	giteaURL := os.Getenv("INTEGRATION_VCS_URL")
 	giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN")
 	giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO")
 	prNumStr := os.Getenv("INTEGRATION_PR_NUMBER")
@@ -104,7 +104,7 @@ func TestIntegration_FullReviewFlow(t *testing.T) {
 }
 
 func TestIntegration_PostAndCleanup(t *testing.T) {
-	giteaURL := os.Getenv("INTEGRATION_GITEA_URL")
+	giteaURL := os.Getenv("INTEGRATION_VCS_URL")
 	giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN")
 	giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO")
 	prNumStr := os.Getenv("INTEGRATION_PR_NUMBER")
@@ -130,7 +130,7 @@ func TestIntegration_PostAndCleanup(t *testing.T) {
 	// Post a test review
 	sentinel := "<!-- review-bot:integration-test -->"
 	testBody := "# Integration Test Review\n\nThis is a test review.\n\n" + sentinel
-	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "COMMENT", testBody, nil)
+	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "COMMENT", testBody, "", nil)
 	if err != nil {
 		t.Fatalf("PostReview: %v", err)
 	}

cmd/review-bot/main.go

@@ -54,7 +54,8 @@ func main() {
 	logFormat := flag.String("log-format", envOrDefault("LOG_FORMAT", "text"), "Log output format: text or json")
 	verbosity := flag.String("verbosity", envOrDefault("LOG_VERBOSITY", "info"), "Log verbosity: debug, info, warn, error")
 	// CLI flags
-	giteaURL := flag.String("gitea-url", envOrDefault("GITEA_URL", ""), "Gitea instance URL")
+	vcsURL := flag.String("vcs-url", os.Getenv("VCS_URL"), "VCS server URL (e.g. https://gitea.example.com)")
+	giteaURLAlias := flag.String("gitea-url", "", "Deprecated: use --vcs-url")
 	repo := flag.String("repo", envOrDefault("GITEA_REPO", ""), "Repository (owner/name)")
 	prNum := flag.String("pr", envOrDefault("PR_NUMBER", ""), "Pull request number")
 	reviewerName := flag.String("reviewer-name", envOrDefault("REVIEWER_NAME", ""), "Reviewer display name")
@@ -91,12 +92,24 @@ func main() {
 
 	slog.Info("review-bot starting", "version", version)
 
+	// Backward compatibility: fall back to deprecated env var / flag if VCS_URL / --vcs-url not set.
+	if *vcsURL == "" {
+		if v := os.Getenv("GITEA_URL"); v != "" {
+			slog.Warn("GITEA_URL is deprecated; rename the environment variable to VCS_URL")
+			*vcsURL = v
+		}
+	}
+	if *vcsURL == "" && *giteaURLAlias != "" {
+		slog.Warn("--gitea-url is deprecated; use --vcs-url instead")
+		*vcsURL = *giteaURLAlias
+	}
+
 	// Validate required fields
 	// For aicore provider, llm-base-url and llm-api-key are not required
 	isAICore := llm.Provider(*llmProvider) == llm.ProviderAICore
-	if *giteaURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" || *llmModel == "" {
+	if *vcsURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" || *llmModel == "" {
 		fmt.Fprintf(os.Stderr, "Error: missing required flags or environment variables\n\n")
-		fmt.Fprintf(os.Stderr, "Required: --gitea-url, --repo, --pr, --reviewer-token, --llm-model\n")
+		fmt.Fprintf(os.Stderr, "Required: --vcs-url, --repo, --pr, --reviewer-token, --llm-model\n")
 		os.Exit(1)
 	}
 	if !isAICore && (*llmBaseURL == "" || *llmAPIKey == "") {
@@ -139,7 +152,7 @@ func main() {
 	}
 
 	// Initialize clients
-	giteaClient := gitea.NewClient(*giteaURL, *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")
@@ -444,7 +457,7 @@ func main() {
 
 	// POST new review
 	slog.Info("posting review", "event", event, "pr", prNumber)
-	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)
+	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, evaluatedSHA, inlineComments)
 	if err != nil {
 		slog.Error("failed to post review", "pr", prNumber, "event", event, "error", err)
 		os.Exit(1)
@@ -453,7 +466,7 @@ func main() {
 
 	// Supersede all old reviews with link to the new one
 	if len(oldReviews) > 0 {
-		newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*giteaURL, "/"), owner, repoName, prNumber, posted.ID)
+		newReviewURL := fmt.Sprintf("%s/%s/%s/pulls/%d#pullrequestreview-%d", strings.TrimRight(*vcsURL, "/"), owner, repoName, prNumber, posted.ID)
 		for _, oldReview := range oldReviews {
 			cid, err := giteaClient.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID)
 			if err != nil {

gitea/client.go

@@ -11,6 +11,7 @@ import (
 	"fmt"
 	"io"
 	"log/slog"
+	"math"
 	"net"
 	"net/http"
 	"net/url"
@@ -47,6 +48,12 @@ func IsServerError(err error) bool {
 	return errors.As(err, &apiErr) && apiErr.StatusCode >= 500 && apiErr.StatusCode < 600
 }
 
+// DefaultMaxDiffSize is the default maximum diff size in bytes (10 MB).
+const DefaultMaxDiffSize = 10 * 1024 * 1024
+
+// ErrDiffTooLarge is returned when a PR diff exceeds the configured MaxDiffSize.
+var ErrDiffTooLarge = errors.New("diff size exceeds maximum allowed size")
+
 // Client interacts with the Gitea API.
 // A Client is safe for concurrent use by multiple goroutines.
 type Client struct {
@@ -61,6 +68,42 @@ type Client struct {
 	// This field must be configured before the first request is made.
 	// Modifying it while requests are in flight is not safe.
 	RetryBackoff []time.Duration
+
+	// MaxDiffSize is the maximum number of bytes allowed when fetching a PR diff.
+	// If zero, defaults to DefaultMaxDiffSize (10 MB). Set to any negative value
+	// (or math.MaxInt64) to disable the limit.
+	//
+	// This field must be configured before the first request is made.
+	// Modifying it while requests are in flight is not safe.
+	MaxDiffSize int64
+}
+
+// defaultCheckRedirect is the redirect policy used by NewClient.
+// NOTE: This function is intentionally duplicated in github/client.go (and vice versa)
+// because the packages are separate. Changes here must be mirrored there.
+// It rejects HTTPS->HTTP protocol downgrades (to prevent plaintext leakage)
+// and cross-host redirects (to prevent following responses from untrusted
+// endpoints). Same-host, same-or-upgraded-scheme redirects are allowed.
+func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
+	if len(via) >= 10 {
+		return fmt.Errorf("stopped after 10 redirects")
+	}
+	// Guard for direct invocation in tests and any future callers;
+	// net/http guarantees len(via) >= 1 during actual redirects.
+	if len(via) == 0 {
+		return nil
+	}
+	prev := via[len(via)-1]
+	// Reject protocol downgrade: HTTPS->HTTP leaks request metadata over plaintext.
+	if prev.URL.Scheme == "https" && req.URL.Scheme == "http" {
+		return fmt.Errorf("refusing redirect: HTTPS to HTTP downgrade (%s -> %s)", prev.URL.Host, req.URL.Host)
+	}
+	// Reject cross-host redirect entirely to avoid consuming responses
+	// from untrusted endpoints.
+	if req.URL.Host != prev.URL.Host {
+		return fmt.Errorf("refusing redirect: cross-host (%s -> %s)", prev.URL.Host, req.URL.Host)
+	}
+	return nil
 }
 
 // NewClient creates a new Gitea API client.
@@ -68,13 +111,30 @@ func NewClient(baseURL, token string) *Client {
 	return &Client{
 		baseURL: strings.TrimRight(baseURL, "/"),
 		token:   token,
-		http:    &http.Client{Timeout: 30 * time.Second},
+		http: &http.Client{
+			Timeout:       30 * time.Second,
+			CheckRedirect: defaultCheckRedirect,
+		},
 	}
 }
 
 // SetHTTPClient sets the underlying HTTP client used for requests.
-// This is intended for testing to inject mock transports.
+// 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 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 = &http.Client{
+			Timeout:       30 * time.Second,
+			CheckRedirect: defaultCheckRedirect,
+		}
+	}
 	c.http = hc
 }
 
@@ -125,13 +185,32 @@ func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number
 }
 
 // GetPullRequestDiff fetches the unified diff for a PR.
+// It enforces MaxDiffSize to prevent unbounded memory allocation.
+// Returns ErrDiffTooLarge if the diff exceeds the configured limit.
 func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) {
 	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d.diff", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
+
+	maxSize := c.MaxDiffSize
+	if maxSize == 0 {
+		maxSize = DefaultMaxDiffSize
+	}
+
+	// When the limit is disabled (negative) or set to math.MaxInt64 (which
+	// would overflow the +1 detection and silently disable enforcement),
+	// use the standard unlimited doGet path.
+	if maxSize < 0 || maxSize == math.MaxInt64 {
 		body, err := c.doGet(ctx, reqURL)
 		if err != nil {
 			return "", fmt.Errorf("fetch diff: %w", err)
 		}
 		return string(body), nil
+	}
+
+	body, err := c.doGetLimited(ctx, reqURL, maxSize)
+	if err != nil {
+		return "", fmt.Errorf("fetch diff: %w", err)
+	}
+	return string(body), nil
 }
 
 // GetPullRequestFiles fetches the list of files changed in a PR.
@@ -183,18 +262,22 @@ func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, r
 }
 
 // PostReview submits a review to a PR and returns the created review.
-// event should be "APPROVED" or "REQUEST_CHANGES".
+// event should be one of "APPROVED", "REQUEST_CHANGES", or "COMMENT".
+// commitID anchors the review to a specific commit SHA. If empty, Gitea
+// defaults to the current PR head.
 // comments are optional inline comments attached to specific lines.
-func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string, comments []ReviewComment) (*Review, error) {
+func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []ReviewComment) (*Review, error) {
 	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
 
 	payload := struct {
 		Body     string          `json:"body"`
 		Event    string          `json:"event"`
+		CommitID string          `json:"commit_id,omitempty"`
 		Comments []ReviewComment `json:"comments,omitempty"`
 	}{
 		Body:     body,
 		Event:    event,
+		CommitID: commitID,
 		Comments: comments,
 	}
 
@@ -292,9 +375,9 @@ func isRetriableSyscallError(err error) bool {
 	return true
 }
 
-// redactURL strips query parameters from a URL for safe logging.
-// This prevents accidental exposure of sensitive data that future callers
-// might pass via query strings.
+// redactURL strips query parameters and userinfo credentials from a URL for
+// safe logging. This prevents accidental exposure of sensitive data (tokens in
+// query strings, or user:pass in the authority) in log output.
 func redactURL(rawURL string) string {
 	parsed, err := url.Parse(rawURL)
 	if err != nil {
@@ -302,6 +385,9 @@ func redactURL(rawURL string) string {
 		// potentially logging something sensitive.
 		return "[invalid URL]"
 	}
+	if parsed.User != nil {
+		parsed.User = url.User("REDACTED")
+	}
 	if parsed.RawQuery != "" {
 		parsed.RawQuery = "[redacted]"
 	}
@@ -322,10 +408,12 @@ func sanitizeErrorForLog(err error) string {
 	return err.Error()
 }
 
-// doGet performs an HTTP GET request with retry on 5xx errors and temporary
-// network errors. Retries up to 3 times with exponential backoff (1s, 2s delays
-// by default; configurable via Client.RetryBackoff for testing).
-func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
+// doGetWithReader performs an HTTP GET request with retry on 5xx errors and
+// temporary network errors. Retries up to 3 times with exponential backoff
+// (1s, 2s delays by default; configurable via Client.RetryBackoff for testing).
+// The readBody function is called with the response body on success (2xx) and
+// is responsible for reading and closing it.
+func (c *Client) doGetWithReader(ctx context.Context, reqURL string, readBody func(io.ReadCloser) ([]byte, error)) ([]byte, error) {
 	const maxAttempts = 3
 	// backoff[i] is the delay before attempt i+1 (i.e., after attempt i fails).
 	// First attempt (i=0) has no delay; retries wait 1s then 2s by default.
@@ -390,12 +478,7 @@ func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
 			return nil, lastErr
 		}
 		if resp.StatusCode >= 200 && resp.StatusCode < 300 {
-			body, err := io.ReadAll(resp.Body)
-			resp.Body.Close()
-			if err != nil {
-				return nil, err
-			}
-			return body, nil
+			return readBody(resp.Body)
 		}
 
 		// Error path: limit how much we read from potentially malicious server
@@ -413,6 +496,39 @@ func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
 	return nil, lastErr
 }
 
+// doGet performs an HTTP GET request with retry, reading the full response body.
+func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
+	return c.doGetWithReader(ctx, reqURL, func(body io.ReadCloser) ([]byte, error) {
+		defer body.Close()
+		return io.ReadAll(body)
+	})
+}
+
+// doGetLimited performs an HTTP GET request with retry but enforces a maximum
+// response body size. Returns ErrDiffTooLarge if the response exceeds maxBytes.
+// It reads maxBytes+1 (clamped to avoid overflow) to detect truncation without
+// buffering the entire body.
+func (c *Client) doGetLimited(ctx context.Context, reqURL string, maxBytes int64) ([]byte, error) {
+	return c.doGetWithReader(ctx, reqURL, func(body io.ReadCloser) ([]byte, error) {
+		defer body.Close()
+		// Read up to maxBytes+1 to detect overflow.
+		// Clamp to prevent integer overflow when maxBytes == math.MaxInt64.
+		limitBytes := maxBytes + 1
+		if limitBytes <= 0 {
+			limitBytes = math.MaxInt64
+		}
+		limited := io.LimitReader(body, limitBytes)
+		data, err := io.ReadAll(limited)
+		if err != nil {
+			return nil, err
+		}
+		if int64(len(data)) > maxBytes {
+			return nil, fmt.Errorf("%w: response exceeds %d bytes", ErrDiffTooLarge, maxBytes)
+		}
+		return data, nil
+	})
+}
+
 // escapePath escapes each segment of a relative file path for use in URLs.
 // Slashes are preserved as path separators; other special characters are escaped.
 // Input should be a relative path (no leading slash). Already-encoded segments

gitea/client_test.go

@@ -9,6 +9,7 @@ import (
 	"net"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"strings"
 	"sync/atomic"
 	"syscall"
@@ -118,6 +119,7 @@ func TestPostReview(t *testing.T) {
 		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 {
 			t.Fatalf("failed to decode payload: %v", err)
@@ -128,14 +130,16 @@ func TestPostReview(t *testing.T) {
 		if payload.Event != "APPROVED" {
 			t.Errorf("expected event %q, got %q", "APPROVED", payload.Event)
 		}
-
+		if payload.CommitID != "abc123def" {
+			t.Errorf("expected commit_id %q, got %q", "abc123def", payload.CommitID)
+		}
 		w.WriteHeader(http.StatusOK)
 		w.Write([]byte(`{"id":100,"user":{"login":"review-bot"},"state":"APPROVED","stale":false}`))
 	}))
 	defer server.Close()
 
 	client := NewClient(server.URL, "test-token")
-	review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", nil)
+	review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", "abc123def", nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -182,12 +186,35 @@ func TestPostReview_Non200(t *testing.T) {
 	defer server.Close()
 
 	client := NewClient(server.URL, "test-token")
-	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", nil)
+	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", "", nil)
 	if err == nil {
 		t.Fatal("expected error for 403, got nil")
 	}
 }
 
+func TestPostReview_EmptyCommitID_OmittedFromPayload(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		body, _ := io.ReadAll(r.Body)
+		var raw map[string]interface{}
+		if err := json.Unmarshal(body, &raw); err != nil {
+			t.Fatalf("failed to decode payload: %v", err)
+		}
+		if _, exists := raw["commit_id"]; exists {
+			t.Errorf("expected commit_id to be omitted from payload when empty, but it was present")
+		}
+
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte(`{"id":200,"user":{"login":"bot"},"state":"APPROVED","stale":false}`))
+	}))
+	defer server.Close()
+
+	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)
+	}
+}
+
 func TestGetFileContent(t *testing.T) {
 	expected := "# Conventions\n- Be nice\n"
 
@@ -944,8 +971,6 @@ func TestDoGet_RespectsContextCancellation(t *testing.T) {
 		t.Errorf("attempts = %d, expected 1 before context cancel during backoff", attempts)
 	}
 }
-
-
 // mockTransport is a test helper that returns errors for the first N calls,
 // then delegates to a real server.
 type mockTransport struct {
@@ -1092,6 +1117,21 @@ func TestRedactURL(t *testing.T) {
 			input: "",
 			want:  "",
 		},
+		{
+			name:  "with userinfo - redacts credentials",
+			input: "https://admin:secret@gitea.example.com/api/v1/repos",
+			want:  "https://REDACTED@gitea.example.com/api/v1/repos",
+		},
+		{
+			name:  "with userinfo and query params",
+			input: "https://user:pass@example.com/path?token=abc",
+			want:  "https://REDACTED@example.com/path?[redacted]",
+		},
+		{
+			name:  "username only - no password",
+			input: "https://user@example.com/path",
+			want:  "https://REDACTED@example.com/path",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -1144,3 +1184,104 @@ func TestSanitizeErrorForLog(t *testing.T) {
 		})
 	}
 }
+
+func TestNewClient_HasCheckRedirect(t *testing.T) {
+	c := NewClient("https://gitea.example.com", "token")
+	if c.http.CheckRedirect == nil {
+		t.Fatal("expected CheckRedirect to be set")
+	}
+}
+
+func TestDefaultCheckRedirect_RejectsHTTPSToHTTP(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "http", Host: "gitea.example.com", Path: "/foo"},
+		Header: http.Header{"Authorization": []string{"token abc"}},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err == nil {
+		t.Fatal("expected error on HTTPS->HTTP redirect")
+	}
+	if !strings.Contains(err.Error(), "HTTPS to HTTP downgrade") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_RejectsCrossHost(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "https", Host: "cdn.example.com", Path: "/bar"},
+		Header: http.Header{"Authorization": []string{"token abc"}},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err == nil {
+		t.Fatal("expected error on cross-host redirect")
+	}
+	if !strings.Contains(err.Error(), "cross-host") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_AllowsSameHost(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/bar"},
+		Header: http.Header{"Authorization": []string{"token abc"}},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if auth := req.Header.Get("Authorization"); auth != "token abc" {
+		t.Errorf("expected Authorization to be preserved, got %q", auth)
+	}
+}
+
+func TestDefaultCheckRedirect_AllowsSameHostHTTPToHTTP(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "http", Host: "localhost:3000", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "http", Host: "localhost:3000", Path: "/bar"},
+		Header: http.Header{},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_RejectsTooManyRedirects(t *testing.T) {
+	via := make([]*http.Request, 10)
+	for i := range via {
+		via[i] = &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/"}}
+	}
+	req := &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/final"}}
+	err := defaultCheckRedirect(req, via)
+	if err == nil {
+		t.Fatal("expected error after 10 redirects")
+	}
+	if !strings.Contains(err.Error(), "10 redirects") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_EmptyViaAllowed(t *testing.T) {
+	req := &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/foo"}}
+	err := defaultCheckRedirect(req, nil)
+	if err != nil {
+		t.Fatalf("unexpected error with empty via: %v", err)
+	}
+}
+
+func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
+	c := NewClient("https://gitea.example.com", "token")
+	c.SetHTTPClient(nil)
+	if c.http == nil {
+		t.Fatal("expected non-nil http client after SetHTTPClient(nil)")
+	}
+	if c.http.Timeout != 30*time.Second {
+		t.Errorf("expected 30s timeout, got %v", c.http.Timeout)
+	}
+	if c.http.CheckRedirect == nil {
+		t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)")
+	}
+}

gitea/diff_size_test.go

@@ -0,0 +1,97 @@
+package gitea
+
+import (
+	"context"
+	"errors"
+	"math"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"testing"
+	"time"
+)
+
+func TestGetPullRequestDiff_SizeLimits(t *testing.T) {
+	tests := []struct {
+		name        string
+		diff        string
+		maxDiffSize int64
+		wantErr     error
+		wantDiff    string
+	}{
+		{
+			name:        "exceeds max size",
+			diff:        strings.Repeat("+ added line\n", 1000), // ~13 KB
+			maxDiffSize: 100,
+			wantErr:     ErrDiffTooLarge,
+		},
+		{
+			name:        "within max size",
+			diff:        "diff --git a/f.go b/f.go\n--- a/f.go\n+++ b/f.go\n@@ -1 +1 @@\n-old\n+new\n",
+			maxDiffSize: 1024,
+			wantDiff:    "diff --git a/f.go b/f.go\n--- a/f.go\n+++ b/f.go\n@@ -1 +1 @@\n-old\n+new\n",
+		},
+		{
+			name:        "exactly at limit",
+			diff:        strings.Repeat("x", 50),
+			maxDiffSize: 50,
+			wantDiff:    strings.Repeat("x", 50),
+		},
+		{
+			name:        "one byte over limit",
+			diff:        strings.Repeat("x", 51),
+			maxDiffSize: 50,
+			wantErr:     ErrDiffTooLarge,
+		},
+		{
+			name:        "disabled limit",
+			diff:        strings.Repeat("x", 10000),
+			maxDiffSize: -1,
+			wantDiff:    strings.Repeat("x", 10000),
+		},
+		{
+			name:        "math.MaxInt64 treated as disabled",
+			diff:        strings.Repeat("x", 10000),
+			maxDiffSize: math.MaxInt64,
+			wantDiff:    strings.Repeat("x", 10000),
+		},
+		{
+			name:        "default limit",
+			diff:        "diff content",
+			maxDiffSize: 0, // zero means use DefaultMaxDiffSize
+			wantDiff:    "diff content",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+				w.Write([]byte(tt.diff)) //nolint:errcheck // test handler
+			}))
+			defer server.Close()
+
+			client := NewClient(server.URL, "test-token")
+			client.MaxDiffSize = tt.maxDiffSize
+			client.RetryBackoff = []time.Duration{}
+
+			got, err := client.GetPullRequestDiff(context.Background(), "owner", "repo", 1)
+
+			if tt.wantErr != nil {
+				if err == nil {
+					t.Fatal("expected error, got nil")
+				}
+				if !errors.Is(err, tt.wantErr) {
+					t.Errorf("expected %v, got: %v", tt.wantErr, err)
+				}
+				return
+			}
+
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if got != tt.wantDiff {
+				t.Errorf("diff mismatch: got length %d, want length %d", len(got), len(tt.wantDiff))
+			}
+		})
+	}
+}

gitea/post_review_comments_test.go

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

github/client.go

@@ -8,7 +8,10 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"log/slog"
 	"net/http"
+	"net/url"
+	"os"
 	"strconv"
 	"strings"
 	"time"
@@ -97,6 +100,10 @@ type Client struct {
 	token      string
 	httpClient *http.Client
 
+	// allowInsecureHTTP permits requests to HTTP (non-TLS) endpoints.
+	// When false, doRequest rejects URLs with an http:// scheme.
+	allowInsecureHTTP bool
+
 	// retryBackoff defines the delays between retry attempts for 429 responses.
 	// retryBackoff[i] is the delay before attempt i+1 (after attempt i fails).
 	// If nil, defaults to {1s, 2s}.
@@ -107,24 +114,106 @@ type Client struct {
 	now func() time.Time
 }
 
+// defaultCheckRedirect is the redirect policy used by NewClient.
+// NOTE: This function is intentionally duplicated in gitea/client.go (and vice versa)
+// because the packages are separate. Changes here must be mirrored there.
+// It rejects HTTPS->HTTP protocol downgrades (to prevent plaintext leakage)
+// and cross-host redirects (to prevent following responses from untrusted
+// endpoints). Same-host, same-or-upgraded-scheme redirects are allowed.
+func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
+	if len(via) >= 10 {
+		return fmt.Errorf("stopped after 10 redirects")
+	}
+	// Guard for direct invocation in tests and any future callers;
+	// net/http guarantees len(via) >= 1 during actual redirects.
+	if len(via) == 0 {
+		return nil
+	}
+	prev := via[len(via)-1]
+	// Reject protocol downgrade: HTTPS->HTTP leaks request metadata over plaintext.
+	if prev.URL.Scheme == "https" && req.URL.Scheme == "http" {
+		return fmt.Errorf("refusing redirect: HTTPS to HTTP downgrade (%s -> %s)", prev.URL.Host, req.URL.Host)
+	}
+	// Reject cross-host redirect entirely to avoid consuming responses
+	// from untrusted endpoints.
+	if req.URL.Host != prev.URL.Host {
+		return fmt.Errorf("refusing redirect: cross-host (%s -> %s)", prev.URL.Host, req.URL.Host)
+	}
+	return nil
+}
+
+// ClientOption configures optional behavior of a Client.
+type ClientOption func(*clientConfig)
+
+type clientConfig struct {
+	allowInsecureHTTP    bool
+	insecureIsTestBypass bool
+}
+
+// AllowInsecureHTTP permits sending credentials over plaintext HTTP connections.
+// In production, this option is gated by the REVIEW_BOT_ALLOW_INSECURE=1
+// environment variable. Without the env var set, the option is ignored
+// and a warning is logged.
+//
+// For tests, use AllowInsecureHTTPForTest (defined in a _test.go file in the same package) which bypasses the env gate.
+func AllowInsecureHTTP() ClientOption {
+	return func(cfg *clientConfig) {
+		cfg.allowInsecureHTTP = true
+	}
+}
+
 // NewClient creates a new GitHub API client.
 // If baseURL is empty, it defaults to https://api.github.com.
 // For GitHub Enterprise, pass the API base URL (e.g. https://github.concur.com/api/v3).
-func NewClient(token, baseURL string) *Client {
+func NewClient(token, baseURL string, opts ...ClientOption) *Client {
 	if baseURL == "" {
 		baseURL = defaultBaseURL
 	}
+
+	var cfg clientConfig
+	for _, opt := range opts {
+		opt(&cfg)
+	}
+
+	if cfg.allowInsecureHTTP && !cfg.insecureIsTestBypass {
+		if os.Getenv("REVIEW_BOT_ALLOW_INSECURE") != "1" {
+			slog.Warn("AllowInsecureHTTP ignored: set REVIEW_BOT_ALLOW_INSECURE=1 to enable")
+			cfg.allowInsecureHTTP = false
+		} else {
+			slog.Warn("AllowInsecureHTTP enabled — credentials may be sent over plaintext",
+				"env", "REVIEW_BOT_ALLOW_INSECURE=1")
+		}
+	}
+
 	return &Client{
 		baseURL:           strings.TrimRight(baseURL, "/"),
 		token:             token,
-		httpClient: &http.Client{Timeout: 30 * time.Second},
+		allowInsecureHTTP: cfg.allowInsecureHTTP,
+		httpClient: &http.Client{
+			Timeout:       30 * time.Second,
+			CheckRedirect: defaultCheckRedirect,
+		},
 		now: time.Now,
 	}
 }
 
 // SetHTTPClient sets the underlying HTTP client used for requests.
-// This is intended for testing to inject mock transports.
+// 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 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 = &http.Client{
+			Timeout:       30 * time.Second,
+			CheckRedirect: defaultCheckRedirect,
+		}
+	}
 	c.httpClient = hc
 }
 
@@ -170,10 +259,39 @@ func (c *Client) parseRetryAfter(value string) (time.Duration, bool) {
 	return 0, false
 }
 
+// redactURL redacts sensitive components from a URL for safe inclusion in error
+// messages and log output. It removes userinfo (e.g., user:pass@) and replaces
+// query parameters with a placeholder.
+func redactURL(rawURL string) string {
+	u, err := url.Parse(rawURL)
+	if err != nil {
+		return "<unparseable URL>"
+	}
+	u.User = nil
+
+	if u.RawQuery != "" {
+		u.RawQuery = "<redacted>"
+	}
+	return u.String()
+}
+
 // doRequest performs an HTTP request with retry on 429 rate limit responses.
 // It respects the Retry-After header when present, supporting both integer
 // seconds and HTTP-date formats (capped at maxRetryAfter).
 func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
+	// NOTE: This parses reqURL a second time (http.NewRequestWithContext parses it
+	// again internally). Acceptable cost: URL parsing is cheap and threading the
+	// parsed *url.URL through would complicate the interface for negligible gain.
+	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 backoff []time.Duration
 	if c.retryBackoff != nil {
 		backoff = append([]time.Duration(nil), c.retryBackoff...)
@@ -192,7 +310,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
 				timer := time.NewTimer(delay)
 				select {
 				case <-timer.C:
-					timer.Stop()
+					timer.Stop() // no-op after fire; kept for symmetry with the ctx.Done case
 				case <-ctx.Done():
 					timer.Stop()
 					return nil, ctx.Err()

github/client_test.go

@@ -5,6 +5,8 @@ import (
 	"errors"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
+	"strings"
 	"testing"
 	"time"
 )
@@ -33,7 +35,7 @@ func TestDoRequest_Success(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("test-token", srv.URL)
+	c := NewClient("test-token", srv.URL, AllowInsecureHTTPForTest())
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
@@ -58,7 +60,7 @@ func TestDoRequest_429_RetryAfter_IntegerSeconds(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL)
+	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
 	c.SetRetryBackoff([]time.Duration{0, 0})
 
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -92,7 +94,7 @@ func TestDoRequest_429_RetryAfter_HTTPDate(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL)
+	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
 	c.now = func() time.Time { return fixedNow }
 	// Initial backoff is 0; the HTTP-date parser will compute 1s and override.
 	c.SetRetryBackoff([]time.Duration{0, 0})
@@ -128,7 +130,7 @@ func TestDoRequest_429_RetryAfter_HTTPDate_InPast(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL)
+	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
 	c.now = func() time.Time { return fixedNow }
 	c.SetRetryBackoff([]time.Duration{0, 0})
 
@@ -155,7 +157,7 @@ func TestDoRequest_429_NoRetryAfter_UsesDefaultBackoff(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL)
+	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
 	c.SetRetryBackoff([]time.Duration{0, 0})
 
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -185,7 +187,7 @@ func TestDoRequest_429_InvalidRetryAfter_UsesDefaultBackoff(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL)
+	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
 	c.SetRetryBackoff([]time.Duration{0, 0})
 
 	body, err := c.doGet(context.Background(), srv.URL+"/test")
@@ -206,7 +208,7 @@ func TestDoRequest_404_NoRetry(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL)
+	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
 	_, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err == nil {
 		t.Fatal("expected error, got nil")
@@ -228,7 +230,7 @@ func TestDoRequest_401_NoRetry(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL)
+	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
 	_, err := c.doGet(context.Background(), srv.URL+"/test")
 	if err == nil {
 		t.Fatal("expected error, got nil")
@@ -258,7 +260,7 @@ func TestDoRequest_ContextCanceled(t *testing.T) {
 	}))
 	defer srv.Close()
 
-	c := NewClient("tok", srv.URL)
+	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
 	c.SetRetryBackoff([]time.Duration{10 * time.Second, 10 * time.Second})
 
 	ctx, cancel := context.WithCancel(context.Background())
@@ -407,3 +409,250 @@ func TestAPIError_Error_NewlineSanitized(t *testing.T) {
 		}
 	}
 }
+
+func TestNewClient_HasCheckRedirect(t *testing.T) {
+	c := NewClient("secret-token", "https://api.github.com")
+	if c.httpClient.CheckRedirect == nil {
+		t.Fatal("expected CheckRedirect to be set")
+	}
+}
+
+func TestDefaultCheckRedirect_RejectsHTTPSToHTTP(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "http", Host: "api.github.com", Path: "/foo"},
+		Header: http.Header{"Authorization": []string{"Bearer token"}},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err == nil {
+		t.Fatal("expected error on HTTPS->HTTP redirect")
+	}
+	if !strings.Contains(err.Error(), "HTTPS to HTTP downgrade") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_RejectsCrossHost(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "https", Host: "objects.githubusercontent.com", Path: "/bar"},
+		Header: http.Header{"Authorization": []string{"Bearer token"}},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err == nil {
+		t.Fatal("expected error on cross-host redirect")
+	}
+	if !strings.Contains(err.Error(), "cross-host") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_AllowsSameHost(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "https", Host: "api.github.com", Path: "/bar"},
+		Header: http.Header{"Authorization": []string{"Bearer token"}},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	// Auth should be preserved on same-host redirect
+	if auth := req.Header.Get("Authorization"); auth != "Bearer token" {
+		t.Errorf("expected Authorization to be preserved, got %q", auth)
+	}
+}
+
+func TestDefaultCheckRedirect_AllowsSameHostHTTPToHTTP(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "http", Host: "localhost:8080", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "http", Host: "localhost:8080", Path: "/bar"},
+		Header: http.Header{},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_RejectsTooManyRedirects(t *testing.T) {
+	via := make([]*http.Request, 10)
+	for i := range via {
+		via[i] = &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/"}}
+	}
+	req := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/final"}}
+	err := defaultCheckRedirect(req, via)
+	if err == nil {
+		t.Fatal("expected error after 10 redirects")
+	}
+	if !strings.Contains(err.Error(), "10 redirects") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_EmptyViaAllowed(t *testing.T) {
+	req := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
+	err := defaultCheckRedirect(req, nil)
+	if err != nil {
+		t.Fatalf("unexpected error with empty via: %v", err)
+	}
+}
+
+func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
+	c := NewClient("token", "https://api.github.com")
+	c.SetHTTPClient(nil)
+	if c.httpClient == nil {
+		t.Fatal("expected non-nil httpClient after SetHTTPClient(nil)")
+	}
+	if c.httpClient.Timeout != 30*time.Second {
+		t.Errorf("expected 30s timeout, got %v", c.httpClient.Timeout)
+	}
+	if c.httpClient.CheckRedirect == nil {
+		t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)")
+	}
+}
+
+func TestAllowInsecureHTTPForTest_PermitsHTTP(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte("ok"))
+	}))
+	defer srv.Close()
+
+	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
+	body, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if string(body) != "ok" {
+		t.Errorf("body = %q, want %q", body, "ok")
+	}
+}
+
+func TestNoInsecureOption_RejectsHTTP(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		t.Fatal("request should not have been sent")
+	}))
+	defer srv.Close()
+
+	c := NewClient("tok", srv.URL)
+	_, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err == nil {
+		t.Fatal("expected error for HTTP request without AllowInsecureHTTP")
+	}
+	if !strings.Contains(err.Error(), "refusing HTTP request") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestNoInsecureOption_RejectsUppercaseHTTP(t *testing.T) {
+	// Verify case-insensitive scheme check (RFC 3986).
+	c := NewClient("tok", "HTTP://127.0.0.1:1")
+	_, err := c.doGet(context.Background(), "HTTP://127.0.0.1:1/test")
+	if err == nil {
+		t.Fatal("expected error for uppercase HTTP scheme")
+	}
+	if !strings.Contains(err.Error(), "refusing HTTP request") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestNoInsecureOption_RejectsMixedCaseHTTP(t *testing.T) {
+	// Verify mixed case like "Http://" is also rejected.
+	c := NewClient("tok", "Http://127.0.0.1:1")
+	_, err := c.doGet(context.Background(), "Http://127.0.0.1:1/test")
+	if err == nil {
+		t.Fatal("expected error for mixed-case HTTP scheme")
+	}
+	if !strings.Contains(err.Error(), "refusing HTTP request") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestAllowInsecureHTTP_WithoutEnvVar_Rejected(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		t.Fatal("request should not have been sent")
+	}))
+	defer srv.Close()
+
+	t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "")
+
+	c := NewClient("tok", srv.URL, AllowInsecureHTTP())
+	_, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err == nil {
+		t.Fatal("expected error: AllowInsecureHTTP without env var should be rejected")
+	}
+	if !strings.Contains(err.Error(), "refusing HTTP request") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestAllowInsecureHTTP_WithEnvVar_Permitted(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte("insecure-ok"))
+	}))
+	defer srv.Close()
+
+	t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "1")
+
+	c := NewClient("tok", srv.URL, AllowInsecureHTTP())
+	body, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if string(body) != "insecure-ok" {
+		t.Errorf("body = %q, want %q", body, "insecure-ok")
+	}
+}
+
+func TestAllowInsecureHTTP_EnvVarNotOne_Rejected(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		t.Fatal("request should not have been sent")
+	}))
+	defer srv.Close()
+
+	// "true" is not "1" — strict check
+	t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "true")
+
+	c := NewClient("tok", srv.URL, AllowInsecureHTTP())
+	_, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err == nil {
+		t.Fatal("expected error: env var 'true' is not '1'")
+	}
+	if !strings.Contains(err.Error(), "refusing HTTP request") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestRedactURL_WithQuery(t *testing.T) {
+	got := redactURL("http://localhost:1234/path?secret=token&foo=bar")
+	want := "http://localhost:1234/path?<redacted>"
+	if got != want {
+		t.Errorf("redactURL = %q, want %q", got, want)
+	}
+}
+
+func TestRedactURL_NoQuery(t *testing.T) {
+	got := redactURL("http://localhost:1234/path")
+	want := "http://localhost:1234/path"
+	if got != want {
+		t.Errorf("redactURL = %q, want %q", got, want)
+	}
+}
+
+func TestRedactURL_Userinfo(t *testing.T) {
+	got := redactURL("http://user:pass@localhost:1234/path")
+	want := "http://localhost:1234/path"
+	if got != want {
+		t.Errorf("redactURL = %q, want %q", got, want)
+	}
+}
+
+func TestRedactURL_UserinfoWithQuery(t *testing.T) {
+	got := redactURL("http://user:pass@localhost:1234/path?secret=token")
+	want := "http://localhost:1234/path?<redacted>"
+	if got != want {
+		t.Errorf("redactURL = %q, want %q", got, want)
+	}
+}

github/export_test.go

@@ -0,0 +1,13 @@
+package github
+
+// AllowInsecureHTTPForTest permits sending credentials over plaintext HTTP
+// without requiring the REVIEW_BOT_ALLOW_INSECURE environment variable.
+// This is intended exclusively for test code using httptest.Server.
+//
+// Defined in a _test.go file so it is only available to test binaries.
+func AllowInsecureHTTPForTest() ClientOption {
+	return func(cfg *clientConfig) {
+		cfg.allowInsecureHTTP = true
+		cfg.insecureIsTestBypass = true
+	}
+}

integration_test.go

@@ -16,7 +16,8 @@ import (
 
 // Integration test requires a running Gitea instance and LLM endpoint.
 // Set environment variables:
-//   INTEGRATION_GITEA_URL     - Gitea base URL
+//
+//	INTEGRATION_VCS_URL       - VCS base URL
 //	INTEGRATION_GITEA_TOKEN   - Gitea API token with repo access
 //	INTEGRATION_GITEA_REPO    - owner/repo with an open PR
 //	INTEGRATION_PR_NUMBER     - PR number to test against
@@ -25,7 +26,7 @@ import (
 //	INTEGRATION_LLM_MODEL     - Model name
 
 func TestIntegration_FullReviewFlow(t *testing.T) {
-	giteaURL := os.Getenv("INTEGRATION_GITEA_URL")
+	giteaURL := os.Getenv("INTEGRATION_VCS_URL")
 	giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN")
 	giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO")
 	prNumStr := os.Getenv("INTEGRATION_PR_NUMBER")