Compare commits
17 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
| bd2df7d986 | |||
| d3bb83a10a | |||
| c56f5fec52 | |||
| b80a1517ed | |||
| 5f7ffab487 | |||
| f8b9d7d282 | |||
| 7a8fc166ec | |||
| 5e351b85f0 | |||
| ab2a6c8aef | |||
| 6b7f3f6924 | |||
| 4c032a3b53 | |||
| 64c9d551ba | |||
| db7b7e66bf | |||
| 0232343126 | |||
| b26514714f | |||
| 028d46942a | |||
| e59c2bc831 |
@@ -1,17 +1,37 @@
|
|||||||
# This composite action is designed for Gitea Actions runners.
|
# This composite action supports both Gitea Actions and GitHub Actions runners.
|
||||||
# Gitea Actions supports GitHub Actions syntax including $GITHUB_OUTPUT,
|
# It detects the VCS host type by checking whether github.api_url is set
|
||||||
# actions/cache, and actions/checkout.
|
# (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).
|
# Requirements: python3, sha256sum, curl (all present on ubuntu-* runners).
|
||||||
name: 'AI Code Review'
|
name: 'AI Code Review'
|
||||||
description: 'Run AI-powered code review on a pull request using review-bot'
|
description: 'Run AI-powered code review on a pull request using review-bot'
|
||||||
|
|
||||||
inputs:
|
inputs:
|
||||||
gitea-url:
|
vcs-url:
|
||||||
description: 'Gitea instance URL (defaults to server_url)'
|
description: 'VCS server URL (only used on Gitea runners; ignored on GitHub/GHES). Defaults to server_url.'
|
||||||
required: false
|
required: false
|
||||||
default: ''
|
default: ''
|
||||||
repo:
|
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
|
required: false
|
||||||
default: ''
|
default: ''
|
||||||
pr-number:
|
pr-number:
|
||||||
@@ -19,7 +39,7 @@ inputs:
|
|||||||
required: false
|
required: false
|
||||||
default: ''
|
default: ''
|
||||||
reviewer-token:
|
reviewer-token:
|
||||||
description: 'Gitea token for posting the review'
|
description: 'Token for posting the review'
|
||||||
required: true
|
required: true
|
||||||
reviewer-name:
|
reviewer-name:
|
||||||
description: 'Display name for the reviewer'
|
description: 'Display name for the reviewer'
|
||||||
@@ -112,45 +132,265 @@ runs:
|
|||||||
id: version
|
id: version
|
||||||
shell: bash
|
shell: bash
|
||||||
run: |
|
run: |
|
||||||
GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
|
set -euo pipefail
|
||||||
REPO="${{ inputs.repo || 'rodin/review-bot' }}"
|
|
||||||
|
# --- 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
|
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 '')")
|
| 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
|
if [ -z "$VERSION" ]; then
|
||||||
echo "Failed to determine latest version" >&2
|
echo "Failed to determine latest version from ${API_URL}" >&2
|
||||||
exit 1
|
exit 1
|
||||||
fi
|
fi
|
||||||
else
|
else
|
||||||
VERSION="${{ inputs.version }}"
|
VERSION="${{ inputs.version }}"
|
||||||
fi
|
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 "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
|
- name: Cache review-bot binary
|
||||||
id: cache
|
id: cache
|
||||||
uses: actions/cache@v4
|
uses: actions/cache@v4
|
||||||
with:
|
with:
|
||||||
path: ${{ runner.temp }}/review-bot
|
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
|
- name: Install review-bot
|
||||||
if: steps.cache.outputs.cache-hit != 'true'
|
if: steps.cache.outputs.cache-hit != 'true'
|
||||||
shell: bash
|
shell: bash
|
||||||
run: |
|
run: |
|
||||||
GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
|
set -euo pipefail
|
||||||
REPO="${{ inputs.repo || 'rodin/review-bot' }}"
|
|
||||||
VERSION="${{ steps.version.outputs.version }}"
|
|
||||||
BINARY="review-bot-linux-amd64"
|
|
||||||
|
|
||||||
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"
|
-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"
|
-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
|
# 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 }}"
|
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}')
|
ACTUAL=$(sha256sum review-bot | awk '{print $1}')
|
||||||
|
fi
|
||||||
|
|
||||||
if [ -z "$EXPECTED" ]; then
|
if [ -z "$EXPECTED" ]; then
|
||||||
echo "Error: no checksum found for ${BINARY}" >&2
|
echo "Error: no checksum found for ${BINARY}" >&2
|
||||||
@@ -164,12 +404,12 @@ runs:
|
|||||||
fi
|
fi
|
||||||
|
|
||||||
chmod +x "${{ runner.temp }}/review-bot"
|
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
|
- name: Run review
|
||||||
shell: bash
|
shell: bash
|
||||||
env:
|
env:
|
||||||
GITEA_URL: ${{ inputs.gitea-url || github.server_url }}
|
VCS_URL: ${{ steps.version.outputs.server_url }}
|
||||||
GITEA_REPO: ${{ inputs.repo || github.repository }}
|
GITEA_REPO: ${{ inputs.repo || github.repository }}
|
||||||
PR_NUMBER: ${{ inputs.pr-number || github.event.pull_request.number }}
|
PR_NUMBER: ${{ inputs.pr-number || github.event.pull_request.number }}
|
||||||
REVIEWER_TOKEN: ${{ inputs.reviewer-token }}
|
REVIEWER_TOKEN: ${{ inputs.reviewer-token }}
|
||||||
|
|||||||
@@ -49,7 +49,7 @@ jobs:
|
|||||||
- run: go build -o review-bot ./cmd/review-bot
|
- run: go build -o review-bot ./cmd/review-bot
|
||||||
- name: Run ${{ matrix.name }} review
|
- name: Run ${{ matrix.name }} review
|
||||||
env:
|
env:
|
||||||
GITEA_URL: ${{ github.server_url }}
|
VCS_URL: ${{ github.server_url }}
|
||||||
GITEA_REPO: ${{ github.repository }}
|
GITEA_REPO: ${{ github.repository }}
|
||||||
PR_NUMBER: ${{ github.event.pull_request.number }}
|
PR_NUMBER: ${{ github.event.pull_request.number }}
|
||||||
REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }}
|
REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }}
|
||||||
|
|||||||
+175
@@ -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.
|
||||||
@@ -282,7 +282,7 @@ Rules:
|
|||||||
|
|
||||||
```bash
|
```bash
|
||||||
review-bot \
|
review-bot \
|
||||||
--gitea-url https://gitea.example.com \
|
--vcs-url https://gitea.example.com \
|
||||||
--repo owner/name \
|
--repo owner/name \
|
||||||
--pr 42 \
|
--pr 42 \
|
||||||
--reviewer-token "$GITEA_TOKEN" \
|
--reviewer-token "$GITEA_TOKEN" \
|
||||||
@@ -299,7 +299,7 @@ All flags have environment variable equivalents:
|
|||||||
|
|
||||||
| Flag | Env Var |
|
| Flag | Env Var |
|
||||||
|------|---------|
|
|------|---------|
|
||||||
| `--gitea-url` | `GITEA_URL` |
|
| `--vcs-url` | `VCS_URL` (fallback: `GITEA_URL`) |
|
||||||
| `--repo` | `GITEA_REPO` |
|
| `--repo` | `GITEA_REPO` |
|
||||||
| `--pr` | `PR_NUMBER` |
|
| `--pr` | `PR_NUMBER` |
|
||||||
| `--reviewer-token` | `REVIEWER_TOKEN` |
|
| `--reviewer-token` | `REVIEWER_TOKEN` |
|
||||||
|
|||||||
@@ -17,7 +17,7 @@ import (
|
|||||||
// Integration test requires a running Gitea instance and LLM endpoint.
|
// Integration test requires a running Gitea instance and LLM endpoint.
|
||||||
// Set environment variables:
|
// 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_TOKEN - Gitea API token with repo access
|
||||||
// INTEGRATION_GITEA_REPO - owner/repo with an open PR
|
// INTEGRATION_GITEA_REPO - owner/repo with an open PR
|
||||||
// INTEGRATION_PR_NUMBER - PR number to test against
|
// INTEGRATION_PR_NUMBER - PR number to test against
|
||||||
@@ -25,7 +25,7 @@ import (
|
|||||||
// INTEGRATION_LLM_API_KEY - LLM API key
|
// INTEGRATION_LLM_API_KEY - LLM API key
|
||||||
// INTEGRATION_LLM_MODEL - Model name
|
// INTEGRATION_LLM_MODEL - Model name
|
||||||
func TestIntegration_FullReviewFlow(t *testing.T) {
|
func TestIntegration_FullReviewFlow(t *testing.T) {
|
||||||
giteaURL := os.Getenv("INTEGRATION_GITEA_URL")
|
giteaURL := os.Getenv("INTEGRATION_VCS_URL")
|
||||||
giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN")
|
giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN")
|
||||||
giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO")
|
giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO")
|
||||||
prNumStr := os.Getenv("INTEGRATION_PR_NUMBER")
|
prNumStr := os.Getenv("INTEGRATION_PR_NUMBER")
|
||||||
@@ -104,7 +104,7 @@ func TestIntegration_FullReviewFlow(t *testing.T) {
|
|||||||
}
|
}
|
||||||
|
|
||||||
func TestIntegration_PostAndCleanup(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")
|
giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN")
|
||||||
giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO")
|
giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO")
|
||||||
prNumStr := os.Getenv("INTEGRATION_PR_NUMBER")
|
prNumStr := os.Getenv("INTEGRATION_PR_NUMBER")
|
||||||
@@ -130,7 +130,7 @@ func TestIntegration_PostAndCleanup(t *testing.T) {
|
|||||||
// Post a test review
|
// Post a test review
|
||||||
sentinel := "<!-- review-bot:integration-test -->"
|
sentinel := "<!-- review-bot:integration-test -->"
|
||||||
testBody := "# Integration Test Review\n\nThis is a test review.\n\n" + sentinel
|
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 {
|
if err != nil {
|
||||||
t.Fatalf("PostReview: %v", err)
|
t.Fatalf("PostReview: %v", err)
|
||||||
}
|
}
|
||||||
|
|||||||
+19
-6
@@ -54,7 +54,8 @@ func main() {
|
|||||||
logFormat := flag.String("log-format", envOrDefault("LOG_FORMAT", "text"), "Log output format: text or json")
|
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")
|
verbosity := flag.String("verbosity", envOrDefault("LOG_VERBOSITY", "info"), "Log verbosity: debug, info, warn, error")
|
||||||
// CLI flags
|
// 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)")
|
repo := flag.String("repo", envOrDefault("GITEA_REPO", ""), "Repository (owner/name)")
|
||||||
prNum := flag.String("pr", envOrDefault("PR_NUMBER", ""), "Pull request number")
|
prNum := flag.String("pr", envOrDefault("PR_NUMBER", ""), "Pull request number")
|
||||||
reviewerName := flag.String("reviewer-name", envOrDefault("REVIEWER_NAME", ""), "Reviewer display name")
|
reviewerName := flag.String("reviewer-name", envOrDefault("REVIEWER_NAME", ""), "Reviewer display name")
|
||||||
@@ -91,12 +92,24 @@ func main() {
|
|||||||
|
|
||||||
slog.Info("review-bot starting", "version", version)
|
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
|
// Validate required fields
|
||||||
// For aicore provider, llm-base-url and llm-api-key are not required
|
// For aicore provider, llm-base-url and llm-api-key are not required
|
||||||
isAICore := llm.Provider(*llmProvider) == llm.ProviderAICore
|
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, "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)
|
os.Exit(1)
|
||||||
}
|
}
|
||||||
if !isAICore && (*llmBaseURL == "" || *llmAPIKey == "") {
|
if !isAICore && (*llmBaseURL == "" || *llmAPIKey == "") {
|
||||||
@@ -139,7 +152,7 @@ func main() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
// Initialize clients
|
// Initialize clients
|
||||||
giteaClient := gitea.NewClient(*giteaURL, *reviewerToken)
|
giteaClient := gitea.NewClient(*vcsURL, *reviewerToken)
|
||||||
llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel)
|
llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel)
|
||||||
if *llmTemp < 0 || *llmTemp > 2 {
|
if *llmTemp < 0 || *llmTemp > 2 {
|
||||||
slog.Error("invalid LLM temperature", "temperature", *llmTemp, "range", "0-2")
|
slog.Error("invalid LLM temperature", "temperature", *llmTemp, "range", "0-2")
|
||||||
@@ -444,7 +457,7 @@ func main() {
|
|||||||
|
|
||||||
// POST new review
|
// POST new review
|
||||||
slog.Info("posting review", "event", event, "pr", prNumber)
|
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 {
|
if err != nil {
|
||||||
slog.Error("failed to post review", "pr", prNumber, "event", event, "error", err)
|
slog.Error("failed to post review", "pr", prNumber, "event", event, "error", err)
|
||||||
os.Exit(1)
|
os.Exit(1)
|
||||||
@@ -453,7 +466,7 @@ func main() {
|
|||||||
|
|
||||||
// Supersede all old reviews with link to the new one
|
// Supersede all old reviews with link to the new one
|
||||||
if len(oldReviews) > 0 {
|
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 {
|
for _, oldReview := range oldReviews {
|
||||||
cid, err := giteaClient.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID)
|
cid, err := giteaClient.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
|||||||
+6
-2
@@ -262,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.
|
// 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.
|
// 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)
|
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
|
||||||
|
|
||||||
payload := struct {
|
payload := struct {
|
||||||
Body string `json:"body"`
|
Body string `json:"body"`
|
||||||
Event string `json:"event"`
|
Event string `json:"event"`
|
||||||
|
CommitID string `json:"commit_id,omitempty"`
|
||||||
Comments []ReviewComment `json:"comments,omitempty"`
|
Comments []ReviewComment `json:"comments,omitempty"`
|
||||||
}{
|
}{
|
||||||
Body: body,
|
Body: body,
|
||||||
Event: event,
|
Event: event,
|
||||||
|
CommitID: commitID,
|
||||||
Comments: comments,
|
Comments: comments,
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
+29
-5
@@ -119,6 +119,7 @@ func TestPostReview(t *testing.T) {
|
|||||||
var payload struct {
|
var payload struct {
|
||||||
Body string `json:"body"`
|
Body string `json:"body"`
|
||||||
Event string `json:"event"`
|
Event string `json:"event"`
|
||||||
|
CommitID string `json:"commit_id"`
|
||||||
}
|
}
|
||||||
if err := json.NewDecoder(r.Body).Decode(&payload); err != nil {
|
if err := json.NewDecoder(r.Body).Decode(&payload); err != nil {
|
||||||
t.Fatalf("failed to decode payload: %v", err)
|
t.Fatalf("failed to decode payload: %v", err)
|
||||||
@@ -129,14 +130,16 @@ func TestPostReview(t *testing.T) {
|
|||||||
if payload.Event != "APPROVED" {
|
if payload.Event != "APPROVED" {
|
||||||
t.Errorf("expected event %q, got %q", "APPROVED", payload.Event)
|
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.WriteHeader(http.StatusOK)
|
||||||
w.Write([]byte(`{"id":100,"user":{"login":"review-bot"},"state":"APPROVED","stale":false}`))
|
w.Write([]byte(`{"id":100,"user":{"login":"review-bot"},"state":"APPROVED","stale":false}`))
|
||||||
}))
|
}))
|
||||||
defer server.Close()
|
defer server.Close()
|
||||||
|
|
||||||
client := NewClient(server.URL, "test-token")
|
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 {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -183,12 +186,35 @@ func TestPostReview_Non200(t *testing.T) {
|
|||||||
defer server.Close()
|
defer server.Close()
|
||||||
|
|
||||||
client := NewClient(server.URL, "test-token")
|
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 {
|
if err == nil {
|
||||||
t.Fatal("expected error for 403, got 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) {
|
func TestGetFileContent(t *testing.T) {
|
||||||
expected := "# Conventions\n- Be nice\n"
|
expected := "# Conventions\n- Be nice\n"
|
||||||
|
|
||||||
@@ -945,8 +971,6 @@ func TestDoGet_RespectsContextCancellation(t *testing.T) {
|
|||||||
t.Errorf("attempts = %d, expected 1 before context cancel during backoff", attempts)
|
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,
|
// mockTransport is a test helper that returns errors for the first N calls,
|
||||||
// then delegates to a real server.
|
// then delegates to a real server.
|
||||||
type mockTransport struct {
|
type mockTransport struct {
|
||||||
|
|||||||
@@ -37,7 +37,7 @@ func TestPostReview_WithComments(t *testing.T) {
|
|||||||
{Path: "util.go", NewPosition: 10, Body: "[MINOR] Style issue"},
|
{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 {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
@@ -72,7 +72,7 @@ func TestPostReview_NilComments(t *testing.T) {
|
|||||||
defer server.Close()
|
defer server.Close()
|
||||||
|
|
||||||
client := NewClient(server.URL, "test-token")
|
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 {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
}
|
}
|
||||||
|
|||||||
+75
-2
@@ -8,7 +8,10 @@ import (
|
|||||||
"errors"
|
"errors"
|
||||||
"fmt"
|
"fmt"
|
||||||
"io"
|
"io"
|
||||||
|
"log/slog"
|
||||||
"net/http"
|
"net/http"
|
||||||
|
"net/url"
|
||||||
|
"os"
|
||||||
"strconv"
|
"strconv"
|
||||||
"strings"
|
"strings"
|
||||||
"time"
|
"time"
|
||||||
@@ -97,6 +100,10 @@ type Client struct {
|
|||||||
token string
|
token string
|
||||||
httpClient *http.Client
|
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 defines the delays between retry attempts for 429 responses.
|
||||||
// retryBackoff[i] is the delay before attempt i+1 (after attempt i fails).
|
// retryBackoff[i] is the delay before attempt i+1 (after attempt i fails).
|
||||||
// If nil, defaults to {1s, 2s}.
|
// If nil, defaults to {1s, 2s}.
|
||||||
@@ -135,16 +142,53 @@ func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
|
|||||||
return nil
|
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.
|
// NewClient creates a new GitHub API client.
|
||||||
// If baseURL is empty, it defaults to https://api.github.com.
|
// 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).
|
// 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 == "" {
|
if baseURL == "" {
|
||||||
baseURL = defaultBaseURL
|
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{
|
return &Client{
|
||||||
baseURL: strings.TrimRight(baseURL, "/"),
|
baseURL: strings.TrimRight(baseURL, "/"),
|
||||||
token: token,
|
token: token,
|
||||||
|
allowInsecureHTTP: cfg.allowInsecureHTTP,
|
||||||
httpClient: &http.Client{
|
httpClient: &http.Client{
|
||||||
Timeout: 30 * time.Second,
|
Timeout: 30 * time.Second,
|
||||||
CheckRedirect: defaultCheckRedirect,
|
CheckRedirect: defaultCheckRedirect,
|
||||||
@@ -215,10 +259,39 @@ func (c *Client) parseRetryAfter(value string) (time.Duration, bool) {
|
|||||||
return 0, false
|
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.
|
// doRequest performs an HTTP request with retry on 429 rate limit responses.
|
||||||
// It respects the Retry-After header when present, supporting both integer
|
// It respects the Retry-After header when present, supporting both integer
|
||||||
// seconds and HTTP-date formats (capped at maxRetryAfter).
|
// seconds and HTTP-date formats (capped at maxRetryAfter).
|
||||||
func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
|
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
|
var backoff []time.Duration
|
||||||
if c.retryBackoff != nil {
|
if c.retryBackoff != nil {
|
||||||
backoff = append([]time.Duration(nil), c.retryBackoff...)
|
backoff = append([]time.Duration(nil), c.retryBackoff...)
|
||||||
@@ -237,7 +310,7 @@ func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept st
|
|||||||
timer := time.NewTimer(delay)
|
timer := time.NewTimer(delay)
|
||||||
select {
|
select {
|
||||||
case <-timer.C:
|
case <-timer.C:
|
||||||
timer.Stop()
|
timer.Stop() // no-op after fire; kept for symmetry with the ctx.Done case
|
||||||
case <-ctx.Done():
|
case <-ctx.Done():
|
||||||
timer.Stop()
|
timer.Stop()
|
||||||
return nil, ctx.Err()
|
return nil, ctx.Err()
|
||||||
|
|||||||
+154
-9
@@ -35,7 +35,7 @@ func TestDoRequest_Success(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
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")
|
body, err := c.doGet(context.Background(), srv.URL+"/test")
|
||||||
if err != nil {
|
if err != nil {
|
||||||
t.Fatalf("unexpected error: %v", err)
|
t.Fatalf("unexpected error: %v", err)
|
||||||
@@ -60,7 +60,7 @@ func TestDoRequest_429_RetryAfter_IntegerSeconds(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
defer srv.Close()
|
||||||
|
|
||||||
c := NewClient("tok", srv.URL)
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
c.SetRetryBackoff([]time.Duration{0, 0})
|
c.SetRetryBackoff([]time.Duration{0, 0})
|
||||||
|
|
||||||
body, err := c.doGet(context.Background(), srv.URL+"/test")
|
body, err := c.doGet(context.Background(), srv.URL+"/test")
|
||||||
@@ -94,7 +94,7 @@ func TestDoRequest_429_RetryAfter_HTTPDate(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
defer srv.Close()
|
||||||
|
|
||||||
c := NewClient("tok", srv.URL)
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
c.now = func() time.Time { return fixedNow }
|
c.now = func() time.Time { return fixedNow }
|
||||||
// Initial backoff is 0; the HTTP-date parser will compute 1s and override.
|
// Initial backoff is 0; the HTTP-date parser will compute 1s and override.
|
||||||
c.SetRetryBackoff([]time.Duration{0, 0})
|
c.SetRetryBackoff([]time.Duration{0, 0})
|
||||||
@@ -130,7 +130,7 @@ func TestDoRequest_429_RetryAfter_HTTPDate_InPast(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
defer srv.Close()
|
||||||
|
|
||||||
c := NewClient("tok", srv.URL)
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
c.now = func() time.Time { return fixedNow }
|
c.now = func() time.Time { return fixedNow }
|
||||||
c.SetRetryBackoff([]time.Duration{0, 0})
|
c.SetRetryBackoff([]time.Duration{0, 0})
|
||||||
|
|
||||||
@@ -157,7 +157,7 @@ func TestDoRequest_429_NoRetryAfter_UsesDefaultBackoff(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
defer srv.Close()
|
||||||
|
|
||||||
c := NewClient("tok", srv.URL)
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
c.SetRetryBackoff([]time.Duration{0, 0})
|
c.SetRetryBackoff([]time.Duration{0, 0})
|
||||||
|
|
||||||
body, err := c.doGet(context.Background(), srv.URL+"/test")
|
body, err := c.doGet(context.Background(), srv.URL+"/test")
|
||||||
@@ -187,7 +187,7 @@ func TestDoRequest_429_InvalidRetryAfter_UsesDefaultBackoff(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
defer srv.Close()
|
||||||
|
|
||||||
c := NewClient("tok", srv.URL)
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
c.SetRetryBackoff([]time.Duration{0, 0})
|
c.SetRetryBackoff([]time.Duration{0, 0})
|
||||||
|
|
||||||
body, err := c.doGet(context.Background(), srv.URL+"/test")
|
body, err := c.doGet(context.Background(), srv.URL+"/test")
|
||||||
@@ -208,7 +208,7 @@ func TestDoRequest_404_NoRetry(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
defer srv.Close()
|
||||||
|
|
||||||
c := NewClient("tok", srv.URL)
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
_, err := c.doGet(context.Background(), srv.URL+"/test")
|
_, err := c.doGet(context.Background(), srv.URL+"/test")
|
||||||
if err == nil {
|
if err == nil {
|
||||||
t.Fatal("expected error, got nil")
|
t.Fatal("expected error, got nil")
|
||||||
@@ -230,7 +230,7 @@ func TestDoRequest_401_NoRetry(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
defer srv.Close()
|
||||||
|
|
||||||
c := NewClient("tok", srv.URL)
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
_, err := c.doGet(context.Background(), srv.URL+"/test")
|
_, err := c.doGet(context.Background(), srv.URL+"/test")
|
||||||
if err == nil {
|
if err == nil {
|
||||||
t.Fatal("expected error, got nil")
|
t.Fatal("expected error, got nil")
|
||||||
@@ -260,7 +260,7 @@ func TestDoRequest_ContextCanceled(t *testing.T) {
|
|||||||
}))
|
}))
|
||||||
defer srv.Close()
|
defer srv.Close()
|
||||||
|
|
||||||
c := NewClient("tok", srv.URL)
|
c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
|
||||||
c.SetRetryBackoff([]time.Duration{10 * time.Second, 10 * time.Second})
|
c.SetRetryBackoff([]time.Duration{10 * time.Second, 10 * time.Second})
|
||||||
|
|
||||||
ctx, cancel := context.WithCancel(context.Background())
|
ctx, cancel := context.WithCancel(context.Background())
|
||||||
@@ -511,3 +511,148 @@ func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
|
|||||||
t.Fatal("expected CheckRedirect policy after SetHTTPClient(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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|||||||
@@ -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
|
||||||
|
}
|
||||||
|
}
|
||||||
+3
-2
@@ -16,7 +16,8 @@ import (
|
|||||||
|
|
||||||
// Integration test requires a running Gitea instance and LLM endpoint.
|
// Integration test requires a running Gitea instance and LLM endpoint.
|
||||||
// Set environment variables:
|
// 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_TOKEN - Gitea API token with repo access
|
||||||
// INTEGRATION_GITEA_REPO - owner/repo with an open PR
|
// INTEGRATION_GITEA_REPO - owner/repo with an open PR
|
||||||
// INTEGRATION_PR_NUMBER - PR number to test against
|
// INTEGRATION_PR_NUMBER - PR number to test against
|
||||||
@@ -25,7 +26,7 @@ import (
|
|||||||
// INTEGRATION_LLM_MODEL - Model name
|
// INTEGRATION_LLM_MODEL - Model name
|
||||||
|
|
||||||
func TestIntegration_FullReviewFlow(t *testing.T) {
|
func TestIntegration_FullReviewFlow(t *testing.T) {
|
||||||
giteaURL := os.Getenv("INTEGRATION_GITEA_URL")
|
giteaURL := os.Getenv("INTEGRATION_VCS_URL")
|
||||||
giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN")
|
giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN")
|
||||||
giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO")
|
giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO")
|
||||||
prNumStr := os.Getenv("INTEGRATION_PR_NUMBER")
|
prNumStr := os.Getenv("INTEGRATION_PR_NUMBER")
|
||||||
|
|||||||
Reference in New Issue
Block a user