Compare commits
1 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
 Rodin
|
e7bbd55dae |
@@ -34,10 +34,6 @@ inputs:
|
||||
llm-model:
|
||||
description: 'LLM model name'
|
||||
required: true
|
||||
llm-provider:
|
||||
description: 'LLM API provider: openai or anthropic (default openai)'
|
||||
required: false
|
||||
default: 'openai'
|
||||
conventions-file:
|
||||
description: 'Path to conventions file in the repo (e.g. CLAUDE.md)'
|
||||
required: false
|
||||
@@ -66,22 +62,6 @@ inputs:
|
||||
description: 'Print review to stdout instead of posting'
|
||||
required: false
|
||||
default: 'false'
|
||||
update-existing:
|
||||
description: 'Delete previous review from same bot after posting new one. Accepts: true/1/yes or false/0/no (default true)'
|
||||
required: false
|
||||
default: 'true'
|
||||
system-prompt-file:
|
||||
description: 'Local file with additional system prompt instructions (e.g. security review focus)'
|
||||
required: false
|
||||
default: ''
|
||||
persona:
|
||||
description: 'Built-in persona name (security, architect, docs)'
|
||||
required: false
|
||||
default: ''
|
||||
persona-file:
|
||||
description: 'Path to custom persona JSON file'
|
||||
required: false
|
||||
default: ''
|
||||
|
||||
runs:
|
||||
using: 'composite'
|
||||
@@ -160,11 +140,6 @@ runs:
|
||||
PATTERNS_FILES: ${{ inputs.patterns-files }}
|
||||
LLM_TEMPERATURE: ${{ inputs.temperature }}
|
||||
LLM_TIMEOUT: ${{ inputs.timeout }}
|
||||
LLM_PROVIDER: ${{ inputs.llm-provider }}
|
||||
UPDATE_EXISTING: ${{ inputs.update-existing }}
|
||||
SYSTEM_PROMPT_FILE: ${{ inputs.system-prompt-file }}
|
||||
PERSONA: ${{ inputs.persona }}
|
||||
PERSONA_FILE: ${{ inputs.persona-file }}
|
||||
run: |
|
||||
ARGS=""
|
||||
if [ "${{ inputs.dry-run }}" = "true" ]; then
|
||||
|
||||
+3
-17
@@ -19,7 +19,6 @@ jobs:
|
||||
- run: go build -o review-bot ./cmd/review-bot
|
||||
|
||||
# Self-review: builds from source since we're pre-release
|
||||
# Models configured to match SAP AI Core deployments
|
||||
review:
|
||||
runs-on: ubuntu-24.04
|
||||
if: github.event_name == 'pull_request'
|
||||
@@ -29,20 +28,10 @@ jobs:
|
||||
include:
|
||||
- name: sonnet
|
||||
token_secret: SONNET_REVIEW_TOKEN
|
||||
provider: anthropic
|
||||
llm_path: /anthropic/v1
|
||||
model: anthropic--claude-4.6-sonnet
|
||||
model: gpt-5
|
||||
- name: gpt
|
||||
token_secret: GPT_REVIEW_TOKEN
|
||||
provider: openai
|
||||
llm_path: /openai/v1
|
||||
model: gpt-5
|
||||
- name: security
|
||||
token_secret: SECURITY_REVIEW_TOKEN
|
||||
provider: openai
|
||||
llm_path: /openai/v1
|
||||
model: gpt-5
|
||||
system_prompt_file: SECURITY_REVIEW.md
|
||||
model: gpt-5-mini
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
- uses: actions/setup-go@v5
|
||||
@@ -55,14 +44,11 @@ jobs:
|
||||
GITEA_REPO: ${{ github.repository }}
|
||||
PR_NUMBER: ${{ github.event.pull_request.number }}
|
||||
REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }}
|
||||
REVIEWER_NAME: ${{ matrix.name }}
|
||||
LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }}${{ matrix.llm_path }}
|
||||
LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }}
|
||||
LLM_API_KEY: ${{ secrets.LLM_API_KEY }}
|
||||
LLM_MODEL: ${{ matrix.model }}
|
||||
LLM_PROVIDER: ${{ matrix.provider }}
|
||||
CONVENTIONS_FILE: "CONVENTIONS.md"
|
||||
PATTERNS_REPO: "rodin/go-patterns"
|
||||
PATTERNS_FILES: "README.md,patterns/"
|
||||
LLM_TIMEOUT: "600"
|
||||
SYSTEM_PROMPT_FILE: ${{ matrix.system_prompt_file }}
|
||||
run: ./review-bot
|
||||
|
||||
@@ -1,38 +0,0 @@
|
||||
name: PR Ready Gate
|
||||
|
||||
on:
|
||||
pull_request:
|
||||
types: [synchronize]
|
||||
|
||||
jobs:
|
||||
clear-labels:
|
||||
runs-on: ubuntu-24.04
|
||||
# Always run - curl commands are safe if labels don't exist
|
||||
steps:
|
||||
- name: Remove ready and self-reviewed labels, reassign to author
|
||||
env:
|
||||
GITEA_TOKEN: ${{ secrets.RODIN_TOKEN }}
|
||||
run: |
|
||||
PR_NUMBER=${{ github.event.pull_request.number }}
|
||||
AUTHOR=${{ github.event.pull_request.user.login }}
|
||||
READY_LABEL_ID=38
|
||||
SELF_REVIEWED_LABEL_ID=37
|
||||
|
||||
# Remove ready label if present
|
||||
curl -sS -X DELETE \
|
||||
-H "Authorization: token $GITEA_TOKEN" \
|
||||
"https://gitea.weiker.me/api/v1/repos/${{ github.repository }}/issues/${PR_NUMBER}/labels/${READY_LABEL_ID}" || true
|
||||
|
||||
# Remove self-reviewed label if present
|
||||
curl -sS -X DELETE \
|
||||
-H "Authorization: token $GITEA_TOKEN" \
|
||||
"https://gitea.weiker.me/api/v1/repos/${{ github.repository }}/issues/${PR_NUMBER}/labels/${SELF_REVIEWED_LABEL_ID}" || true
|
||||
|
||||
# Reassign to author
|
||||
curl -sS -X PATCH \
|
||||
-H "Authorization: token $GITEA_TOKEN" \
|
||||
-H "Content-Type: application/json" \
|
||||
-d "{\"assignees\": [\"${AUTHOR}\"]}" \
|
||||
"https://gitea.weiker.me/api/v1/repos/${{ github.repository }}/pulls/${PR_NUMBER}"
|
||||
|
||||
echo "Cleared ready/self-reviewed labels and reassigned PR #${PR_NUMBER} to ${AUTHOR}"
|
||||
@@ -69,28 +69,14 @@ jobs:
|
||||
|
||||
echo "Release ID: ${RELEASE_ID}"
|
||||
|
||||
# Upload each asset (idempotent: delete existing asset with same name first)
|
||||
# Upload each asset
|
||||
for file in dist/*; do
|
||||
filename=$(basename "$file")
|
||||
echo "Uploading ${filename}..."
|
||||
|
||||
# Check if asset already exists and delete it
|
||||
EXISTING_ID=$(export ASSET_NAME="${filename}"; curl -sS \
|
||||
-H "Authorization: token ${GITEA_TOKEN}" \
|
||||
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets" \
|
||||
| python3 -c "import json,sys,os; name=os.environ['ASSET_NAME']; assets=json.load(sys.stdin); print(next((str(a['id']) for a in assets if a['name']==name),''))" 2>/dev/null)
|
||||
|
||||
if [ -n "$EXISTING_ID" ]; then
|
||||
echo " Asset ${filename} already exists (id=${EXISTING_ID}), deleting..."
|
||||
curl -sSf -X DELETE \
|
||||
-H "Authorization: token ${GITEA_TOKEN}" \
|
||||
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets/${EXISTING_ID}"
|
||||
fi
|
||||
|
||||
curl -sSf -X POST \
|
||||
-H "Authorization: token ${GITEA_TOKEN}" \
|
||||
-H "Content-Type: application/octet-stream" \
|
||||
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets?name=$(printf '%s' "${filename}" | jq -sRr @uri)" \
|
||||
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets?name=${filename}" \
|
||||
--data-binary "@${file}"
|
||||
done
|
||||
|
||||
|
||||
@@ -1,2 +1 @@
|
||||
/review-bot
|
||||
coverage.out
|
||||
|
||||
@@ -1,20 +0,0 @@
|
||||
.PHONY: build test test-integration lint clean coverage
|
||||
|
||||
build:
|
||||
go build -o review-bot ./cmd/review-bot/
|
||||
|
||||
test:
|
||||
go test ./...
|
||||
|
||||
test-integration:
|
||||
go test -tags integration -v ./cmd/review-bot/
|
||||
|
||||
lint:
|
||||
go vet ./...
|
||||
|
||||
clean:
|
||||
rm -f review-bot
|
||||
|
||||
coverage:
|
||||
go test -coverprofile=coverage.out ./...
|
||||
go tool cover -func=coverage.out
|
||||
@@ -1,256 +1,17 @@
|
||||
# review-bot
|
||||
|
||||
AI-powered code review bot for Gitea pull requests. Fetches diff + context, sends to an LLM, and posts a structured review (APPROVE / REQUEST_CHANGES) back to the PR.
|
||||
Automated code review bot for Gitea. Fetches a pull request diff, sends it to an LLM for analysis, and posts a structured review back to the PR.
|
||||
|
||||
## Features
|
||||
|
||||
- **Multi-provider**: OpenAI-compatible and Anthropic Messages API
|
||||
- **Context-aware**: Fetches full file content, conventions, language patterns, CI status
|
||||
- **Smart budget**: Automatically trims context to fit model token limits
|
||||
- **Idempotent reviews**: Posts new review, then cleans up stale ones (one review per bot)
|
||||
- **Custom prompts**: Load additional instructions from a file (e.g. security-focused review)
|
||||
- **Zero dependencies**: Go stdlib only
|
||||
- Fetches PR metadata, diff, and CI status from Gitea API
|
||||
- Sends context-rich prompts to any OpenAI-compatible LLM
|
||||
- Parses structured JSON review responses
|
||||
- Posts formatted reviews (APPROVE / REQUEST_CHANGES) back to Gitea
|
||||
- Supports custom coding conventions via repo files
|
||||
- Zero external dependencies — Go stdlib only
|
||||
|
||||
## Quick Start: Composite Action
|
||||
|
||||
The easiest way to use review-bot in your Gitea CI:
|
||||
|
||||
```yaml
|
||||
# .gitea/workflows/review.yml
|
||||
name: Review
|
||||
on:
|
||||
pull_request:
|
||||
types: [opened, synchronize]
|
||||
|
||||
jobs:
|
||||
review:
|
||||
runs-on: ubuntu-24.04
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
- uses: https://gitea.weiker.me/rodin/review-bot/.gitea/actions/review@v0.1.0
|
||||
with:
|
||||
reviewer-token: ${{ secrets.REVIEW_TOKEN }}
|
||||
reviewer-name: code-review
|
||||
llm-base-url: ${{ secrets.LLM_BASE_URL }}
|
||||
llm-api-key: ${{ secrets.LLM_API_KEY }}
|
||||
llm-model: gpt-4.1
|
||||
```
|
||||
|
||||
That's it. Every PR gets an automated review.
|
||||
|
||||
## Examples
|
||||
|
||||
### Single reviewer with conventions
|
||||
|
||||
```yaml
|
||||
jobs:
|
||||
review:
|
||||
runs-on: ubuntu-24.04
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
- uses: https://gitea.weiker.me/rodin/review-bot/.gitea/actions/review@v0.1.0
|
||||
with:
|
||||
reviewer-token: ${{ secrets.REVIEW_TOKEN }}
|
||||
reviewer-name: reviewer
|
||||
llm-base-url: ${{ secrets.LLM_BASE_URL }}
|
||||
llm-api-key: ${{ secrets.LLM_API_KEY }}
|
||||
llm-model: gpt-4.1
|
||||
conventions-file: CONVENTIONS.md
|
||||
timeout: '600'
|
||||
```
|
||||
|
||||
### Two reviewers with different models (diversity of opinion)
|
||||
|
||||
```yaml
|
||||
jobs:
|
||||
review:
|
||||
runs-on: ubuntu-24.04
|
||||
strategy:
|
||||
matrix:
|
||||
include:
|
||||
- name: gpt
|
||||
model: gpt-4.1
|
||||
token_secret: GPT_REVIEW_TOKEN
|
||||
- name: claude
|
||||
model: claude-sonnet-4-20250514
|
||||
token_secret: CLAUDE_REVIEW_TOKEN
|
||||
provider: anthropic
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
- uses: https://gitea.weiker.me/rodin/review-bot/.gitea/actions/review@v0.1.0
|
||||
with:
|
||||
reviewer-token: ${{ secrets[matrix.token_secret] }}
|
||||
reviewer-name: ${{ matrix.name }}
|
||||
llm-base-url: ${{ secrets.LLM_BASE_URL }}
|
||||
llm-api-key: ${{ secrets.LLM_API_KEY }}
|
||||
llm-model: ${{ matrix.model }}
|
||||
llm-provider: ${{ matrix.provider }}
|
||||
conventions-file: CONVENTIONS.md
|
||||
```
|
||||
|
||||
Each reviewer posts independently and only cleans up its own stale reviews.
|
||||
|
||||
### Multiple review types from a single bot account
|
||||
|
||||
Use the same Gitea token but different `reviewer-name` values to run specialized reviews without needing multiple bot accounts:
|
||||
|
||||
```yaml
|
||||
jobs:
|
||||
review:
|
||||
runs-on: ubuntu-24.04
|
||||
strategy:
|
||||
matrix:
|
||||
include:
|
||||
- name: code-quality
|
||||
model: gpt-4.1
|
||||
- name: security
|
||||
model: gpt-4.1
|
||||
system_prompt_file: .review/SECURITY.md
|
||||
- name: performance
|
||||
model: gpt-4.1
|
||||
system_prompt_file: .review/PERFORMANCE.md
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
- uses: https://gitea.weiker.me/rodin/review-bot/.gitea/actions/review@v0.1.0
|
||||
with:
|
||||
reviewer-token: ${{ secrets.REVIEW_TOKEN }}
|
||||
reviewer-name: ${{ matrix.name }}
|
||||
llm-base-url: ${{ secrets.LLM_BASE_URL }}
|
||||
llm-api-key: ${{ secrets.LLM_API_KEY }}
|
||||
llm-model: ${{ matrix.model }}
|
||||
system-prompt-file: ${{ matrix.system_prompt_file }}
|
||||
```
|
||||
|
||||
The sentinel `<!-- review-bot:security -->` ensures the security review only replaces previous security reviews, never the code-quality or performance reviews.
|
||||
|
||||
### With language patterns from another repo
|
||||
|
||||
```yaml
|
||||
- uses: https://gitea.weiker.me/rodin/review-bot/.gitea/actions/review@v0.1.0
|
||||
with:
|
||||
reviewer-token: ${{ secrets.REVIEW_TOKEN }}
|
||||
reviewer-name: reviewer
|
||||
llm-base-url: ${{ secrets.LLM_BASE_URL }}
|
||||
llm-api-key: ${{ secrets.LLM_API_KEY }}
|
||||
llm-model: gpt-4.1
|
||||
conventions-file: CLAUDE.md
|
||||
patterns-repo: rodin/go-patterns,rodin/kubernetes-conventions
|
||||
patterns-files: "README.md,patterns/"
|
||||
```
|
||||
|
||||
Pattern repos are fetched at review time. The reviewer uses them as criteria for idiomatic code.
|
||||
|
||||
### Dry run (test without posting)
|
||||
|
||||
```yaml
|
||||
- uses: https://gitea.weiker.me/rodin/review-bot/.gitea/actions/review@v0.1.0
|
||||
with:
|
||||
reviewer-token: ${{ secrets.REVIEW_TOKEN }}
|
||||
reviewer-name: test
|
||||
llm-base-url: ${{ secrets.LLM_BASE_URL }}
|
||||
llm-api-key: ${{ secrets.LLM_API_KEY }}
|
||||
llm-model: gpt-4.1
|
||||
dry-run: 'true'
|
||||
```
|
||||
|
||||
Prints the review to CI logs without posting to the PR. Useful for testing prompt changes.
|
||||
|
||||
### Using Anthropic directly
|
||||
|
||||
```yaml
|
||||
- uses: https://gitea.weiker.me/rodin/review-bot/.gitea/actions/review@v0.1.0
|
||||
with:
|
||||
reviewer-token: ${{ secrets.REVIEW_TOKEN }}
|
||||
reviewer-name: claude
|
||||
llm-base-url: https://api.anthropic.com
|
||||
llm-api-key: ${{ secrets.ANTHROPIC_API_KEY }}
|
||||
llm-model: claude-sonnet-4-20250514
|
||||
llm-provider: anthropic
|
||||
```
|
||||
|
||||
## Action Inputs
|
||||
|
||||
| Input | Required | Default | Description |
|
||||
|-------|----------|---------|-------------|
|
||||
| `reviewer-token` | Yes | — | Gitea token for posting reviews (needs `write:issue`, `write:repository`) |
|
||||
| `reviewer-name` | No | `""` | Logical identity for this reviewer. Used as sentinel for idempotent cleanup. Set this when running multiple review bots on the same PR. |
|
||||
| `llm-base-url` | Yes | — | LLM API base URL |
|
||||
| `llm-api-key` | Yes | — | LLM API key |
|
||||
| `llm-model` | Yes | — | Model name |
|
||||
| `llm-provider` | No | `openai` | API provider: `openai` or `anthropic` |
|
||||
| `conventions-file` | No | `""` | Path to coding conventions file in the repo |
|
||||
| `patterns-repo` | No | `""` | Comma-separated repos with language patterns (e.g. `rodin/go-patterns`) |
|
||||
| `patterns-files` | No | `README.md` | Files/directories to fetch from pattern repos |
|
||||
| `system-prompt-file` | No | `""` | Local file with additional system prompt instructions |
|
||||
| `persona` | No | `""` | Built-in persona name (security, architect, docs) |
|
||||
| `persona-file` | No | `""` | Path to persona JSON file with custom review focus |
|
||||
| `temperature` | No | `0` | LLM temperature (0 = server default) |
|
||||
| `timeout` | No | `300` | LLM request timeout in seconds |
|
||||
| `dry-run` | No | `false` | Print review to stdout instead of posting |
|
||||
| `update-existing` | No | `true` | Delete previous review from same bot before posting. Accepts: true/1/yes or false/0/no |
|
||||
| `version` | No | `latest` | review-bot version to install |
|
||||
|
||||
## Runner Requirements
|
||||
|
||||
The composite action requires these tools on the runner:
|
||||
|
||||
| Tool | Used For |
|
||||
|------|----------|
|
||||
| `python3` | JSON parsing during version detection |
|
||||
| `sha256sum` | Checksum verification of downloaded binary |
|
||||
| `curl` | Downloading releases and querying the API |
|
||||
|
||||
All three are pre-installed on `ubuntu-*` runners (e.g. `ubuntu-24.04`). If you use a custom runner image, ensure these are available.
|
||||
|
||||
## How Review Cleanup Works
|
||||
|
||||
When `reviewer-name` is set, the bot embeds a hidden sentinel in each review:
|
||||
|
||||
```html
|
||||
<!-- review-bot:code-review -->
|
||||
```
|
||||
|
||||
On the next run, it finds and deletes any review containing its own sentinel (except the one it just posted). This means:
|
||||
|
||||
- **One review per bot per PR** — no clutter from repeated pushes
|
||||
- **Multiple bots coexist** — each only cleans up its own reviews
|
||||
- **Same token, different roles** — a single bot account can post "code-review" and "security" reviews without conflict
|
||||
- **No extra permissions** — identity comes from the sentinel, not the API
|
||||
|
||||
If `reviewer-name` is empty, cleanup is skipped (reviews stack like before).
|
||||
|
||||
### Shared Token: Worst-Wins Behavior
|
||||
|
||||
When multiple review types share the same Gitea bot account (e.g. code-quality and security), Gitea determines the user's approval state from their **most recent review**. This creates a race condition: if security finds issues (REQUEST_CHANGES) but code-quality finishes last (APPROVE), the PR appears approved.
|
||||
|
||||
review-bot handles this automatically with **worst-wins reconciliation**: before posting, each job checks whether any sibling review from the same user already has REQUEST_CHANGES. If so and this job would post APPROVE, it posts as REQUEST_CHANGES instead — maintaining the block. This ensures the PR stays blocked until all checks pass, regardless of execution order.
|
||||
|
||||
**If you need independent approval/block per review type**, use separate Gitea bot accounts with their own tokens.
|
||||
|
||||
## Custom Review Prompts
|
||||
|
||||
Use `system-prompt-file` to specialize the review focus. The file contents are appended to the base system prompt as "Additional Review Instructions."
|
||||
|
||||
Example `SECURITY_REVIEW.md`:
|
||||
|
||||
```markdown
|
||||
You are performing a security-focused code review.
|
||||
|
||||
Focus areas:
|
||||
- Injection attacks (SQL, command, path traversal, template)
|
||||
- Authentication/Authorization (missing checks, privilege escalation)
|
||||
- Secrets exposure (hardcoded credentials, tokens in logs)
|
||||
- Input validation (unsanitized input, unsafe deserialization)
|
||||
- Race conditions (TOCTOU, unsynchronized shared state)
|
||||
|
||||
Rules:
|
||||
- Only report findings with security implications
|
||||
- Ignore style, naming, and general code quality
|
||||
- MAJOR = exploitable vulnerability, MINOR = hardening opportunity, NIT = theoretical risk
|
||||
- If no security-relevant changes exist, APPROVE with empty findings
|
||||
```
|
||||
|
||||
## CLI Usage
|
||||
## Usage
|
||||
|
||||
```bash
|
||||
review-bot \
|
||||
@@ -258,176 +19,73 @@ review-bot \
|
||||
--repo owner/name \
|
||||
--pr 42 \
|
||||
--reviewer-token "$GITEA_TOKEN" \
|
||||
--reviewer-name "code-review" \
|
||||
--llm-base-url https://api.openai.com/v1 \
|
||||
--llm-api-key "$OPENAI_API_KEY" \
|
||||
--llm-model gpt-4.1 \
|
||||
--conventions-file CONVENTIONS.md
|
||||
--llm-model gpt-4 \
|
||||
--reviewer-name "Sonnet" \
|
||||
--conventions-file CONVENTIONS.md \
|
||||
--dry-run
|
||||
```
|
||||
|
||||
## Environment Variables
|
||||
|
||||
All flags have environment variable equivalents:
|
||||
All flags can be set via environment variables:
|
||||
|
||||
| Flag | Env Var |
|
||||
|------|---------|
|
||||
| `--gitea-url` | `GITEA_URL` |
|
||||
| `--repo` | `GITEA_REPO` |
|
||||
| `--pr` | `PR_NUMBER` |
|
||||
| `--reviewer-token` | `REVIEWER_TOKEN` |
|
||||
| `--reviewer-name` | `REVIEWER_NAME` |
|
||||
| `--llm-base-url` | `LLM_BASE_URL` |
|
||||
| `--llm-api-key` | `LLM_API_KEY` |
|
||||
| `--llm-model` | `LLM_MODEL` |
|
||||
| `--llm-provider` | `LLM_PROVIDER` |
|
||||
| `--conventions-file` | `CONVENTIONS_FILE` |
|
||||
| `--patterns-repo` | `PATTERNS_REPO` |
|
||||
| `--patterns-files` | `PATTERNS_FILES` |
|
||||
| `--system-prompt-file` | `SYSTEM_PROMPT_FILE` |
|
||||
| `--llm-temperature` | `LLM_TEMPERATURE` |
|
||||
| `--llm-timeout` | `LLM_TIMEOUT` |
|
||||
| `--update-existing` | `UPDATE_EXISTING` |
|
||||
| Flag | Env Var | Required | Description |
|
||||
|------|---------|----------|-------------|
|
||||
| `--gitea-url` | `GITEA_URL` | Yes | Gitea instance base URL |
|
||||
| `--repo` | `GITEA_REPO` | Yes | Repository in `owner/name` format |
|
||||
| `--pr` | `PR_NUMBER` | Yes | Pull request number |
|
||||
| `--reviewer-token` | `REVIEWER_TOKEN` | Yes | Gitea API token for posting reviews |
|
||||
| `--llm-base-url` | `LLM_BASE_URL` | Yes | OpenAI-compatible API base URL |
|
||||
| `--llm-api-key` | `LLM_API_KEY` | Yes | LLM API key |
|
||||
| `--llm-model` | `LLM_MODEL` | Yes | Model identifier |
|
||||
| `--reviewer-name` | `REVIEWER_NAME` | No | Display name in review footer |
|
||||
| `--conventions-file` | `CONVENTIONS_FILE` | No | Path to conventions file in repo |
|
||||
| `--dry-run` | — | No | Print review to stdout instead of posting |
|
||||
|
||||
## Setup
|
||||
## Adding to a Gitea Repository
|
||||
|
||||
1. **Create a Gitea bot account** (e.g. `review-bot`)
|
||||
2. **Generate a token** with scopes: `write:issue`, `write:repository`
|
||||
3. **Add secrets** to your Gitea repo (Settings → Actions → Secrets):
|
||||
- `REVIEW_TOKEN` — the bot's Gitea token
|
||||
- `LLM_BASE_URL` — your LLM endpoint
|
||||
- `LLM_API_KEY` — your LLM key
|
||||
4. **Add the workflow** (see Quick Start above)
|
||||
1. Build the binary or use the CI workflow approach (build in CI).
|
||||
|
||||
### Token Scopes Required
|
||||
2. Add secrets to your Gitea repo (Settings → Actions → Secrets):
|
||||
- `SONNET_REVIEW_TOKEN` — Gitea token for the Sonnet reviewer account
|
||||
- `GPT_REVIEW_TOKEN` — Gitea token for the GPT reviewer account
|
||||
- `LLM_BASE_URL` — Your LLM API endpoint
|
||||
- `LLM_API_KEY` — Your LLM API key
|
||||
|
||||
| Scope | Purpose |
|
||||
|-------|---------|
|
||||
| `write:issue` | Post and delete reviews |
|
||||
| `write:repository` | Read PR diffs, file content, commit statuses |
|
||||
3. Copy `.gitea/workflows/ci.yml` to your repo (or adapt it).
|
||||
|
||||
No `read:user` scope needed — the bot identifies itself from the review response.
|
||||
4. On every PR, the bot will:
|
||||
- Run tests and vet
|
||||
- Build review-bot
|
||||
- Post reviews from each configured LLM reviewer
|
||||
|
||||
## Development
|
||||
|
||||
```bash
|
||||
go test ./... # Unit tests
|
||||
go vet ./... # Static analysis
|
||||
# Run tests
|
||||
go test ./...
|
||||
|
||||
# Run vet
|
||||
go vet ./...
|
||||
|
||||
# Build
|
||||
go build -o review-bot ./cmd/review-bot
|
||||
|
||||
# Integration tests (requires env vars set)
|
||||
# Integration tests (requires env vars)
|
||||
go test -tags=integration ./...
|
||||
```
|
||||
|
||||
## Architecture
|
||||
|
||||
```
|
||||
cmd/review-bot/ CLI entrypoint + orchestration
|
||||
gitea/ Gitea API client (reviews, PRs, files)
|
||||
llm/ Multi-provider LLM client (OpenAI + Anthropic)
|
||||
cmd/review-bot/ CLI entrypoint
|
||||
gitea/ Gitea API client
|
||||
llm/ OpenAI-compatible LLM client
|
||||
review/ Prompt building, response parsing, formatting
|
||||
budget/ Token estimation + context trimming
|
||||
```
|
||||
|
||||
## License
|
||||
|
||||
MIT
|
||||
|
||||
## Review Personas
|
||||
|
||||
Personas provide role-based review specialization. Instead of generic code review, each persona focuses on a specific domain (security, architecture, documentation) with tailored prompts and severity calibration.
|
||||
|
||||
### Built-in Personas
|
||||
|
||||
| Persona | Focus |
|
||||
|---------|-------|
|
||||
| `security` | Vulnerabilities, auth bypass, secrets exposure, injection attacks |
|
||||
| `architect` | Design patterns, code organization, API contracts, testability |
|
||||
| `docs` | Documentation quality, API clarity, error messages |
|
||||
|
||||
### Using Built-in Personas
|
||||
|
||||
```yaml
|
||||
- uses: rodin/review-bot/.gitea/actions/review@v1
|
||||
with:
|
||||
reviewer-name: security
|
||||
persona: security
|
||||
llm-model: claude-opus-4-20250514 # Security benefits from strong reasoning
|
||||
...
|
||||
```
|
||||
|
||||
### Multiple Personas in Parallel
|
||||
|
||||
```yaml
|
||||
jobs:
|
||||
review:
|
||||
strategy:
|
||||
matrix:
|
||||
include:
|
||||
- name: security
|
||||
persona: security
|
||||
- name: architect
|
||||
persona: architect
|
||||
steps:
|
||||
- uses: rodin/review-bot/.gitea/actions/review@v1
|
||||
with:
|
||||
reviewer-name: ${{ matrix.name }}
|
||||
persona: ${{ matrix.persona }}
|
||||
...
|
||||
```
|
||||
|
||||
Each persona posts independently with its own sentinel, so reviews don't interfere.
|
||||
|
||||
|
||||
### Custom Personas
|
||||
|
||||
Create a JSON file with your domain-specific review focus:
|
||||
|
||||
```json
|
||||
// .review/personas/trading.json
|
||||
{
|
||||
"name": "trading",
|
||||
"display_name": "Trading Domain Expert",
|
||||
"identity": "You are a trading systems expert reviewing code for correctness.\n\nYour expertise:\n- Order lifecycle and state machines\n- Fill handling and partial fills\n- Position tracking and P&L calculations\n- Event sourcing invariants",
|
||||
"focus": [
|
||||
"Order state machine correctness",
|
||||
"Fill handling edge cases (partial, overfill)",
|
||||
"Position and P&L calculation accuracy",
|
||||
"Event replay determinism",
|
||||
"Decimal precision for money"
|
||||
],
|
||||
"ignore": [
|
||||
"Code style",
|
||||
"General performance",
|
||||
"Documentation formatting"
|
||||
],
|
||||
"severity": {
|
||||
"major": "Bugs that cause incorrect positions, fills, or money calculations",
|
||||
"minor": "Edge cases that could cause issues under unusual conditions",
|
||||
"nit": "Clarity improvements for domain logic"
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Use it in CI:
|
||||
|
||||
```yaml
|
||||
- uses: rodin/review-bot/.gitea/actions/review@v1
|
||||
with:
|
||||
reviewer-name: trading
|
||||
persona-file: .review/personas/trading.json
|
||||
...
|
||||
```
|
||||
|
||||
|
||||
### Persona vs system-prompt-file
|
||||
|
||||
| Feature | `persona` / `persona-file` | `system-prompt-file` |
|
||||
|---------|---------------------------|----------------------|
|
||||
| Replaces base prompt | Yes | No (appends) |
|
||||
| Structured format | Yes (JSON) | No (freeform) |
|
||||
| Focus/ignore lists | Yes | Manual |
|
||||
| Severity calibration | Yes | Manual |
|
||||
| Header display name | Yes | No |
|
||||
| Built-in options | Yes | No |
|
||||
|
||||
Use personas for domain-specialized reviews. Use `system-prompt-file` for minor tweaks to the generic review.
|
||||
|
||||
@@ -1,18 +0,0 @@
|
||||
You are performing a security-focused code review. Your primary concern is identifying vulnerabilities, not general code quality.
|
||||
|
||||
Focus areas:
|
||||
- **Injection attacks**: SQL injection, command injection, path traversal, template injection
|
||||
- **Authentication/Authorization**: Missing auth checks, privilege escalation, IDOR
|
||||
- **Secrets exposure**: Hardcoded credentials, API keys in code, tokens in logs
|
||||
- **Input validation**: Untrusted input used without sanitization, unsafe deserialization
|
||||
- **Cryptography**: Weak algorithms, predictable randomness, improper key management
|
||||
- **Error handling**: Information leakage in error messages, stack traces exposed
|
||||
- **Dependencies**: Known vulnerable patterns, unsafe use of external libraries
|
||||
- **Race conditions**: TOCTOU bugs, unsynchronized shared state
|
||||
- **Resource exhaustion**: Unbounded allocations, missing timeouts, denial-of-service vectors
|
||||
|
||||
Rules for this review:
|
||||
- Only report findings with actual security implications. Ignore style, naming, and general code quality.
|
||||
- Severity mapping: MAJOR = exploitable vulnerability or data exposure. MINOR = defense-in-depth improvement or hardening opportunity. NIT = theoretical concern with low practical risk.
|
||||
- If the code has no security-relevant changes, APPROVE with an empty findings list.
|
||||
- Do not duplicate findings that a standard code review would catch (logic bugs, missing error checks) unless they have a security dimension.
|
||||
+14
-14
@@ -8,7 +8,6 @@ package budget
|
||||
import (
|
||||
"fmt"
|
||||
"strings"
|
||||
"unicode/utf8"
|
||||
)
|
||||
|
||||
// modelLimit pairs a model name prefix with its context window size.
|
||||
@@ -39,7 +38,7 @@ const diffTooLargeMarker = "... [diff too large for context window — review ma
|
||||
const userMetaTruncMarker = "\n... [description truncated] ..."
|
||||
|
||||
// EstimateTokens estimates the number of tokens in a string.
|
||||
// Uses the rough heuristic of ~4 bytes per token, which is
|
||||
// Uses the rough heuristic of ~4 characters per token, which is
|
||||
// conservative for English text and code.
|
||||
func EstimateTokens(s string) int {
|
||||
return len(s) / 4
|
||||
@@ -65,7 +64,7 @@ type Sections struct {
|
||||
Conventions string // Repo conventions (trimmed second)
|
||||
FileContext string // Full file content (trimmed third)
|
||||
Diff string // The actual diff (trimmed last, only truncated)
|
||||
UserMeta string // PR title, description, CI status (truncated only if base exceeds budget)
|
||||
UserMeta string // PR title, description, CI status (never trimmed)
|
||||
}
|
||||
|
||||
// Result holds the trimmed content and metadata about what was dropped.
|
||||
@@ -154,11 +153,7 @@ func Fit(model string, sections Sections) Result {
|
||||
removed := EstimateTokens(sections.Diff) - diffBudget
|
||||
trimmed = append(trimmed, fmt.Sprintf("diff truncated (~%dK tokens removed)", removed/1000))
|
||||
if maxChars > 0 {
|
||||
if diffBudget >= markerBudget {
|
||||
sections.Diff = truncateUTF8(sections.Diff, maxChars) + diffTruncMarker
|
||||
} else {
|
||||
sections.Diff = truncateUTF8(sections.Diff, maxChars)
|
||||
}
|
||||
sections.Diff = truncateUTF8(sections.Diff, maxChars) + diffTruncMarker
|
||||
} else {
|
||||
sections.Diff = diffTooLargeMarker
|
||||
}
|
||||
@@ -193,11 +188,9 @@ func buildResult(s Sections, trimmed []string, estTokens int) Result {
|
||||
usr.WriteString(s.FileContext)
|
||||
usr.WriteString("\n")
|
||||
}
|
||||
if s.Diff != "" {
|
||||
usr.WriteString("\n### Diff (changes to review)\n\n```diff\n")
|
||||
usr.WriteString(s.Diff)
|
||||
usr.WriteString("\n```\n")
|
||||
}
|
||||
usr.WriteString("\n### Diff (changes to review)\n\n```diff\n")
|
||||
usr.WriteString(s.Diff)
|
||||
usr.WriteString("\n```\n")
|
||||
|
||||
if len(trimmed) > 0 {
|
||||
usr.WriteString("\n⚠️ Note: Context was trimmed to fit model limits. Dropped: ")
|
||||
@@ -219,8 +212,15 @@ func truncateUTF8(s string, maxBytes int) string {
|
||||
if len(s) <= maxBytes {
|
||||
return s
|
||||
}
|
||||
for maxBytes > 0 && !utf8.RuneStart(s[maxBytes]) {
|
||||
// Walk backwards from maxBytes to find a valid UTF-8 boundary
|
||||
for maxBytes > 0 && !isUTF8Start(s[maxBytes]) {
|
||||
maxBytes--
|
||||
}
|
||||
return s[:maxBytes]
|
||||
}
|
||||
|
||||
// isUTF8Start returns true if b is a valid start byte for a UTF-8 sequence
|
||||
// (single-byte ASCII or multi-byte lead byte, not a continuation byte).
|
||||
func isUTF8Start(b byte) bool {
|
||||
return b&0xC0 != 0x80
|
||||
}
|
||||
|
||||
@@ -1,161 +0,0 @@
|
||||
//go:build integration
|
||||
|
||||
package main
|
||||
|
||||
import (
|
||||
"context"
|
||||
"os"
|
||||
"strconv"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"gitea.weiker.me/rodin/review-bot/gitea"
|
||||
"gitea.weiker.me/rodin/review-bot/llm"
|
||||
"gitea.weiker.me/rodin/review-bot/review"
|
||||
)
|
||||
|
||||
// Integration test requires a running Gitea instance and LLM endpoint.
|
||||
// Set environment variables:
|
||||
//
|
||||
// INTEGRATION_GITEA_URL - Gitea 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
|
||||
// INTEGRATION_LLM_BASE_URL - LLM API base URL
|
||||
// INTEGRATION_LLM_API_KEY - LLM API key
|
||||
// INTEGRATION_LLM_MODEL - Model name
|
||||
func TestIntegration_FullReviewFlow(t *testing.T) {
|
||||
giteaURL := os.Getenv("INTEGRATION_GITEA_URL")
|
||||
giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN")
|
||||
giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO")
|
||||
prNumStr := os.Getenv("INTEGRATION_PR_NUMBER")
|
||||
llmBaseURL := os.Getenv("INTEGRATION_LLM_BASE_URL")
|
||||
llmAPIKey := os.Getenv("INTEGRATION_LLM_API_KEY")
|
||||
llmModel := os.Getenv("INTEGRATION_LLM_MODEL")
|
||||
|
||||
if giteaURL == "" || giteaToken == "" || giteaRepo == "" || prNumStr == "" ||
|
||||
llmBaseURL == "" || llmAPIKey == "" || llmModel == "" {
|
||||
t.Skip("Integration test env vars not set, skipping")
|
||||
}
|
||||
|
||||
prNumber, err := strconv.Atoi(prNumStr)
|
||||
if err != nil {
|
||||
t.Fatalf("Invalid PR number %q: %v", prNumStr, err)
|
||||
}
|
||||
|
||||
// Parse owner/repo
|
||||
parts := strings.SplitN(giteaRepo, "/", 2)
|
||||
if len(parts) != 2 {
|
||||
t.Fatalf("Invalid repo format %q", giteaRepo)
|
||||
}
|
||||
owner, repoName := parts[0], parts[1]
|
||||
if owner == "" || repoName == "" {
|
||||
t.Fatalf("Invalid repo format %q", giteaRepo)
|
||||
}
|
||||
|
||||
ctx := context.Background()
|
||||
|
||||
// Step 1: Fetch PR
|
||||
giteaClient := gitea.NewClient(giteaURL, giteaToken)
|
||||
pr, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber)
|
||||
if err != nil {
|
||||
t.Fatalf("GetPullRequest: %v", err)
|
||||
}
|
||||
t.Logf("PR: %s (sha: %s)", pr.Title, pr.Head.Sha)
|
||||
|
||||
// Step 2: Fetch diff
|
||||
diff, err := giteaClient.GetPullRequestDiff(ctx, owner, repoName, prNumber)
|
||||
if err != nil {
|
||||
t.Fatalf("GetPullRequestDiff: %v", err)
|
||||
}
|
||||
if diff == "" {
|
||||
t.Fatal("diff is empty")
|
||||
}
|
||||
t.Logf("Diff size: %d bytes", len(diff))
|
||||
|
||||
// Step 3: Build prompts
|
||||
systemPrompt := review.BuildSystemPrompt("", "")
|
||||
userPrompt := review.BuildUserPrompt(pr.Title, pr.Body, diff, "", true, "")
|
||||
|
||||
// Step 4: Call LLM
|
||||
llmClient := llm.NewClient(llmBaseURL, llmAPIKey, llmModel)
|
||||
response, err := llmClient.Complete(ctx, []llm.Message{
|
||||
{Role: "system", Content: systemPrompt},
|
||||
{Role: "user", Content: userPrompt},
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("LLM Complete: %v", err)
|
||||
}
|
||||
t.Logf("LLM response: %d bytes", len(response))
|
||||
|
||||
// Step 5: Parse response
|
||||
result, err := review.ParseResponse(response)
|
||||
if err != nil {
|
||||
t.Fatalf("ParseResponse: %v", err)
|
||||
}
|
||||
t.Logf("Verdict: %s, Findings: %d", result.Verdict, len(result.Findings))
|
||||
|
||||
// Step 6: Format (dry-run validation)
|
||||
body := review.FormatMarkdown(result, "integration-test")
|
||||
if body == "" {
|
||||
t.Fatal("formatted review body is empty")
|
||||
}
|
||||
t.Logf("Review body:\n%s", body)
|
||||
}
|
||||
|
||||
func TestIntegration_PostAndCleanup(t *testing.T) {
|
||||
giteaURL := os.Getenv("INTEGRATION_GITEA_URL")
|
||||
giteaToken := os.Getenv("INTEGRATION_GITEA_TOKEN")
|
||||
giteaRepo := os.Getenv("INTEGRATION_GITEA_REPO")
|
||||
prNumStr := os.Getenv("INTEGRATION_PR_NUMBER")
|
||||
|
||||
if giteaURL == "" || giteaToken == "" || giteaRepo == "" || prNumStr == "" {
|
||||
t.Skip("Integration test env vars not set, skipping")
|
||||
}
|
||||
|
||||
prNumber, err := strconv.Atoi(prNumStr)
|
||||
if err != nil {
|
||||
t.Fatalf("Invalid PR number %q: %v", prNumStr, err)
|
||||
}
|
||||
|
||||
parts := strings.SplitN(giteaRepo, "/", 2)
|
||||
if len(parts) != 2 {
|
||||
t.Fatalf("Invalid repo format %q", giteaRepo)
|
||||
}
|
||||
owner, repoName := parts[0], parts[1]
|
||||
|
||||
ctx := context.Background()
|
||||
giteaClient := gitea.NewClient(giteaURL, giteaToken)
|
||||
|
||||
// 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)
|
||||
if err != nil {
|
||||
t.Fatalf("PostReview: %v", err)
|
||||
}
|
||||
t.Logf("Posted review ID: %d", posted.ID)
|
||||
|
||||
// Verify it appears in listing
|
||||
reviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
|
||||
if err != nil {
|
||||
t.Fatalf("ListReviews: %v", err)
|
||||
}
|
||||
|
||||
found := false
|
||||
for _, r := range reviews {
|
||||
if r.ID == posted.ID && strings.Contains(r.Body, sentinel) {
|
||||
found = true
|
||||
break
|
||||
}
|
||||
}
|
||||
if !found {
|
||||
t.Error("posted review not found in listing")
|
||||
}
|
||||
|
||||
// Cleanup: delete the test review
|
||||
err = giteaClient.DeleteReview(ctx, owner, repoName, prNumber, posted.ID)
|
||||
if err != nil {
|
||||
t.Logf("Warning: could not delete test review %d: %v", posted.ID, err)
|
||||
}
|
||||
}
|
||||
+44
-475
@@ -4,9 +4,8 @@ import (
|
||||
"context"
|
||||
"flag"
|
||||
"fmt"
|
||||
"log/slog"
|
||||
"log"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strconv"
|
||||
"strings"
|
||||
"time"
|
||||
@@ -19,40 +18,8 @@ import (
|
||||
|
||||
var version = "dev"
|
||||
|
||||
// setupLogger configures the global slog default logger based on format and verbosity.
|
||||
func setupLogger(format, verbosity string) {
|
||||
var level slog.Level
|
||||
switch strings.ToLower(verbosity) {
|
||||
case "debug":
|
||||
level = slog.LevelDebug
|
||||
case "info":
|
||||
level = slog.LevelInfo
|
||||
case "warn":
|
||||
level = slog.LevelWarn
|
||||
case "error":
|
||||
level = slog.LevelError
|
||||
default:
|
||||
level = slog.LevelInfo
|
||||
}
|
||||
|
||||
opts := &slog.HandlerOptions{Level: level}
|
||||
|
||||
var handler slog.Handler
|
||||
switch strings.ToLower(format) {
|
||||
case "json":
|
||||
handler = slog.NewJSONHandler(os.Stderr, opts)
|
||||
default:
|
||||
handler = slog.NewTextHandler(os.Stderr, opts)
|
||||
}
|
||||
|
||||
slog.SetDefault(slog.New(handler))
|
||||
}
|
||||
|
||||
func main() {
|
||||
versionFlag := flag.Bool("version", false, "Print version and exit")
|
||||
// Logging flags
|
||||
logFormat := flag.String("log-format", envOrDefault("LOG_FORMAT", "text"), "Log output format: text or json")
|
||||
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")
|
||||
repo := flag.String("repo", envOrDefault("GITEA_REPO", ""), "Repository (owner/name)")
|
||||
@@ -63,15 +30,11 @@ func main() {
|
||||
llmAPIKey := flag.String("llm-api-key", envOrDefault("LLM_API_KEY", ""), "LLM API key")
|
||||
llmModel := flag.String("llm-model", envOrDefault("LLM_MODEL", ""), "LLM model name")
|
||||
conventionsFile := flag.String("conventions-file", envOrDefault("CONVENTIONS_FILE", ""), "Conventions file path in repo (e.g. CLAUDE.md)")
|
||||
systemPromptFile := flag.String("system-prompt-file", envOrDefault("SYSTEM_PROMPT_FILE", ""), "Local file with additional system prompt instructions")
|
||||
patternsRepo := flag.String("patterns-repo", envOrDefault("PATTERNS_REPO", ""), "Repo with language patterns (e.g. rodin/elixir-patterns)")
|
||||
patternsFiles := flag.String("patterns-files", envOrDefault("PATTERNS_FILES", "README.md"), "Comma-separated file paths to fetch from patterns repo")
|
||||
dryRun := flag.Bool("dry-run", false, "Print review to stdout instead of posting")
|
||||
llmTemp := flag.Float64("llm-temperature", envOrDefaultFloat("LLM_TEMPERATURE", 0), "LLM temperature (0 = server default)")
|
||||
llmTimeout := flag.Int("llm-timeout", envOrDefaultInt("LLM_TIMEOUT", 300), "LLM request timeout in seconds (default 300)")
|
||||
llmProvider := flag.String("llm-provider", envOrDefault("LLM_PROVIDER", "openai"), "LLM API provider: openai or anthropic")
|
||||
personaName := flag.String("persona", envOrDefault("PERSONA", ""), "Built-in persona name (security, architect, docs)")
|
||||
personaFile := flag.String("persona-file", envOrDefault("PERSONA_FILE", ""), "Path to persona JSON file")
|
||||
|
||||
flag.Parse()
|
||||
|
||||
@@ -80,10 +43,7 @@ func main() {
|
||||
os.Exit(0)
|
||||
}
|
||||
|
||||
// Initialize structured logger
|
||||
setupLogger(*logFormat, *verbosity)
|
||||
|
||||
slog.Info("review-bot starting", "version", version)
|
||||
log.Printf("review-bot %s", version)
|
||||
|
||||
// Validate required fields
|
||||
if *giteaURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" ||
|
||||
@@ -93,74 +53,28 @@ func main() {
|
||||
os.Exit(1)
|
||||
}
|
||||
|
||||
// Validate persona flags are mutually exclusive
|
||||
if *personaName != "" && *personaFile != "" {
|
||||
slog.Error("--persona and --persona-file are mutually exclusive")
|
||||
os.Exit(1)
|
||||
}
|
||||
|
||||
// Load persona if specified
|
||||
var persona *review.Persona
|
||||
if *personaName != "" {
|
||||
var err error
|
||||
persona, err = review.LoadBuiltinPersona(*personaName)
|
||||
if err != nil {
|
||||
slog.Error("failed to load persona", "persona", *personaName, "error", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
slog.Info("loaded built-in persona", "persona", persona.Name, "display", persona.DisplayName)
|
||||
} else if *personaFile != "" {
|
||||
resolvedPath, err := validateWorkspacePath(*personaFile, "persona-file")
|
||||
if err != nil {
|
||||
slog.Error("invalid persona-file path", "error", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
persona, err = review.LoadPersona(resolvedPath)
|
||||
if err != nil {
|
||||
slog.Error("failed to load persona file", "file", *personaFile, "error", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
slog.Info("loaded persona from file", "file", *personaFile, "persona", persona.Name)
|
||||
}
|
||||
|
||||
// Validate reviewer-name: only safe characters allowed in sentinel
|
||||
if err := validateReviewerName(*reviewerName); err != nil {
|
||||
slog.Error("invalid reviewer name", "error", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
|
||||
// Parse repo owner/name
|
||||
parts := strings.SplitN(*repo, "/", 2)
|
||||
if len(parts) != 2 {
|
||||
slog.Error("invalid repo format", "repo", *repo, "expected", "owner/name")
|
||||
os.Exit(1)
|
||||
log.Fatalf("Invalid repo format %q, expected owner/name", *repo)
|
||||
}
|
||||
owner, repoName := parts[0], parts[1]
|
||||
|
||||
// Parse PR number
|
||||
prNumber, err := strconv.Atoi(*prNum)
|
||||
if err != nil {
|
||||
slog.Error("invalid PR number", "pr", *prNum, "error", err)
|
||||
os.Exit(1)
|
||||
log.Fatalf("Invalid PR number %q: %v", *prNum, err)
|
||||
}
|
||||
|
||||
// Initialize clients
|
||||
giteaClient := gitea.NewClient(*giteaURL, *reviewerToken)
|
||||
llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel)
|
||||
if *llmTemp < 0 || *llmTemp > 2 {
|
||||
slog.Error("invalid LLM temperature", "temperature", *llmTemp, "range", "0-2")
|
||||
os.Exit(1)
|
||||
log.Fatal("--llm-temperature must be between 0 and 2")
|
||||
}
|
||||
if *llmTemp > 0 {
|
||||
llmClient.WithTemperature(*llmTemp)
|
||||
}
|
||||
switch llm.Provider(*llmProvider) {
|
||||
case llm.ProviderOpenAI, llm.ProviderAnthropic:
|
||||
llmClient.WithProvider(llm.Provider(*llmProvider))
|
||||
default:
|
||||
slog.Error("invalid LLM provider", "provider", *llmProvider, "valid", "openai, anthropic")
|
||||
os.Exit(1)
|
||||
}
|
||||
if *llmTimeout > 0 {
|
||||
llmClient.WithTimeout(time.Duration(*llmTimeout) * time.Second)
|
||||
}
|
||||
@@ -170,32 +84,30 @@ func main() {
|
||||
ctx, cancel := context.WithTimeout(context.Background(), overallTimeout)
|
||||
defer cancel()
|
||||
|
||||
slog.Info("reviewing pull request", "pr", prNumber, "repo", fmt.Sprintf("%s/%s", owner, repoName))
|
||||
log.Printf("Reviewing PR #%d on %s/%s", prNumber, owner, repoName)
|
||||
|
||||
// Step 1: Fetch PR metadata
|
||||
pr, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber)
|
||||
if err != nil {
|
||||
slog.Error("failed to fetch PR", "pr", prNumber, "error", err)
|
||||
os.Exit(1)
|
||||
log.Fatalf("Failed to fetch PR: %v", err)
|
||||
}
|
||||
slog.Info("fetched PR metadata", "pr", prNumber, "title", pr.Title)
|
||||
log.Printf("PR: %s", pr.Title)
|
||||
|
||||
// Step 2: Fetch diff
|
||||
diff, err := giteaClient.GetPullRequestDiff(ctx, owner, repoName, prNumber)
|
||||
if err != nil {
|
||||
slog.Error("failed to fetch diff", "pr", prNumber, "error", err)
|
||||
os.Exit(1)
|
||||
log.Fatalf("Failed to fetch diff: %v", err)
|
||||
}
|
||||
slog.Info("fetched diff", "bytes", len(diff))
|
||||
log.Printf("Diff size: %d bytes", len(diff))
|
||||
|
||||
// Step 3: Fetch full file content for modified files
|
||||
fileContext := ""
|
||||
files, err := giteaClient.GetPullRequestFiles(ctx, owner, repoName, prNumber)
|
||||
if err != nil {
|
||||
slog.Warn("could not fetch PR files list", "pr", prNumber, "error", err)
|
||||
log.Printf("Warning: could not fetch PR files list: %v", err)
|
||||
} else {
|
||||
fileContext = fetchFileContext(ctx, giteaClient, owner, repoName, pr.Head.Ref, files)
|
||||
slog.Debug("fetched file context", "files", len(files))
|
||||
log.Printf("Fetched full context for %d files", len(files))
|
||||
}
|
||||
|
||||
// Step 4: Check CI status
|
||||
@@ -204,10 +116,10 @@ func main() {
|
||||
if pr.Head.Sha != "" {
|
||||
statuses, err := giteaClient.GetCommitStatuses(ctx, owner, repoName, pr.Head.Sha)
|
||||
if err != nil {
|
||||
slog.Warn("could not fetch CI status", "sha", pr.Head.Sha, "error", err)
|
||||
log.Printf("Warning: could not fetch CI status: %v", err)
|
||||
} else {
|
||||
ciPassed, ciDetails = evaluateCIStatus(statuses)
|
||||
slog.Info("CI status checked", "passed", ciPassed)
|
||||
log.Printf("CI status: passed=%v", ciPassed)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -216,10 +128,10 @@ func main() {
|
||||
if *conventionsFile != "" {
|
||||
content, err := giteaClient.GetFileContent(ctx, owner, repoName, *conventionsFile)
|
||||
if err != nil {
|
||||
slog.Warn("could not load conventions file", "file", *conventionsFile, "error", err)
|
||||
log.Printf("Warning: could not load conventions file %q: %v", *conventionsFile, err)
|
||||
} else {
|
||||
conventions = content
|
||||
slog.Debug("loaded conventions file", "file", *conventionsFile, "bytes", len(conventions))
|
||||
log.Printf("Loaded conventions file: %s (%d bytes)", *conventionsFile, len(conventions))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -227,39 +139,12 @@ func main() {
|
||||
patterns := ""
|
||||
if *patternsRepo != "" {
|
||||
patterns = fetchPatterns(ctx, giteaClient, *patternsRepo, *patternsFiles)
|
||||
slog.Debug("loaded patterns", "repo", *patternsRepo, "bytes", len(patterns))
|
||||
}
|
||||
|
||||
// Step 6b: Load additional system prompt if specified
|
||||
additionalPrompt := ""
|
||||
if *systemPromptFile != "" {
|
||||
resolvedPath, err := validateWorkspacePath(*systemPromptFile, "system-prompt-file")
|
||||
if err != nil {
|
||||
slog.Error("invalid system-prompt-file path", "error", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
data, err := os.ReadFile(resolvedPath)
|
||||
if err != nil {
|
||||
slog.Error("failed to read system prompt file", "path", *systemPromptFile, "error", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
additionalPrompt = string(data)
|
||||
slog.Debug("loaded system prompt file", "file", *systemPromptFile, "bytes", len(additionalPrompt))
|
||||
log.Printf("Loaded patterns from %s (%d bytes)", *patternsRepo, len(patterns))
|
||||
}
|
||||
|
||||
// Step 7: Budget-aware prompt assembly
|
||||
var systemBase string
|
||||
if persona != nil {
|
||||
systemBase = review.BuildPersonaSystemPrompt(persona)
|
||||
slog.Debug("using persona system prompt", "persona", persona.Name)
|
||||
} else {
|
||||
systemBase = review.BuildSystemBase()
|
||||
}
|
||||
if additionalPrompt != "" {
|
||||
systemBase += "\n\n## Additional Review Instructions\n\n" + additionalPrompt
|
||||
}
|
||||
sections := budget.Sections{
|
||||
SystemBase: systemBase,
|
||||
SystemBase: review.BuildSystemBase(),
|
||||
Patterns: patterns,
|
||||
Conventions: conventions,
|
||||
FileContext: fileContext,
|
||||
@@ -267,66 +152,33 @@ func main() {
|
||||
UserMeta: review.BuildUserMeta(pr.Title, pr.Body, ciPassed, ciDetails),
|
||||
}
|
||||
budgetResult := budget.Fit(*llmModel, sections)
|
||||
slog.Info("token budget calculated", "tokens", budgetResult.EstTokens, "limit", budget.LimitForModel(*llmModel), "model", *llmModel)
|
||||
log.Printf("Token estimate: ~%dK (limit: %dK)", budgetResult.EstTokens/1000, budget.LimitForModel(*llmModel)/1000)
|
||||
if len(budgetResult.Trimmed) > 0 {
|
||||
slog.Warn("context trimmed to fit budget", "trimmed", budgetResult.Trimmed)
|
||||
log.Printf("Context trimmed: %v", budgetResult.Trimmed)
|
||||
}
|
||||
|
||||
// Step 8: Call LLM (with retry on parse failure)
|
||||
slog.Info("sending request to LLM", "model", *llmModel)
|
||||
// Step 8: Call LLM
|
||||
log.Printf("Sending to LLM (%s)...", *llmModel)
|
||||
messages := []llm.Message{
|
||||
{Role: "system", Content: budgetResult.SystemPrompt},
|
||||
{Role: "user", Content: budgetResult.UserPrompt},
|
||||
}
|
||||
|
||||
var response string
|
||||
var result *review.ReviewResult
|
||||
for attempt := 1; attempt <= 2; attempt++ {
|
||||
if attempt > 1 {
|
||||
slog.Warn("retrying LLM request after parse failure", "attempt", attempt)
|
||||
time.Sleep(time.Second)
|
||||
}
|
||||
|
||||
response, err = llmClient.Complete(ctx, messages)
|
||||
if err != nil {
|
||||
slog.Error("LLM request failed", "model", *llmModel, "error", err, "attempt", attempt)
|
||||
if attempt == 2 {
|
||||
os.Exit(1)
|
||||
}
|
||||
continue
|
||||
}
|
||||
slog.Info("LLM response received", "bytes", len(response), "attempt", attempt)
|
||||
|
||||
// Step 9: Parse response
|
||||
result, err = review.ParseResponse(response)
|
||||
if err != nil {
|
||||
slog.Error("failed to parse LLM response", "error", err, "attempt", attempt)
|
||||
if attempt == 2 {
|
||||
os.Exit(1)
|
||||
}
|
||||
continue
|
||||
}
|
||||
break
|
||||
response, err := llmClient.Complete(ctx, messages)
|
||||
if err != nil {
|
||||
log.Fatalf("LLM request failed: %v", err)
|
||||
}
|
||||
slog.Info("review parsed", "verdict", result.Verdict, "findings", len(result.Findings))
|
||||
log.Printf("LLM response received (%d bytes)", len(response))
|
||||
|
||||
// Step 9: Parse response
|
||||
result, err := review.ParseResponse(response)
|
||||
if err != nil {
|
||||
log.Fatalf("Failed to parse LLM response: %v", err)
|
||||
}
|
||||
log.Printf("Verdict: %s (%d findings)", result.Verdict, len(result.Findings))
|
||||
|
||||
// Step 10: Format and post review
|
||||
var reviewBody string
|
||||
if persona != nil && persona.DisplayName != "" {
|
||||
reviewBody = review.FormatMarkdownWithDisplay(result, persona.DisplayName, *reviewerName)
|
||||
} else {
|
||||
reviewBody = review.FormatMarkdown(result, *reviewerName)
|
||||
}
|
||||
|
||||
// Add commit footer so readers know which commit was evaluated
|
||||
if pr.Head.Sha != "" {
|
||||
shortSHA := pr.Head.Sha
|
||||
if len(shortSHA) > 8 {
|
||||
shortSHA = shortSHA[:8]
|
||||
}
|
||||
reviewBody += fmt.Sprintf("\n\n---\n*Evaluated against %s*", shortSHA)
|
||||
}
|
||||
|
||||
reviewBody := review.FormatMarkdown(result, *reviewerName)
|
||||
event := review.GiteaEvent(result.Verdict)
|
||||
|
||||
if *dryRun {
|
||||
@@ -336,124 +188,11 @@ func main() {
|
||||
return
|
||||
}
|
||||
|
||||
sentinel := fmt.Sprintf("<!-- review-bot:%s -->", *reviewerName)
|
||||
|
||||
// Stale check: verify HEAD hasn't moved since we started
|
||||
evaluatedSHA := pr.Head.Sha
|
||||
var currentSHA string
|
||||
currentPR, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber)
|
||||
if err != nil {
|
||||
slog.Warn("could not re-fetch PR for stale check", "pr", prNumber, "error", err)
|
||||
// currentSHA stays empty — shouldSkipStaleReview will return false
|
||||
} else {
|
||||
currentSHA = currentPR.Head.Sha
|
||||
log.Printf("Posting review (event=%s)...", event)
|
||||
if err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody); err != nil {
|
||||
log.Fatalf("Failed to post review: %v", err)
|
||||
}
|
||||
if shouldSkipStaleReview(evaluatedSHA, currentSHA) {
|
||||
slog.Warn("HEAD moved during review — skipping stale review",
|
||||
"evaluated", evaluatedSHA,
|
||||
"current", currentSHA,
|
||||
"pr", prNumber)
|
||||
return
|
||||
}
|
||||
|
||||
// Map findings to inline comments for lines present in the diff
|
||||
diffRanges := gitea.ParseDiffNewLines(diff)
|
||||
var inlineComments []gitea.ReviewComment
|
||||
for _, f := range result.Findings {
|
||||
if f.File != "" && f.Line > 0 && diffRanges.Contains(f.File, f.Line) {
|
||||
inlineComments = append(inlineComments, gitea.ReviewComment{
|
||||
Path: f.File,
|
||||
NewPosition: int64(f.Line),
|
||||
Body: fmt.Sprintf("**[%s]** %s", f.Severity, f.Finding),
|
||||
})
|
||||
}
|
||||
}
|
||||
if len(inlineComments) > 0 {
|
||||
slog.Debug("attaching inline comments", "count", len(inlineComments))
|
||||
}
|
||||
|
||||
// --- Review update strategy ---
|
||||
// 1. POST new review first (gets non-stale approval badge on HEAD)
|
||||
// 2. Then supersede old review with link to the new one
|
||||
// Order matters: post first so we have the new review's URL for the supersede message.
|
||||
var oldReviews []gitea.Review
|
||||
if *reviewerName != "" {
|
||||
existingReviews, err := giteaClient.ListReviews(ctx, owner, repoName, prNumber)
|
||||
if err != nil {
|
||||
slog.Warn("could not list existing reviews", "pr", prNumber, "error", err)
|
||||
} else {
|
||||
if hasSharedToken(existingReviews, sentinel) {
|
||||
slog.Warn("shared token mode: skipping supersede to avoid clobbering sibling review")
|
||||
} else {
|
||||
oldReviews = findAllOwnReviews(existingReviews, sentinel)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Self-request as reviewer (ensures we appear in required-reviewer checks)
|
||||
authUser, err := giteaClient.GetAuthenticatedUser(ctx)
|
||||
if err != nil {
|
||||
slog.Warn("could not determine authenticated user for reviewer self-request", "error", err)
|
||||
} else if authUser != "" {
|
||||
if err := giteaClient.RequestReviewer(ctx, owner, repoName, prNumber, authUser); err != nil {
|
||||
slog.Warn("could not self-request as reviewer", "user", authUser, "error", err)
|
||||
} else {
|
||||
slog.Debug("self-requested as reviewer", "user", authUser, "pr", prNumber)
|
||||
}
|
||||
}
|
||||
|
||||
// POST new review
|
||||
slog.Info("posting review", "event", event, "pr", prNumber)
|
||||
posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)
|
||||
if err != nil {
|
||||
slog.Error("failed to post review", "pr", prNumber, "event", event, "error", err)
|
||||
os.Exit(1)
|
||||
}
|
||||
slog.Info("review posted", "review_id", posted.ID, "user", posted.User.Login, "pr", prNumber)
|
||||
|
||||
// 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)
|
||||
for _, oldReview := range oldReviews {
|
||||
cid, err := giteaClient.GetTimelineReviewCommentIDForReview(ctx, owner, repoName, prNumber, oldReview.ID)
|
||||
if err != nil {
|
||||
slog.Warn("could not find comment ID for old review", "review_id", oldReview.ID, "error", err)
|
||||
continue
|
||||
}
|
||||
supersededBody := buildSupersededBody(oldReview.Body, oldReview.CommitID, newReviewURL, sentinel)
|
||||
if err := giteaClient.EditComment(ctx, owner, repoName, cid, supersededBody); err != nil {
|
||||
slog.Warn("could not mark old review as superseded", "review_id", oldReview.ID, "comment_id", cid, "error", err)
|
||||
continue
|
||||
}
|
||||
slog.Info("marked old review as superseded", "review_id", oldReview.ID, "new_review_id", posted.ID, "pr", prNumber)
|
||||
|
||||
// Resolve old review's inline comments
|
||||
oldComments, err := giteaClient.ListReviewComments(ctx, owner, repoName, prNumber, oldReview.ID)
|
||||
if err != nil {
|
||||
slog.Warn("could not list old review comments for resolution", "review_id", oldReview.ID, "error", err)
|
||||
continue
|
||||
}
|
||||
resolved, failed := 0, 0
|
||||
for _, c := range oldComments {
|
||||
if c.ID == 0 {
|
||||
continue
|
||||
}
|
||||
if err := giteaClient.ResolveComment(ctx, owner, repoName, c.ID); err != nil {
|
||||
slog.Debug("could not resolve inline comment", "comment_id", c.ID, "error", err)
|
||||
failed++
|
||||
} else {
|
||||
resolved++
|
||||
}
|
||||
}
|
||||
if resolved > 0 {
|
||||
slog.Info("resolved old inline comments", "review_id", oldReview.ID, "count", resolved, "pr", prNumber)
|
||||
}
|
||||
if failed > 0 {
|
||||
slog.Warn("some inline comments could not be resolved", "review_id", oldReview.ID, "failed", failed, "pr", prNumber)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
log.Printf("Review posted successfully!")
|
||||
}
|
||||
|
||||
// fetchFileContext fetches the full content of modified files from the PR branch.
|
||||
@@ -468,7 +207,7 @@ func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, re
|
||||
}
|
||||
content, err := client.GetFileContentRef(ctx, owner, repo, f.Filename, ref)
|
||||
if err != nil {
|
||||
slog.Warn("could not fetch file content", "file", f.Filename, "error", err)
|
||||
log.Printf("Warning: could not fetch %s: %v", f.Filename, err)
|
||||
continue
|
||||
}
|
||||
sb.WriteString(fmt.Sprintf("--- %s ---\n", f.Filename))
|
||||
@@ -499,7 +238,7 @@ func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patt
|
||||
}
|
||||
parts := strings.SplitN(repoRef, "/", 2)
|
||||
if len(parts) != 2 {
|
||||
slog.Warn("invalid patterns-repo format", "repo", repoRef, "expected", "owner/name")
|
||||
log.Printf("Warning: invalid patterns-repo format %q, expected owner/name", repoRef)
|
||||
continue
|
||||
}
|
||||
owner, repo := parts[0], parts[1]
|
||||
@@ -512,16 +251,16 @@ func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patt
|
||||
|
||||
files, err := client.GetAllFilesInPath(ctx, owner, repo, path)
|
||||
if err != nil {
|
||||
slog.Warn("could not fetch patterns", "path", path, "repo", repoRef, "error", err)
|
||||
log.Printf("Warning: could not fetch %s from %s: %v", path, repoRef, err)
|
||||
continue
|
||||
}
|
||||
|
||||
for filePath, content := range files {
|
||||
for filepath, content := range files {
|
||||
// Only include markdown and text files as patterns
|
||||
if !isPatternFile(filePath) {
|
||||
if !isPatternFile(filepath) {
|
||||
continue
|
||||
}
|
||||
sb.WriteString(fmt.Sprintf("### %s/%s\n\n%s\n\n", repoRef, filePath, content))
|
||||
sb.WriteString(fmt.Sprintf("### %s/%s\n\n%s\n\n", repoRef, filepath, content))
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -587,173 +326,3 @@ func envOrDefaultInt(key string, defaultVal int) int {
|
||||
}
|
||||
return defaultVal
|
||||
}
|
||||
|
||||
func envOrDefaultBool(key string, defaultVal bool) bool {
|
||||
v := strings.TrimSpace(strings.ToLower(os.Getenv(key)))
|
||||
if v == "" {
|
||||
return defaultVal
|
||||
}
|
||||
return v == "true" || v == "1" || v == "yes"
|
||||
}
|
||||
|
||||
// validateReviewerName checks that the name contains only safe characters
|
||||
// for embedding in an HTML comment sentinel ([a-zA-Z0-9_-]).
|
||||
func validateReviewerName(name string) error {
|
||||
if name == "" {
|
||||
return nil
|
||||
}
|
||||
for _, ch := range name {
|
||||
if !((ch >= 'a' && ch <= 'z') || (ch >= 'A' && ch <= 'Z') || (ch >= '0' && ch <= '9') || ch == '-' || ch == '_') {
|
||||
return fmt.Errorf("reviewer-name must contain only [a-zA-Z0-9_-] (got %q)", name)
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// validateWorkspacePath ensures a file path is within the workspace and resolves
|
||||
// symlinks to prevent traversal attacks. Returns the resolved absolute path or
|
||||
// an error if the path is outside the workspace.
|
||||
func validateWorkspacePath(path, pathName string) (string, error) {
|
||||
workspace := os.Getenv("GITHUB_WORKSPACE")
|
||||
if workspace == "" {
|
||||
workspace, _ = os.Getwd()
|
||||
}
|
||||
absWorkspace, err := filepath.Abs(workspace)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("failed to resolve workspace path: %w", err)
|
||||
}
|
||||
|
||||
// Join and clean the path
|
||||
fullPath := filepath.Join(absWorkspace, path)
|
||||
fullPath = filepath.Clean(fullPath)
|
||||
|
||||
// Check path is within workspace using filepath.Rel (more robust than HasPrefix)
|
||||
rel, err := filepath.Rel(absWorkspace, fullPath)
|
||||
if err != nil || strings.HasPrefix(rel, "..") {
|
||||
return "", fmt.Errorf("%s resolves outside workspace: path=%s workspace=%s", pathName, fullPath, absWorkspace)
|
||||
}
|
||||
|
||||
// Resolve symlinks and re-validate to prevent symlink traversal
|
||||
resolvedPath, err := filepath.EvalSymlinks(fullPath)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("failed to resolve %s: %w", pathName, err)
|
||||
}
|
||||
|
||||
relResolved, err := filepath.Rel(absWorkspace, resolvedPath)
|
||||
if err != nil || strings.HasPrefix(relResolved, "..") {
|
||||
return "", fmt.Errorf("%s symlink resolves outside workspace: resolved=%s workspace=%s", pathName, resolvedPath, absWorkspace)
|
||||
}
|
||||
|
||||
return resolvedPath, nil
|
||||
}
|
||||
|
||||
// buildSupersededBody creates the body for a superseded review: struck-through banner
|
||||
// with collapsed original content and the commit it was evaluated against.
|
||||
func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string) string {
|
||||
shortSHA := commitSHA
|
||||
if len(shortSHA) > 8 {
|
||||
shortSHA = shortSHA[:8]
|
||||
}
|
||||
var sb strings.Builder
|
||||
sb.WriteString("~~Original review~~\n\n")
|
||||
sb.WriteString("**Superseded** \u2014 [see current review](")
|
||||
sb.WriteString(newReviewURL)
|
||||
sb.WriteString(") for up-to-date findings.\n\n")
|
||||
if shortSHA != "" {
|
||||
sb.WriteString("<details><summary>Previous findings (commit ")
|
||||
sb.WriteString(shortSHA)
|
||||
sb.WriteString(")</summary>\n\n")
|
||||
} else {
|
||||
sb.WriteString("<details><summary>Previous findings</summary>\n\n")
|
||||
}
|
||||
sb.WriteString(originalBody)
|
||||
sb.WriteString("\n\n</details>\n\n")
|
||||
sb.WriteString(sentinel)
|
||||
return sb.String()
|
||||
}
|
||||
|
||||
// hasSharedToken detects if another review-bot role posted under the same
|
||||
// Gitea user. This indicates misconfiguration where two roles share a token
|
||||
// instead of having separate Gitea accounts. Returns true if shared token
|
||||
// detected (caller should skip update-in-place logic to avoid clobbering).
|
||||
func hasSharedToken(reviews []gitea.Review, ownSentinel string) bool {
|
||||
ownLogin := ""
|
||||
for _, r := range reviews {
|
||||
if strings.Contains(r.Body, ownSentinel) {
|
||||
ownLogin = r.User.Login
|
||||
break
|
||||
}
|
||||
}
|
||||
if ownLogin == "" {
|
||||
return false
|
||||
}
|
||||
for _, r := range reviews {
|
||||
if r.User.Login == ownLogin && strings.Contains(r.Body, "<!-- review-bot:") && !strings.Contains(r.Body, ownSentinel) {
|
||||
slog.Warn("shared token detected — another review-bot role is using the same Gitea user",
|
||||
"sibling_role", extractSentinelName(r.Body), "user", ownLogin)
|
||||
return true
|
||||
}
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// extractSentinelName pulls the reviewer name from a sentinel comment.
|
||||
func extractSentinelName(body string) string {
|
||||
const prefix = "<!-- review-bot:"
|
||||
const suffix = " -->"
|
||||
idx := strings.Index(body, prefix)
|
||||
if idx < 0 {
|
||||
return "unknown"
|
||||
}
|
||||
rest := body[idx+len(prefix):]
|
||||
end := strings.Index(rest, suffix)
|
||||
if end < 0 {
|
||||
return "unknown"
|
||||
}
|
||||
return rest[:end]
|
||||
}
|
||||
|
||||
// findOwnReview locates the most recent non-superseded review matching the sentinel.
|
||||
func findOwnReview(reviews []gitea.Review, sentinel string) *gitea.Review {
|
||||
var best *gitea.Review
|
||||
for i := range reviews {
|
||||
if !strings.Contains(reviews[i].Body, sentinel) {
|
||||
continue
|
||||
}
|
||||
if strings.Contains(reviews[i].Body, "~~Original review~~") {
|
||||
continue
|
||||
}
|
||||
if best == nil || reviews[i].ID > best.ID {
|
||||
best = &reviews[i]
|
||||
}
|
||||
}
|
||||
return best
|
||||
}
|
||||
|
||||
// findAllOwnReviews returns all non-superseded reviews matching the sentinel.
|
||||
func findAllOwnReviews(reviews []gitea.Review, sentinel string) []gitea.Review {
|
||||
var result []gitea.Review
|
||||
for i := range reviews {
|
||||
if !strings.Contains(reviews[i].Body, sentinel) {
|
||||
continue
|
||||
}
|
||||
if strings.Contains(reviews[i].Body, "~~Original review~~") {
|
||||
continue
|
||||
}
|
||||
result = append(result, reviews[i])
|
||||
}
|
||||
return result
|
||||
}
|
||||
|
||||
// shouldSkipStaleReview reports whether to skip posting because HEAD moved.
|
||||
// Returns true (skip) if evaluatedSHA differs from currentSHA.
|
||||
// Returns false (don't skip) if:
|
||||
// - SHAs match (no movement)
|
||||
// - currentSHA is empty (re-fetch failed; prefer posting stale over failing)
|
||||
func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool {
|
||||
if currentSHA == "" {
|
||||
// Re-fetch failed; better to post potentially stale than fail
|
||||
return false
|
||||
}
|
||||
return evaluatedSHA != currentSHA
|
||||
}
|
||||
|
||||
File diff suppressed because it is too large
Load Diff
@@ -1,334 +0,0 @@
|
||||
# Design: Role-based Review Personas (Issue #51)
|
||||
|
||||
> **Note:** This design was revised during implementation to use JSON instead of YAML
|
||||
> to maintain the repository's zero-external-dependencies convention. All persona
|
||||
> files use JSON format. See "Design Revision" section at the end for details.
|
||||
|
||||
## Problem
|
||||
|
||||
Current review-bot performs generic code review. Every reviewer (regardless of `reviewer-name`) uses the same base prompt and evaluates the same concerns. This leads to:
|
||||
|
||||
1. **Redundancy** — Two reviewers (e.g., GPT + Claude twins) often flag identical issues
|
||||
2. **Gaps** — Generic reviewers miss specialized concerns (security, domain logic, architecture)
|
||||
3. **Noise** — NITs about style mixed with critical security findings
|
||||
4. **No ownership** — Findings lack clear domain attribution
|
||||
|
||||
## Constraints
|
||||
|
||||
- Must work with existing CLI flags and CI workflow patterns
|
||||
- Must not break backwards compatibility (existing configs still work)
|
||||
- Must integrate cleanly with the budget system (personas add to context)
|
||||
- Multiple personas running in parallel must not interfere with each other
|
||||
- Each persona must have clear scope boundaries (no duplication)
|
||||
|
||||
## Proposed Approach
|
||||
|
||||
### 1. Persona Definition
|
||||
|
||||
A persona is a named review role with:
|
||||
- **Identity** — Who am I? What's my expertise?
|
||||
- **Focus** — What do I look for?
|
||||
- **Scope boundaries** — What do I explicitly NOT comment on?
|
||||
- **Severity calibration** — What counts as MAJOR/MINOR/NIT for MY domain?
|
||||
|
||||
Personas are defined in JSON files that can live:
|
||||
1. In the pattern repos (shared across projects)
|
||||
2. In the target repo (project-specific personas)
|
||||
3. Inline via a new `--persona-file` flag (JSON format)
|
||||
|
||||
### 2. Persona File Format
|
||||
|
||||
```json
|
||||
# .review/personas/security.yaml
|
||||
name: security
|
||||
display_name: Security Specialist
|
||||
model_preference: opus # optional hint for expensive analysis
|
||||
|
||||
identity: |
|
||||
You are a security specialist reviewing code for vulnerabilities.
|
||||
Your expertise: OWASP Top 10, injection attacks, auth/authz, secrets management,
|
||||
event sourcing security (replay attacks, event injection).
|
||||
|
||||
focus:
|
||||
- Injection attacks (SQL, command, path traversal, template)
|
||||
- Authentication and authorization gaps
|
||||
- Secrets exposure (hardcoded credentials, tokens in logs)
|
||||
- Input validation (unsanitized input, unsafe deserialization)
|
||||
- Race conditions with security implications
|
||||
- Event sourcing attack vectors
|
||||
|
||||
ignore:
|
||||
- Code style and naming conventions
|
||||
- Performance (unless security-related)
|
||||
- Documentation
|
||||
- General code quality
|
||||
- Test coverage
|
||||
|
||||
severity:
|
||||
critical: "Remote code execution, auth bypass, data exfiltration"
|
||||
major: "Privilege escalation, information disclosure, DoS"
|
||||
minor: "Missing rate limiting, verbose errors"
|
||||
nit: "Theoretical risk with low exploitability"
|
||||
|
||||
output_format: |
|
||||
For each finding:
|
||||
- Severity: [CRITICAL|MAJOR|MINOR|NIT]
|
||||
- Attack vector: How could this be exploited?
|
||||
- Evidence: Code snippet showing the vulnerability
|
||||
- Recommendation: Specific fix
|
||||
```
|
||||
|
||||
### 3. New CLI Flags
|
||||
|
||||
```
|
||||
--persona-file PATH Path to persona JSON file (local or in repo)
|
||||
--persona NAME Built-in persona name (security, architect, domain)
|
||||
```
|
||||
|
||||
Either flag sets the persona. If neither is provided, behavior is unchanged (generic review).
|
||||
|
||||
### 4. Prompt Assembly
|
||||
|
||||
Current flow:
|
||||
```
|
||||
SystemBase → Patterns → Conventions → [LLM]
|
||||
```
|
||||
|
||||
New flow with persona:
|
||||
```
|
||||
PersonaPrompt (from YAML) → Patterns (filtered?) → Conventions → [LLM]
|
||||
```
|
||||
|
||||
The persona's identity/focus/ignore/severity sections become the system prompt, replacing the generic "You are an expert code reviewer" base.
|
||||
|
||||
### 5. Built-in Personas
|
||||
|
||||
Ship with these built-in personas (loadable via `--persona NAME`):
|
||||
|
||||
| Name | Focus |
|
||||
|------|-------|
|
||||
| `security` | Vulnerabilities, auth, secrets |
|
||||
| `architect` | Patterns, consistency, design |
|
||||
| `domain` | Business logic (requires repo-specific config) |
|
||||
| `docs` | Documentation, API clarity |
|
||||
|
||||
Built-in personas live in `review/personas/` as embedded Go assets or YAML shipped with the binary.
|
||||
|
||||
### 6. CI Workflow Integration
|
||||
|
||||
Single persona:
|
||||
```yaml
|
||||
- uses: rodin/review-bot/.gitea/actions/review@v1
|
||||
with:
|
||||
reviewer-name: security
|
||||
persona: security
|
||||
...
|
||||
```
|
||||
|
||||
Multiple personas (parallel jobs):
|
||||
```yaml
|
||||
jobs:
|
||||
review:
|
||||
strategy:
|
||||
matrix:
|
||||
include:
|
||||
- name: security
|
||||
persona: security
|
||||
- name: architect
|
||||
persona: architect
|
||||
steps:
|
||||
- uses: rodin/review-bot/.gitea/actions/review@v1
|
||||
with:
|
||||
reviewer-name: ${{ matrix.name }}
|
||||
persona: ${{ matrix.persona }}
|
||||
```
|
||||
|
||||
Custom persona from repo:
|
||||
```yaml
|
||||
- uses: rodin/review-bot/.gitea/actions/review@v1
|
||||
with:
|
||||
reviewer-name: trading
|
||||
persona-file: .review/personas/trading.yaml
|
||||
```
|
||||
|
||||
### 7. Persona + Patterns Interaction
|
||||
|
||||
Some personas benefit from filtered patterns:
|
||||
- Security → only security-related patterns
|
||||
- Architect → all patterns (structural focus)
|
||||
- Domain → domain docs, not language patterns
|
||||
|
||||
For v1, keep it simple: all patterns are included regardless of persona. Future enhancement could add `patterns_filter` to persona YAML.
|
||||
|
||||
### 8. Output Format Changes
|
||||
|
||||
Persona name appears in the review header:
|
||||
```markdown
|
||||
# Security Review
|
||||
|
||||
## Summary
|
||||
No critical vulnerabilities found in this change.
|
||||
|
||||
## Findings
|
||||
| # | Severity | File | Line | Finding |
|
||||
...
|
||||
|
||||
## Recommendation
|
||||
**APPROVE** — No security-relevant issues detected.
|
||||
|
||||
---
|
||||
*Review by security*
|
||||
<!-- review-bot:security -->
|
||||
```
|
||||
|
||||
## State/Data Model
|
||||
|
||||
### Persona struct
|
||||
|
||||
```go
|
||||
// review/persona.go
|
||||
type Persona struct {
|
||||
Name string `yaml:"name"`
|
||||
DisplayName string `yaml:"display_name"`
|
||||
ModelPref string `yaml:"model_preference,omitempty"`
|
||||
Identity string `yaml:"identity"`
|
||||
Focus []string `yaml:"focus"`
|
||||
Ignore []string `yaml:"ignore"`
|
||||
Severity Severity `yaml:"severity"`
|
||||
OutputFormat string `yaml:"output_format,omitempty"`
|
||||
}
|
||||
|
||||
type Severity struct {
|
||||
Critical string `yaml:"critical"`
|
||||
Major string `yaml:"major"`
|
||||
Minor string `yaml:"minor"`
|
||||
Nit string `yaml:"nit"`
|
||||
}
|
||||
```
|
||||
|
||||
### Loading precedence
|
||||
|
||||
1. `--persona-file PATH` → load from local file system
|
||||
2. `--persona NAME` → load from embedded built-ins
|
||||
3. Neither → use generic system prompt (current behavior)
|
||||
|
||||
## Error Cases
|
||||
|
||||
| Error | Handling |
|
||||
|-------|----------|
|
||||
| Persona file not found | Fatal exit with clear message |
|
||||
| Invalid YAML in persona file | Fatal exit with parse error |
|
||||
| Both `--persona` and `--persona-file` specified | Fatal exit: mutually exclusive |
|
||||
| Unknown built-in persona name | Fatal exit with list of valid names |
|
||||
| Empty identity in persona | Warning, fall back to generic prompt |
|
||||
|
||||
## Edge Cases
|
||||
|
||||
- **Empty focus list**: Valid — persona relies on identity alone
|
||||
- **Empty ignore list**: Valid — no explicit scope exclusions
|
||||
- **No severity section**: Use default MAJOR/MINOR/NIT definitions
|
||||
- **Model preference set but budget insufficient**: Ignore preference, log warning
|
||||
- **Persona file in pattern repo**: Fetch like other pattern files
|
||||
|
||||
## Testing Strategy
|
||||
|
||||
### Unit tests
|
||||
- `persona_test.go`: Parse valid/invalid YAML, validate required fields
|
||||
- `prompt_test.go`: Verify persona prompt assembly
|
||||
- Integration with budget: persona prompts count toward token limit
|
||||
|
||||
### Integration tests
|
||||
- End-to-end with `--persona security` (built-in)
|
||||
- End-to-end with `--persona-file custom.yaml`
|
||||
- Backwards compatibility: no flags = generic behavior
|
||||
|
||||
### Manual verification
|
||||
- Run security persona on a PR with obvious vulnerability
|
||||
- Verify security persona ignores style issues
|
||||
- Verify non-security persona doesn't flag security issues
|
||||
|
||||
## Implementation Phases
|
||||
|
||||
### Phase 1: Persona types and loading
|
||||
- [ ] `review/persona.go`: Persona struct + YAML parsing
|
||||
- [ ] `review/persona_test.go`: Unit tests
|
||||
- [ ] Embed built-in personas in binary
|
||||
- [ ] Compiles clean, tests pass
|
||||
|
||||
### Phase 2: Prompt generation
|
||||
- [ ] `review/prompt.go`: `BuildPersonaPrompt(p Persona) string`
|
||||
- [ ] Modify `BuildSystemBase()` to accept optional persona
|
||||
- [ ] Integrate persona prompt with budget system
|
||||
- [ ] Tests for prompt assembly
|
||||
|
||||
### Phase 3: CLI integration
|
||||
- [ ] Add `--persona` and `--persona-file` flags
|
||||
- [ ] Flag validation (mutually exclusive, valid names)
|
||||
- [ ] Load persona based on flags
|
||||
- [ ] Pass persona to prompt builder
|
||||
|
||||
### Phase 4: Action integration
|
||||
- [ ] Add `persona` and `persona-file` inputs to action.yml
|
||||
- [ ] Update README with persona examples
|
||||
- [ ] End-to-end CI test
|
||||
|
||||
### Phase 5: Built-in personas
|
||||
- [ ] `security.yaml` built-in
|
||||
- [ ] `architect.yaml` built-in
|
||||
- [ ] `docs.yaml` built-in
|
||||
- [ ] Document each persona's focus
|
||||
|
||||
## Open Questions
|
||||
|
||||
1. **Persona file location in repo**: Should we support `--persona-file .review/security.yaml` where the file is fetched from the PR's repo (like conventions)? This adds complexity but enables project-specific personas without action changes.
|
||||
|
||||
2. **Model preference enforcement**: If persona specifies `model_preference: opus` but the action uses a different model, should we warn? Override? Ignore? Current thinking: log warning, use the specified model (user controls model via action input).
|
||||
|
||||
3. **Severity override output**: If persona defines custom severity levels (CRITICAL), should the JSON output include them, or map back to standard MAJOR/MINOR/NIT? Current thinking: keep standard output format, use severity calibration only for prompt guidance.
|
||||
|
||||
## Completion Checklist
|
||||
|
||||
1. Persona struct matches YAML schema exactly?
|
||||
2. Built-in personas embedded in binary (not external files)?
|
||||
3. `--persona` and `--persona-file` are mutually exclusive?
|
||||
4. Unknown persona name produces clear error with valid options?
|
||||
5. Empty persona file fields have sensible defaults?
|
||||
6. Persona prompt integrates with budget system (token counting)?
|
||||
7. Backwards compatibility: no flags = current behavior?
|
||||
8. Review header shows persona display name?
|
||||
9. Sentinel still uses reviewer-name (not persona name)?
|
||||
10. Unit tests cover parse errors, missing fields, valid YAML?
|
||||
|
||||
## Design Review Findings (Self-Review)
|
||||
|
||||
### Finding 1: Severity Mapping
|
||||
The persona YAML allows `critical` severity, but the LLM output parser (`review/parser.go`) only accepts MAJOR/MINOR/NIT.
|
||||
|
||||
**Resolution:** Keep standard output format. Persona severity section is ONLY for calibrating the LLM's judgment (prompt guidance). Output must still use MAJOR/MINOR/NIT. Document this clearly in persona format docs.
|
||||
|
||||
### Finding 2: Embedding Built-in Personas
|
||||
Go doesn't natively embed YAML. Must use `//go:embed` directive (Go 1.16+).
|
||||
|
||||
**Resolution:** Create `review/personas/` directory with YAML files and use:
|
||||
```go
|
||||
//go:embed personas/*.yaml
|
||||
var embeddedPersonas embed.FS
|
||||
```
|
||||
|
||||
### Finding 3: display_name vs reviewer-name
|
||||
Design says header shows "persona display name" but sentinel uses "reviewer-name". This is correct - they serve different purposes:
|
||||
- `display_name` → human-readable header ("Security Specialist Review")
|
||||
- `reviewer-name` → machine sentinel for cleanup (`<!-- review-bot:security -->`)
|
||||
|
||||
When persona is used, `display_name` takes precedence for the header title, but `reviewer-name` (CLI flag) is still used for the sentinel.
|
||||
|
||||
## Design Revision: YAML with gopkg.in/yaml.v3
|
||||
|
||||
**Decision:** Add `gopkg.in/yaml.v3` as a dependency.
|
||||
|
||||
YAML is preferred over JSON for persona files because:
|
||||
- Multi-line strings are cleaner (no escaping quotes in identity/focus text)
|
||||
- Comments are supported for documentation
|
||||
- More human-readable for complex persona definitions
|
||||
|
||||
The implementation supports both YAML (`.yaml`, `.yml`) and JSON (`.json`) for backwards compatibility, with YAML as the default for built-in personas.
|
||||
@@ -1,97 +0,0 @@
|
||||
# Review Update Strategy
|
||||
|
||||
review-bot uses an **edit-in-place** strategy for updating reviews. Reviews are never deleted — this preserves conversation threads on inline comments.
|
||||
|
||||
## State Transition Diagram
|
||||
|
||||
```mermaid
|
||||
stateDiagram-v2
|
||||
[*] --> NoExistingReview: First run
|
||||
|
||||
NoExistingReview --> POST_Review: Generate findings + event
|
||||
POST_Review --> PostEscalationCheck: event == APPROVED?
|
||||
|
||||
PostEscalationCheck --> Done: No sibling blocks
|
||||
PostEscalationCheck --> Supersede_And_Repost: Sibling has REQUEST_CHANGES
|
||||
Supersede_And_Repost --> Done: Posted as REQUEST_CHANGES
|
||||
|
||||
[*] --> ExistingReviewFound: Subsequent run (sentinel match)
|
||||
|
||||
ExistingReviewFound --> CheckEscalation: Determine final event
|
||||
CheckEscalation --> CompareState: Apply worst-wins if needed
|
||||
|
||||
CompareState --> SameState: existing.state == new event
|
||||
CompareState --> StateChange: existing.state != new event
|
||||
|
||||
SameState --> Skip: Body unchanged
|
||||
SameState --> PatchBody: Body changed → PATCH in place
|
||||
|
||||
StateChange --> Escalate: APPROVED → REQUEST_CHANGES
|
||||
StateChange --> Downgrade: REQUEST_CHANGES → APPROVED
|
||||
|
||||
Escalate --> Supersede: PATCH old body → "Superseded"
|
||||
Supersede --> POST_New_RC: POST new REQUEST_CHANGES
|
||||
|
||||
Downgrade --> POST_New_Approve: POST new APPROVED (old stays intact)
|
||||
|
||||
Skip --> Done
|
||||
PatchBody --> Done
|
||||
POST_New_RC --> Done
|
||||
POST_New_Approve --> Done
|
||||
```
|
||||
|
||||
## Rules
|
||||
|
||||
| Scenario | Action | Reason |
|
||||
|----------|--------|--------|
|
||||
| No existing review | POST new | First run |
|
||||
| Same state, same body | Skip | Nothing changed — preserve threads |
|
||||
| Same state, body changed | PATCH body | Update findings without losing threads |
|
||||
| APPROVED → REQUEST_CHANGES | Supersede old + POST new | Can always escalate; old APPROVED is no longer valid |
|
||||
| REQUEST_CHANGES → APPROVED | POST new APPROVED | Can't edit state; old REQUEST_CHANGES stays as historical record |
|
||||
| Sibling has REQUEST_CHANGES (worst-wins) | Escalate to REQUEST_CHANGES | PR must stay blocked if ANY reviewer blocks |
|
||||
|
||||
## Key Constraints
|
||||
|
||||
1. **Review state is immutable after POST** — Gitea has no API to change APPROVED ↔ REQUEST_CHANGES
|
||||
2. **Never delete reviews** — Deleting cascades to inline comments and reply threads
|
||||
3. **"Last review per user" wins** — Gitea uses the most recent review from a user for merge decisions
|
||||
4. **REQUEST_CHANGES reviews are never touched** — Their inline comments and threads are preserved as historical record
|
||||
5. **APPROVED reviews can be superseded** — When escalation is needed, mark old as superseded and POST new
|
||||
|
||||
## Worst-Wins (Shared Token)
|
||||
|
||||
When multiple reviewer roles share a token (e.g., `sonnet` and `security` both use `sonnet-review-bot`):
|
||||
|
||||
```
|
||||
CI Matrix Run:
|
||||
sonnet → REQUEST_CHANGES (findings)
|
||||
security → APPROVED (no security issues)
|
||||
↓
|
||||
security sees sibling REQUEST_CHANGES
|
||||
↓
|
||||
security escalates → REQUEST_CHANGES
|
||||
↓
|
||||
PR stays blocked ✓
|
||||
```
|
||||
|
||||
The **first-run case** (no existing review to read login from) uses a post-posting fallback:
|
||||
POST APPROVED → check siblings → if blocked, supersede own APPROVED → re-POST as REQUEST_CHANGES.
|
||||
|
||||
## Edit Mechanism
|
||||
|
||||
Reviews are edited via `PATCH /repos/{owner}/{repo}/issues/comments/{id}`:
|
||||
|
||||
- **Review body**: ID obtained from the timeline API (`/issues/{index}/timeline`, type `"review"`)
|
||||
- **Inline comments**: IDs obtained from `/pulls/{index}/reviews/{id}/comments`
|
||||
- **Both are editable** by the token that created them
|
||||
- **ListReviews always returns the original body** (reads from review table, not comment table) — sentinel matching works regardless of edits
|
||||
|
||||
## Inline Comments Lifecycle
|
||||
|
||||
| Event | Inline comments behavior |
|
||||
|-------|--------------------------|
|
||||
| First POST | Created on specific diff lines |
|
||||
| PATCH body (same state) | Unchanged — still current findings |
|
||||
| Supersede (state change) | Old inline comments stay (readable but on outdated code) |
|
||||
| New POST after supersede | Fresh inline comments on current diff |
|
||||
+26
-393
@@ -7,38 +7,15 @@ import (
|
||||
"bytes"
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"log/slog"
|
||||
"log"
|
||||
"net/http"
|
||||
"net/url"
|
||||
"strings"
|
||||
"time"
|
||||
)
|
||||
|
||||
// APIError represents an HTTP error response from the Gitea API.
|
||||
// It carries the status code so callers can distinguish between
|
||||
// different failure modes (e.g. 404 vs 500).
|
||||
type APIError struct {
|
||||
StatusCode int
|
||||
Body string
|
||||
}
|
||||
|
||||
func (e *APIError) Error() string {
|
||||
body := e.Body
|
||||
if len(body) > 200 {
|
||||
body = body[:200] + "...(truncated)"
|
||||
}
|
||||
return fmt.Sprintf("HTTP %d: %s", e.StatusCode, body)
|
||||
}
|
||||
|
||||
// IsNotFound reports whether an error is an API 404 response.
|
||||
func IsNotFound(err error) bool {
|
||||
var apiErr *APIError
|
||||
return errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound
|
||||
}
|
||||
|
||||
// Client interacts with the Gitea API.
|
||||
// A Client is safe for concurrent use by multiple goroutines.
|
||||
type Client struct {
|
||||
@@ -80,17 +57,9 @@ type ChangedFile struct {
|
||||
Status string `json:"status"`
|
||||
}
|
||||
|
||||
// ReviewComment represents an inline comment to attach to a review.
|
||||
type ReviewComment struct {
|
||||
ID int64 `json:"id,omitempty"`
|
||||
Path string `json:"path"`
|
||||
NewPosition int64 `json:"new_position"`
|
||||
Body string `json:"body"`
|
||||
}
|
||||
|
||||
// GetPullRequest fetches PR metadata.
|
||||
func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d", c.baseURL, owner, repo, number)
|
||||
body, err := c.doGet(ctx, reqURL)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("fetch PR: %w", err)
|
||||
@@ -104,7 +73,7 @@ func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number
|
||||
|
||||
// GetPullRequestDiff fetches the unified diff for a PR.
|
||||
func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d.diff", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d.diff", c.baseURL, owner, repo, number)
|
||||
body, err := c.doGet(ctx, reqURL)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("fetch diff: %w", err)
|
||||
@@ -114,7 +83,7 @@ func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, num
|
||||
|
||||
// GetPullRequestFiles fetches the list of files changed in a PR.
|
||||
func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error) {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/files", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/files", c.baseURL, owner, repo, number)
|
||||
body, err := c.doGet(ctx, reqURL)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("fetch PR files: %w", err)
|
||||
@@ -128,7 +97,7 @@ func (c *Client) GetPullRequestFiles(ctx context.Context, owner, repo string, nu
|
||||
|
||||
// GetCommitStatuses fetches CI statuses for a commit SHA.
|
||||
func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string) ([]CommitStatus, error) {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/commits/%s/statuses", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), url.PathEscape(sha))
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/commits/%s/statuses", c.baseURL, owner, repo, sha)
|
||||
body, err := c.doGet(ctx, reqURL)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("fetch commit statuses: %w", err)
|
||||
@@ -142,7 +111,7 @@ func (c *Client) GetCommitStatuses(ctx context.Context, owner, repo, sha string)
|
||||
|
||||
// GetFileContent fetches a file from the default branch of a repo.
|
||||
func (c *Client) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(filepath))
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s", c.baseURL, owner, repo, escapePath(filepath))
|
||||
body, err := c.doGet(ctx, reqURL)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("fetch file %s: %w", filepath, err)
|
||||
@@ -152,7 +121,7 @@ func (c *Client) GetFileContent(ctx context.Context, owner, repo, filepath strin
|
||||
|
||||
// GetFileContentRef fetches a file from a specific ref (branch/tag/sha) in a repo.
|
||||
func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, ref string) (string, error) {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s?ref=%s", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(filepath), url.QueryEscape(ref))
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s?ref=%s", c.baseURL, owner, repo, escapePath(filepath), url.QueryEscape(ref))
|
||||
body, err := c.doGet(ctx, reqURL)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("fetch file %s@%s: %w", filepath, ref, err)
|
||||
@@ -160,54 +129,42 @@ func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, r
|
||||
return string(body), nil
|
||||
}
|
||||
|
||||
// PostReview submits a review to a PR and returns the created review.
|
||||
// PostReview submits a review to a PR.
|
||||
// event should be "APPROVED" or "REQUEST_CHANGES".
|
||||
// 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) {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
|
||||
func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string) error {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, owner, repo, number)
|
||||
|
||||
payload := struct {
|
||||
Body string `json:"body"`
|
||||
Event string `json:"event"`
|
||||
Comments []ReviewComment `json:"comments,omitempty"`
|
||||
Body string `json:"body"`
|
||||
Event string `json:"event"`
|
||||
}{
|
||||
Body: body,
|
||||
Event: event,
|
||||
Comments: comments,
|
||||
Body: body,
|
||||
Event: event,
|
||||
}
|
||||
|
||||
data, err := json.Marshal(payload)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("marshal review payload: %w", err)
|
||||
return fmt.Errorf("marshal review payload: %w", err)
|
||||
}
|
||||
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data))
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("create review request: %w", err)
|
||||
return fmt.Errorf("create review request: %w", err)
|
||||
}
|
||||
req.Header.Set("Authorization", "token "+c.token)
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
|
||||
resp, err := c.http.Do(req)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("post review: %w", err)
|
||||
return fmt.Errorf("post review: %w", err)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
|
||||
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
||||
respBody, _ := io.ReadAll(resp.Body)
|
||||
return nil, fmt.Errorf("post review failed (status %d): %s", resp.StatusCode, string(respBody))
|
||||
return fmt.Errorf("post review failed (status %d): %s", resp.StatusCode, string(respBody))
|
||||
}
|
||||
|
||||
respBody, err := io.ReadAll(resp.Body)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("read review response: %w", err)
|
||||
}
|
||||
var review Review
|
||||
if err := json.Unmarshal(respBody, &review); err != nil {
|
||||
return nil, fmt.Errorf("parse review response: %w", err)
|
||||
}
|
||||
return &review, nil
|
||||
return nil
|
||||
}
|
||||
|
||||
func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
|
||||
@@ -225,7 +182,7 @@ func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
|
||||
|
||||
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
||||
body, _ := io.ReadAll(resp.Body)
|
||||
return nil, &APIError{StatusCode: resp.StatusCode, Body: string(body)}
|
||||
return nil, fmt.Errorf("HTTP %d: %s", resp.StatusCode, string(body))
|
||||
}
|
||||
return io.ReadAll(resp.Body)
|
||||
}
|
||||
@@ -254,9 +211,9 @@ type ContentEntry struct {
|
||||
func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) {
|
||||
var reqURL string
|
||||
if path == "" {
|
||||
reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents", c.baseURL, url.PathEscape(owner), url.PathEscape(repo))
|
||||
reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents", c.baseURL, owner, repo)
|
||||
} else {
|
||||
reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents/%s", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), escapePath(path))
|
||||
reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents/%s", c.baseURL, owner, repo, escapePath(path))
|
||||
}
|
||||
body, err := c.doGet(ctx, reqURL)
|
||||
if err != nil {
|
||||
@@ -278,15 +235,10 @@ func (c *Client) GetAllFilesInPath(ctx context.Context, owner, repo, path string
|
||||
// Try listing as directory first
|
||||
entries, err := c.ListContents(ctx, owner, repo, path)
|
||||
if err != nil {
|
||||
// Only fall back to single-file fetch on 404 (path is a file, not a dir).
|
||||
// Propagate all other errors (auth failures, server errors, rate limits).
|
||||
if !IsNotFound(err) {
|
||||
return nil, fmt.Errorf("list contents %q: %w", path, err)
|
||||
}
|
||||
// 404 means the path might be a file — try fetching directly
|
||||
// Might be a file, try fetching directly
|
||||
content, fileErr := c.GetFileContent(ctx, owner, repo, path)
|
||||
if fileErr != nil {
|
||||
return nil, fmt.Errorf("path %q is neither a file nor directory: %w", path, fileErr)
|
||||
return nil, fmt.Errorf("path %q is neither a file nor directory: %w", path, err)
|
||||
}
|
||||
results[path] = content
|
||||
return results, nil
|
||||
@@ -297,14 +249,14 @@ func (c *Client) GetAllFilesInPath(ctx context.Context, owner, repo, path string
|
||||
case "file":
|
||||
content, err := c.GetFileContent(ctx, owner, repo, entry.Path)
|
||||
if err != nil {
|
||||
slog.Warn("could not fetch file from patterns repo", "file", entry.Path, "error", err)
|
||||
log.Printf("Warning: could not fetch file %s: %v", entry.Path, err)
|
||||
continue
|
||||
}
|
||||
results[entry.Path] = content
|
||||
case "dir":
|
||||
subResults, err := c.GetAllFilesInPath(ctx, owner, repo, entry.Path)
|
||||
if err != nil {
|
||||
slog.Warn("could not recurse into directory", "dir", entry.Path, "error", err)
|
||||
log.Printf("Warning: could not recurse into %s: %v", entry.Path, err)
|
||||
continue
|
||||
}
|
||||
for k, v := range subResults {
|
||||
@@ -314,322 +266,3 @@ func (c *Client) GetAllFilesInPath(ctx context.Context, owner, repo, path string
|
||||
}
|
||||
return results, nil
|
||||
}
|
||||
|
||||
// Review represents a pull request review from the Gitea API.
|
||||
type Review struct {
|
||||
ID int64 `json:"id"`
|
||||
Body string `json:"body"`
|
||||
User struct {
|
||||
Login string `json:"login"`
|
||||
} `json:"user"`
|
||||
State string `json:"state"`
|
||||
Stale bool `json:"stale"`
|
||||
CommitID string `json:"commit_id"`
|
||||
}
|
||||
|
||||
// ListReviews returns all reviews on a pull request.
|
||||
// Paginates through all pages to ensure no reviews are missed.
|
||||
func (c *Client) ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error) {
|
||||
const pageSize = 50
|
||||
var all []Review
|
||||
for page := 1; ; page++ {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews?limit=%d&page=%d",
|
||||
c.baseURL,
|
||||
url.PathEscape(owner),
|
||||
url.PathEscape(repo),
|
||||
number,
|
||||
pageSize,
|
||||
page)
|
||||
body, err := c.doGet(ctx, reqURL)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("list reviews (page %d): %w", page, err)
|
||||
}
|
||||
var batch []Review
|
||||
if err := json.Unmarshal(body, &batch); err != nil {
|
||||
return nil, fmt.Errorf("parse reviews (page %d): %w", page, err)
|
||||
}
|
||||
all = append(all, batch...)
|
||||
if len(batch) < pageSize {
|
||||
break
|
||||
}
|
||||
}
|
||||
return all, nil
|
||||
}
|
||||
|
||||
// DeleteReview deletes a review by ID. The token must belong to the review author.
|
||||
func (c *Client) DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews/%d",
|
||||
c.baseURL,
|
||||
url.PathEscape(owner),
|
||||
url.PathEscape(repo),
|
||||
number,
|
||||
reviewID)
|
||||
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodDelete, reqURL, nil)
|
||||
if err != nil {
|
||||
return fmt.Errorf("create delete request: %w", err)
|
||||
}
|
||||
req.Header.Set("Authorization", "token "+c.token)
|
||||
|
||||
resp, err := c.http.Do(req)
|
||||
if err != nil {
|
||||
return fmt.Errorf("delete review: %w", err)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
|
||||
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
||||
respBody, _ := io.ReadAll(resp.Body)
|
||||
return fmt.Errorf("delete review failed (status %d): %s", resp.StatusCode, string(respBody))
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// TimelineEvent represents an entry from the issue timeline API.
|
||||
type TimelineEvent struct {
|
||||
ID int64 `json:"id"`
|
||||
Type string `json:"type"`
|
||||
Body string `json:"body"`
|
||||
User struct {
|
||||
Login string `json:"login"`
|
||||
} `json:"user"`
|
||||
}
|
||||
|
||||
// GetTimelineReviewCommentID finds the comment ID for a review body by
|
||||
// scanning the issue timeline for a review event containing the sentinel.
|
||||
func (c *Client) GetTimelineReviewCommentID(ctx context.Context, owner, repo string, number int, sentinel string) (int64, error) {
|
||||
const pageSize = 50
|
||||
for page := 1; ; page++ {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues/%d/timeline?limit=%d&page=%d",
|
||||
c.baseURL,
|
||||
url.PathEscape(owner),
|
||||
url.PathEscape(repo),
|
||||
number,
|
||||
pageSize,
|
||||
page)
|
||||
body, err := c.doGet(ctx, reqURL)
|
||||
if err != nil {
|
||||
return 0, fmt.Errorf("get timeline (page %d): %w", page, err)
|
||||
}
|
||||
var events []TimelineEvent
|
||||
if err := json.Unmarshal(body, &events); err != nil {
|
||||
return 0, fmt.Errorf("parse timeline (page %d): %w", page, err)
|
||||
}
|
||||
for _, ev := range events {
|
||||
if ev.Type == "review" && strings.Contains(ev.Body, sentinel) {
|
||||
return ev.ID, nil
|
||||
}
|
||||
}
|
||||
if len(events) < pageSize {
|
||||
break
|
||||
}
|
||||
}
|
||||
return 0, fmt.Errorf("no timeline event found with sentinel")
|
||||
}
|
||||
|
||||
// GetTimelineReviewCommentIDForReview finds the timeline comment ID for a
|
||||
// specific review by matching its body content in the timeline.
|
||||
func (c *Client) GetTimelineReviewCommentIDForReview(ctx context.Context, owner, repo string, number int, reviewID int64) (int64, error) {
|
||||
// Use the reviews API to get the review body, then find in timeline
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews/%d",
|
||||
c.baseURL,
|
||||
url.PathEscape(owner),
|
||||
url.PathEscape(repo),
|
||||
number,
|
||||
reviewID)
|
||||
body, err := c.doGet(ctx, reqURL)
|
||||
if err != nil {
|
||||
return 0, fmt.Errorf("get review %d: %w", reviewID, err)
|
||||
}
|
||||
var review struct {
|
||||
Body string `json:"body"`
|
||||
User struct {
|
||||
Login string `json:"login"`
|
||||
} `json:"user"`
|
||||
}
|
||||
if err := json.Unmarshal(body, &review); err != nil {
|
||||
return 0, fmt.Errorf("parse review %d: %w", reviewID, err)
|
||||
}
|
||||
if review.Body == "" {
|
||||
return 0, fmt.Errorf("review %d has empty body", reviewID)
|
||||
}
|
||||
|
||||
// Use a prefix for matching (handles minor trailing whitespace differences)
|
||||
matchPrefix := review.Body
|
||||
if len(matchPrefix) > 200 {
|
||||
matchPrefix = matchPrefix[:200]
|
||||
}
|
||||
|
||||
const pageSize = 50
|
||||
for page := 1; ; page++ {
|
||||
timelineURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues/%d/timeline?limit=%d&page=%d",
|
||||
c.baseURL,
|
||||
url.PathEscape(owner),
|
||||
url.PathEscape(repo),
|
||||
number,
|
||||
pageSize,
|
||||
page)
|
||||
tlBody, err := c.doGet(ctx, timelineURL)
|
||||
if err != nil {
|
||||
return 0, fmt.Errorf("get timeline (page %d): %w", page, err)
|
||||
}
|
||||
var events []TimelineEvent
|
||||
if err := json.Unmarshal(tlBody, &events); err != nil {
|
||||
return 0, fmt.Errorf("parse timeline (page %d): %w", page, err)
|
||||
}
|
||||
for _, ev := range events {
|
||||
if ev.Type == "review" && ev.User.Login == review.User.Login && strings.HasPrefix(ev.Body, matchPrefix) {
|
||||
return ev.ID, nil
|
||||
}
|
||||
}
|
||||
if len(events) < pageSize {
|
||||
break
|
||||
}
|
||||
}
|
||||
return 0, fmt.Errorf("no timeline event found for review %d", reviewID)
|
||||
}
|
||||
|
||||
// EditComment updates the body of an issue/review comment.
|
||||
func (c *Client) EditComment(ctx context.Context, owner, repo string, commentID int64, newBody string) error {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/issues/comments/%d",
|
||||
c.baseURL,
|
||||
url.PathEscape(owner),
|
||||
url.PathEscape(repo),
|
||||
commentID)
|
||||
|
||||
payload := struct {
|
||||
Body string `json:"body"`
|
||||
}{Body: newBody}
|
||||
data, err := json.Marshal(payload)
|
||||
if err != nil {
|
||||
return fmt.Errorf("marshal edit payload: %w", err)
|
||||
}
|
||||
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodPatch, reqURL, bytes.NewReader(data))
|
||||
if err != nil {
|
||||
return fmt.Errorf("create edit request: %w", err)
|
||||
}
|
||||
req.Header.Set("Authorization", "token "+c.token)
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
|
||||
resp, err := c.http.Do(req)
|
||||
if err != nil {
|
||||
return fmt.Errorf("edit comment: %w", err)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
|
||||
if resp.StatusCode != http.StatusOK {
|
||||
body, _ := io.ReadAll(resp.Body)
|
||||
return fmt.Errorf("edit comment failed (status %d): %s", resp.StatusCode, body)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// GetAuthenticatedUser returns the login of the user authenticated by the token.
|
||||
func (c *Client) GetAuthenticatedUser(ctx context.Context) (string, error) {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/user", c.baseURL)
|
||||
body, err := c.doGet(ctx, reqURL)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("get authenticated user: %w", err)
|
||||
}
|
||||
var result struct {
|
||||
Login string `json:"login"`
|
||||
}
|
||||
if err := json.Unmarshal(body, &result); err != nil {
|
||||
return "", fmt.Errorf("parse user response: %w", err)
|
||||
}
|
||||
return result.Login, nil
|
||||
}
|
||||
|
||||
// RequestReviewer adds the given user as a requested reviewer on a pull request.
|
||||
// This is idempotent — requesting an already-requested reviewer is a no-op.
|
||||
func (c *Client) RequestReviewer(ctx context.Context, owner, repo string, number int, reviewer string) error {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/requested_reviewers",
|
||||
c.baseURL,
|
||||
url.PathEscape(owner),
|
||||
url.PathEscape(repo),
|
||||
number)
|
||||
|
||||
payload := struct {
|
||||
Reviewers []string `json:"reviewers"`
|
||||
}{Reviewers: []string{reviewer}}
|
||||
data, err := json.Marshal(payload)
|
||||
if err != nil {
|
||||
return fmt.Errorf("marshal reviewer request: %w", err)
|
||||
}
|
||||
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, bytes.NewReader(data))
|
||||
if err != nil {
|
||||
return fmt.Errorf("create reviewer request: %w", err)
|
||||
}
|
||||
req.Header.Set("Authorization", "token "+c.token)
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
|
||||
resp, err := c.http.Do(req)
|
||||
if err != nil {
|
||||
return fmt.Errorf("request reviewer: %w", err)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
|
||||
if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent {
|
||||
body, _ := io.ReadAll(io.LimitReader(resp.Body, 256))
|
||||
return fmt.Errorf("request reviewer failed (status %d): %s", resp.StatusCode, body)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// ListReviewComments returns the inline comments attached to a specific review.
|
||||
// Paginates through all pages.
|
||||
func (c *Client) ListReviewComments(ctx context.Context, owner, repo string, prNumber int, reviewID int64) ([]ReviewComment, error) {
|
||||
const pageSize = 50
|
||||
var all []ReviewComment
|
||||
for page := 1; ; page++ {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews/%d/comments?limit=%d&page=%d",
|
||||
c.baseURL,
|
||||
url.PathEscape(owner),
|
||||
url.PathEscape(repo),
|
||||
prNumber,
|
||||
reviewID,
|
||||
pageSize,
|
||||
page)
|
||||
body, err := c.doGet(ctx, reqURL)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("list review comments (page %d): %w", page, err)
|
||||
}
|
||||
var batch []ReviewComment
|
||||
if err := json.Unmarshal(body, &batch); err != nil {
|
||||
return nil, fmt.Errorf("parse review comments (page %d): %w", page, err)
|
||||
}
|
||||
all = append(all, batch...)
|
||||
if len(batch) < pageSize {
|
||||
break
|
||||
}
|
||||
}
|
||||
return all, nil
|
||||
}
|
||||
|
||||
// ResolveComment marks an inline review comment as resolved.
|
||||
func (c *Client) ResolveComment(ctx context.Context, owner, repo string, commentID int64) error {
|
||||
reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/comments/%d/resolve",
|
||||
c.baseURL,
|
||||
url.PathEscape(owner),
|
||||
url.PathEscape(repo),
|
||||
commentID)
|
||||
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodPost, reqURL, nil)
|
||||
if err != nil {
|
||||
return fmt.Errorf("create resolve request: %w", err)
|
||||
}
|
||||
req.Header.Set("Authorization", "token "+c.token)
|
||||
|
||||
resp, err := c.http.Do(req)
|
||||
if err != nil {
|
||||
return fmt.Errorf("resolve comment: %w", err)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
|
||||
if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated && resp.StatusCode != http.StatusNoContent {
|
||||
body, _ := io.ReadAll(io.LimitReader(resp.Body, 256))
|
||||
return fmt.Errorf("resolve comment failed (status %d): %s", resp.StatusCode, body)
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
+3
-428
@@ -3,12 +3,9 @@ package gitea
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
@@ -126,21 +123,15 @@ func TestPostReview(t *testing.T) {
|
||||
}
|
||||
|
||||
w.WriteHeader(http.StatusOK)
|
||||
w.Write([]byte(`{"id":100,"user":{"login":"review-bot"},"state":"APPROVED","stale":false}`))
|
||||
w.Write([]byte(`{}`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", nil)
|
||||
err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if review.ID != 100 {
|
||||
t.Errorf("expected review ID 100, got %d", review.ID)
|
||||
}
|
||||
if review.User.Login != "review-bot" {
|
||||
t.Errorf("expected user login %q, got %q", "review-bot", review.User.Login)
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetPullRequest_Non200(t *testing.T) {
|
||||
@@ -178,7 +169,7 @@ func TestPostReview_Non200(t *testing.T) {
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", nil)
|
||||
err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test")
|
||||
if err == nil {
|
||||
t.Fatal("expected error for 403, got nil")
|
||||
}
|
||||
@@ -327,419 +318,3 @@ func TestEscapePath(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestListReviews(t *testing.T) {
|
||||
pageCount := 0
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.URL.Path != "/api/v1/repos/owner/repo/pulls/5/reviews" {
|
||||
t.Errorf("unexpected path: %s", r.URL.Path)
|
||||
}
|
||||
if r.URL.Query().Get("limit") != "50" {
|
||||
t.Errorf("expected limit=50, got %s", r.URL.Query().Get("limit"))
|
||||
}
|
||||
pageCount++
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
// Return 2 results (less than page size) to signal end
|
||||
w.Write([]byte(`[{"id":10,"user":{"login":"bot-a"},"state":"APPROVED","stale":false},{"id":11,"user":{"login":"bot-b"},"state":"REQUEST_CHANGES","stale":true}]`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
reviews, err := client.ListReviews(context.Background(), "owner", "repo", 5)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if len(reviews) != 2 {
|
||||
t.Fatalf("expected 2 reviews, got %d", len(reviews))
|
||||
}
|
||||
if reviews[0].User.Login != "bot-a" {
|
||||
t.Errorf("expected bot-a, got %s", reviews[0].User.Login)
|
||||
}
|
||||
if pageCount != 1 {
|
||||
t.Errorf("expected 1 page fetch (results < page size), got %d", pageCount)
|
||||
}
|
||||
}
|
||||
|
||||
func TestListReviews_Pagination(t *testing.T) {
|
||||
pageCount := 0
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
pageCount++
|
||||
page := r.URL.Query().Get("page")
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
if page == "1" {
|
||||
// Return exactly 50 items to trigger next page fetch
|
||||
items := "["
|
||||
for i := 0; i < 50; i++ {
|
||||
if i > 0 {
|
||||
items += ","
|
||||
}
|
||||
items += fmt.Sprintf(`{"id":%d,"user":{"login":"bot"},"state":"APPROVED","stale":false}`, i+1)
|
||||
}
|
||||
items += "]"
|
||||
w.Write([]byte(items))
|
||||
} else {
|
||||
// Page 2: return fewer than 50 to signal end
|
||||
w.Write([]byte(`[{"id":51,"user":{"login":"bot"},"state":"APPROVED","stale":false}]`))
|
||||
}
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
reviews, err := client.ListReviews(context.Background(), "owner", "repo", 5)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if len(reviews) != 51 {
|
||||
t.Fatalf("expected 51 reviews across 2 pages, got %d", len(reviews))
|
||||
}
|
||||
if pageCount != 2 {
|
||||
t.Errorf("expected 2 page fetches, got %d", pageCount)
|
||||
}
|
||||
}
|
||||
|
||||
func TestDeleteReview(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.URL.Path != "/api/v1/repos/owner/repo/pulls/5/reviews/10" {
|
||||
t.Errorf("unexpected path: %s", r.URL.Path)
|
||||
}
|
||||
if r.Method != "DELETE" {
|
||||
t.Errorf("expected DELETE, got %s", r.Method)
|
||||
}
|
||||
w.WriteHeader(http.StatusNoContent)
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
err := client.DeleteReview(context.Background(), "owner", "repo", 5, 10)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestDeleteReview_Forbidden(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(http.StatusForbidden)
|
||||
w.Write([]byte(`{"message":"forbidden"}`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
err := client.DeleteReview(context.Background(), "owner", "repo", 5, 10)
|
||||
if err == nil {
|
||||
t.Fatal("expected error for 403, got nil")
|
||||
}
|
||||
}
|
||||
|
||||
func TestEditComment(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.Method != http.MethodPatch {
|
||||
t.Errorf("expected PATCH, got %s", r.Method)
|
||||
}
|
||||
if r.URL.Path != "/api/v1/repos/owner/repo/issues/comments/42" {
|
||||
t.Errorf("unexpected path: %s", r.URL.Path)
|
||||
}
|
||||
|
||||
var payload struct {
|
||||
Body string `json:"body"`
|
||||
}
|
||||
json.NewDecoder(r.Body).Decode(&payload)
|
||||
if payload.Body != "updated body" {
|
||||
t.Errorf("unexpected body: %s", payload.Body)
|
||||
}
|
||||
|
||||
w.WriteHeader(http.StatusOK)
|
||||
w.Write([]byte(`{"id": 42, "body": "updated body"}`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
err := client.EditComment(context.Background(), "owner", "repo", 42, "updated body")
|
||||
if err != nil {
|
||||
t.Fatalf("EditComment() error = %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestEditComment_Forbidden(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(http.StatusForbidden)
|
||||
w.Write([]byte(`{"message": "not allowed"}`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
err := client.EditComment(context.Background(), "owner", "repo", 42, "new body")
|
||||
if err == nil {
|
||||
t.Fatal("expected error for 403 response")
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetTimelineReviewCommentID(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.URL.Path != "/api/v1/repos/owner/repo/issues/5/timeline" {
|
||||
t.Errorf("unexpected path: %s", r.URL.Path)
|
||||
}
|
||||
w.Write([]byte(`[
|
||||
{"id": 100, "type": "comment", "body": "random"},
|
||||
{"id": 200, "type": "review", "body": "other review <!-- review-bot:gpt -->"},
|
||||
{"id": 300, "type": "review", "body": "our review <!-- review-bot:sonnet -->"}
|
||||
]`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
id, err := client.GetTimelineReviewCommentID(context.Background(), "owner", "repo", 5, "<!-- review-bot:sonnet -->")
|
||||
if err != nil {
|
||||
t.Fatalf("GetTimelineReviewCommentID() error = %v", err)
|
||||
}
|
||||
if id != 300 {
|
||||
t.Errorf("got id=%d, want 300", id)
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetTimelineReviewCommentID_NotFound(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.Write([]byte(`[{"id": 100, "type": "review", "body": "no match"}]`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
_, err := client.GetTimelineReviewCommentID(context.Background(), "owner", "repo", 5, "<!-- review-bot:sonnet -->")
|
||||
if err == nil {
|
||||
t.Fatal("expected error when sentinel not found")
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetAllFilesInPath_404FallsBackToFile(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
switch r.URL.Path {
|
||||
case "/api/v1/repos/owner/repo/contents/README.md":
|
||||
// Contents API returns 404 for files (not a directory)
|
||||
w.WriteHeader(http.StatusNotFound)
|
||||
w.Write([]byte(`{"message":"not found"}`))
|
||||
case "/api/v1/repos/owner/repo/raw/README.md":
|
||||
w.Write([]byte("# Hello\n"))
|
||||
default:
|
||||
w.WriteHeader(http.StatusNotFound)
|
||||
w.Write([]byte(`{"message":"not found"}`))
|
||||
}
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
files, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "README.md")
|
||||
if err != nil {
|
||||
t.Fatalf("expected fallback to file on 404, got error: %v", err)
|
||||
}
|
||||
if len(files) != 1 {
|
||||
t.Fatalf("expected 1 file, got %d", len(files))
|
||||
}
|
||||
if files["README.md"] != "# Hello\n" {
|
||||
t.Errorf("unexpected content: %q", files["README.md"])
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetAllFilesInPath_500Propagates(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
// Simulate a server error from ListContents
|
||||
w.WriteHeader(http.StatusInternalServerError)
|
||||
w.Write([]byte(`{"message":"internal server error"}`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
_, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "somepath")
|
||||
if err == nil {
|
||||
t.Fatal("expected error to propagate for 500, got nil")
|
||||
}
|
||||
// Should NOT fall back to file fetch — error should propagate
|
||||
var apiErr *APIError
|
||||
if !errors.As(err, &apiErr) {
|
||||
t.Fatalf("expected APIError in chain, got: %v", err)
|
||||
}
|
||||
if apiErr.StatusCode != http.StatusInternalServerError {
|
||||
t.Errorf("expected status 500, got %d", apiErr.StatusCode)
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetAllFilesInPath_403Propagates(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(http.StatusForbidden)
|
||||
w.Write([]byte(`{"message":"token has insufficient scope"}`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
_, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "private/stuff")
|
||||
if err == nil {
|
||||
t.Fatal("expected error to propagate for 403, got nil")
|
||||
}
|
||||
var apiErr *APIError
|
||||
if !errors.As(err, &apiErr) {
|
||||
t.Fatalf("expected APIError in chain, got: %v", err)
|
||||
}
|
||||
if apiErr.StatusCode != http.StatusForbidden {
|
||||
t.Errorf("expected status 403, got %d", apiErr.StatusCode)
|
||||
}
|
||||
}
|
||||
|
||||
func TestIsNotFound(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
err error
|
||||
want bool
|
||||
}{
|
||||
{"nil error", nil, false},
|
||||
{"non-API error", fmt.Errorf("network timeout"), false},
|
||||
{"404 APIError", &APIError{StatusCode: 404, Body: "not found"}, true},
|
||||
{"500 APIError", &APIError{StatusCode: 500, Body: "server error"}, false},
|
||||
{"wrapped 404", fmt.Errorf("list contents: %w", &APIError{StatusCode: 404, Body: "not found"}), true},
|
||||
{"wrapped 500", fmt.Errorf("list contents: %w", &APIError{StatusCode: 500, Body: "err"}), false},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
got := IsNotFound(tt.err)
|
||||
if got != tt.want {
|
||||
t.Errorf("IsNotFound(%v) = %v, want %v", tt.err, got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetAuthenticatedUser(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.URL.Path != "/api/v1/user" {
|
||||
t.Errorf("unexpected path: %s", r.URL.Path)
|
||||
http.NotFound(w, r)
|
||||
return
|
||||
}
|
||||
if r.Header.Get("Authorization") != "token test-token" {
|
||||
t.Error("missing or wrong auth header")
|
||||
}
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
fmt.Fprint(w, `{"login":"my-bot","id":42}`)
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
login, err := client.GetAuthenticatedUser(context.Background())
|
||||
if err != nil {
|
||||
t.Fatalf("GetAuthenticatedUser() error = %v", err)
|
||||
}
|
||||
if login != "my-bot" {
|
||||
t.Errorf("login = %q, want %q", login, "my-bot")
|
||||
}
|
||||
}
|
||||
|
||||
func TestRequestReviewer(t *testing.T) {
|
||||
var gotBody []byte
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.Method != http.MethodPost {
|
||||
t.Errorf("expected POST, got %s", r.Method)
|
||||
}
|
||||
expected := "/api/v1/repos/owner/repo/pulls/7/requested_reviewers"
|
||||
if r.URL.Path != expected {
|
||||
t.Errorf("path = %q, want %q", r.URL.Path, expected)
|
||||
}
|
||||
gotBody, _ = io.ReadAll(r.Body)
|
||||
w.WriteHeader(http.StatusCreated)
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
err := client.RequestReviewer(context.Background(), "owner", "repo", 7, "bot-user")
|
||||
if err != nil {
|
||||
t.Fatalf("RequestReviewer() error = %v", err)
|
||||
}
|
||||
if !strings.Contains(string(gotBody), `"bot-user"`) {
|
||||
t.Errorf("body = %s, want to contain bot-user", gotBody)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRequestReviewer_204(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(http.StatusNoContent)
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
err := client.RequestReviewer(context.Background(), "owner", "repo", 1, "user")
|
||||
if err != nil {
|
||||
t.Fatalf("RequestReviewer() should accept 204, got error = %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRequestReviewer_Error(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(http.StatusForbidden)
|
||||
fmt.Fprint(w, "no permission")
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
err := client.RequestReviewer(context.Background(), "owner", "repo", 1, "user")
|
||||
if err == nil {
|
||||
t.Fatal("expected error for 403 response")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "403") {
|
||||
t.Errorf("error should mention status code: %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestListReviewComments(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if !strings.Contains(r.URL.Path, "/pulls/1/reviews/42/comments") {
|
||||
t.Errorf("unexpected path: %s", r.URL.Path)
|
||||
}
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
fmt.Fprint(w, `[{"id":100,"path":"main.go","new_position":5,"body":"finding"},{"id":101,"path":"lib.go","new_position":10,"body":"another"}]`)
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
comments, err := client.ListReviewComments(context.Background(), "owner", "repo", 1, 42)
|
||||
if err != nil {
|
||||
t.Fatalf("ListReviewComments() error = %v", err)
|
||||
}
|
||||
if len(comments) != 2 {
|
||||
t.Fatalf("got %d comments, want 2", len(comments))
|
||||
}
|
||||
if comments[0].ID != 100 {
|
||||
t.Errorf("comments[0].ID = %d, want 100", comments[0].ID)
|
||||
}
|
||||
if comments[1].Path != "lib.go" {
|
||||
t.Errorf("comments[1].Path = %q, want %q", comments[1].Path, "lib.go")
|
||||
}
|
||||
}
|
||||
|
||||
func TestResolveComment(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.Method != http.MethodPost {
|
||||
t.Errorf("expected POST, got %s", r.Method)
|
||||
}
|
||||
if !strings.Contains(r.URL.Path, "/pulls/comments/99/resolve") {
|
||||
t.Errorf("unexpected path: %s", r.URL.Path)
|
||||
}
|
||||
w.WriteHeader(http.StatusOK)
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
err := client.ResolveComment(context.Background(), "owner", "repo", 99)
|
||||
if err != nil {
|
||||
t.Fatalf("ResolveComment() error = %v", err)
|
||||
}
|
||||
}
|
||||
|
||||
func TestResolveComment_Error(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(http.StatusNotFound)
|
||||
fmt.Fprint(w, "not found")
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
err := client.ResolveComment(context.Background(), "owner", "repo", 99)
|
||||
if err == nil {
|
||||
t.Fatal("expected error for 404 response")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,85 +0,0 @@
|
||||
package gitea
|
||||
|
||||
import (
|
||||
"strconv"
|
||||
"strings"
|
||||
)
|
||||
|
||||
// DiffLineRanges maps filenames to the set of new-file line numbers present in the diff.
|
||||
type DiffLineRanges struct {
|
||||
files map[string]map[int]bool
|
||||
}
|
||||
|
||||
// Contains reports whether the given file+line is within the diff hunks.
|
||||
func (d *DiffLineRanges) Contains(file string, line int) bool {
|
||||
if d == nil || d.files == nil {
|
||||
return false
|
||||
}
|
||||
lines, ok := d.files[file]
|
||||
if !ok {
|
||||
return false
|
||||
}
|
||||
return lines[line]
|
||||
}
|
||||
|
||||
// ParseDiffNewLines parses a unified diff and extracts the new-file line numbers
|
||||
// that appear in each hunk (both added and context lines).
|
||||
func ParseDiffNewLines(diff string) *DiffLineRanges {
|
||||
result := &DiffLineRanges{files: make(map[string]map[int]bool)}
|
||||
|
||||
var currentFile string
|
||||
var newLine int
|
||||
|
||||
for _, line := range strings.Split(diff, "\n") {
|
||||
// Track current file from +++ header
|
||||
if strings.HasPrefix(line, "+++ b/") {
|
||||
currentFile = strings.TrimPrefix(line, "+++ b/")
|
||||
if result.files[currentFile] == nil {
|
||||
result.files[currentFile] = make(map[int]bool)
|
||||
}
|
||||
continue
|
||||
}
|
||||
if strings.HasPrefix(line, "+++ /dev/null") {
|
||||
currentFile = ""
|
||||
continue
|
||||
}
|
||||
|
||||
// Parse hunk header: @@ -old,count +new,count @@ or @@ -old +new @@
|
||||
if strings.HasPrefix(line, "@@") && currentFile != "" {
|
||||
// Extract the +N part — handle both "+10,8" and "+1" forms
|
||||
parts := strings.Split(line, "+")
|
||||
if len(parts) >= 2 {
|
||||
// Take everything before comma or space
|
||||
numStr := parts[1]
|
||||
if idx := strings.IndexAny(numStr, ", "); idx != -1 {
|
||||
numStr = numStr[:idx]
|
||||
}
|
||||
n, err := strconv.Atoi(numStr)
|
||||
if err == nil {
|
||||
newLine = n
|
||||
}
|
||||
}
|
||||
continue
|
||||
}
|
||||
|
||||
if currentFile == "" {
|
||||
continue
|
||||
}
|
||||
|
||||
// Skip diff metadata lines
|
||||
if strings.HasPrefix(line, "\\") {
|
||||
continue
|
||||
}
|
||||
|
||||
// Count lines in hunk
|
||||
if strings.HasPrefix(line, "+") || strings.HasPrefix(line, " ") {
|
||||
result.files[currentFile][newLine] = true
|
||||
newLine++
|
||||
} else if strings.HasPrefix(line, "-") {
|
||||
// Removed lines don't advance new line counter
|
||||
continue
|
||||
}
|
||||
}
|
||||
|
||||
return result
|
||||
}
|
||||
@@ -1,115 +0,0 @@
|
||||
package gitea
|
||||
|
||||
import (
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestParseDiffLineRanges(t *testing.T) {
|
||||
diff := `diff --git a/main.go b/main.go
|
||||
index abc1234..def5678 100644
|
||||
--- a/main.go
|
||||
+++ b/main.go
|
||||
@@ -10,6 +10,8 @@ func main() {
|
||||
fmt.Println("hello")
|
||||
+ fmt.Println("new line 11")
|
||||
+ fmt.Println("new line 12")
|
||||
fmt.Println("existing")
|
||||
}
|
||||
@@ -30,4 +32,5 @@ func other() {
|
||||
return nil
|
||||
+ // added at line 33
|
||||
}
|
||||
diff --git a/util.go b/util.go
|
||||
new file mode 100644
|
||||
--- /dev/null
|
||||
+++ b/util.go
|
||||
@@ -0,0 +1,5 @@
|
||||
+package main
|
||||
+
|
||||
+func helper() string {
|
||||
+ return "hi"
|
||||
+}
|
||||
`
|
||||
|
||||
ranges := ParseDiffNewLines(diff)
|
||||
|
||||
// main.go should have lines 10-17 (first hunk) and 32-36 (second hunk)
|
||||
if !ranges.Contains("main.go", 11) {
|
||||
t.Error("expected main.go:11 to be in diff")
|
||||
}
|
||||
if !ranges.Contains("main.go", 12) {
|
||||
t.Error("expected main.go:12 to be in diff")
|
||||
}
|
||||
if !ranges.Contains("main.go", 10) {
|
||||
t.Error("expected main.go:10 to be in diff (context line)")
|
||||
}
|
||||
if !ranges.Contains("main.go", 33) {
|
||||
t.Error("expected main.go:33 to be in diff")
|
||||
}
|
||||
if ranges.Contains("main.go", 25) {
|
||||
t.Error("main.go:25 should NOT be in diff")
|
||||
}
|
||||
|
||||
// util.go is entirely new, lines 1-5
|
||||
if !ranges.Contains("util.go", 1) {
|
||||
t.Error("expected util.go:1 to be in diff")
|
||||
}
|
||||
if !ranges.Contains("util.go", 5) {
|
||||
t.Error("expected util.go:5 to be in diff")
|
||||
}
|
||||
if ranges.Contains("util.go", 6) {
|
||||
t.Error("util.go:6 should NOT be in diff")
|
||||
}
|
||||
|
||||
// Unknown file
|
||||
if ranges.Contains("unknown.go", 1) {
|
||||
t.Error("unknown.go should not be in diff")
|
||||
}
|
||||
}
|
||||
|
||||
func TestParseDiffNewLines_Empty(t *testing.T) {
|
||||
ranges := ParseDiffNewLines("")
|
||||
if ranges.Contains("any.go", 1) {
|
||||
t.Error("empty diff should contain nothing")
|
||||
}
|
||||
}
|
||||
|
||||
func TestParseDiffNewLines_NoCommaHunk(t *testing.T) {
|
||||
// Single-line hunks omit the comma: @@ -1 +1 @@
|
||||
diff := `diff --git a/single.go b/single.go
|
||||
--- a/single.go
|
||||
+++ b/single.go
|
||||
@@ -1 +1 @@
|
||||
-old line
|
||||
+new line
|
||||
`
|
||||
ranges := ParseDiffNewLines(diff)
|
||||
if !ranges.Contains("single.go", 1) {
|
||||
t.Error("expected single.go:1 to be in diff (no-comma hunk)")
|
||||
}
|
||||
if ranges.Contains("single.go", 2) {
|
||||
t.Error("single.go:2 should NOT be in diff")
|
||||
}
|
||||
}
|
||||
|
||||
func TestParseDiffNewLines_NoNewlineMarker(t *testing.T) {
|
||||
// "\ No newline at end of file" should not advance line counter
|
||||
diff := `diff --git a/noeof.go b/noeof.go
|
||||
--- a/noeof.go
|
||||
+++ b/noeof.go
|
||||
@@ -1,2 +1,2 @@
|
||||
+line one
|
||||
+line two
|
||||
\ No newline at end of file
|
||||
`
|
||||
ranges := ParseDiffNewLines(diff)
|
||||
if !ranges.Contains("noeof.go", 1) {
|
||||
t.Error("expected noeof.go:1")
|
||||
}
|
||||
if !ranges.Contains("noeof.go", 2) {
|
||||
t.Error("expected noeof.go:2")
|
||||
}
|
||||
if ranges.Contains("noeof.go", 3) {
|
||||
t.Error("noeof.go:3 should NOT be in diff (no-newline marker)")
|
||||
}
|
||||
}
|
||||
@@ -1,88 +0,0 @@
|
||||
package gitea
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestPostReview_WithComments(t *testing.T) {
|
||||
var gotPayload struct {
|
||||
Body string `json:"body"`
|
||||
Event string `json:"event"`
|
||||
Comments []struct {
|
||||
Path string `json:"path"`
|
||||
NewPosition int64 `json:"new_position"`
|
||||
Body string `json:"body"`
|
||||
} `json:"comments"`
|
||||
}
|
||||
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
json.NewDecoder(r.Body).Decode(&gotPayload)
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.WriteHeader(200)
|
||||
json.NewEncoder(w).Encode(map[string]any{
|
||||
"id": 99,
|
||||
"body": gotPayload.Body,
|
||||
"user": map[string]any{"login": "bot"},
|
||||
})
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
comments := []ReviewComment{
|
||||
{Path: "main.go", NewPosition: 42, Body: "[MAJOR] Something bad"},
|
||||
{Path: "util.go", NewPosition: 10, Body: "[MINOR] Style issue"},
|
||||
}
|
||||
|
||||
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "REQUEST_CHANGES", "summary", comments)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
|
||||
if len(gotPayload.Comments) != 2 {
|
||||
t.Fatalf("expected 2 comments, got %d", len(gotPayload.Comments))
|
||||
}
|
||||
if gotPayload.Comments[0].Path != "main.go" {
|
||||
t.Errorf("expected path main.go, got %s", gotPayload.Comments[0].Path)
|
||||
}
|
||||
if gotPayload.Comments[0].NewPosition != 42 {
|
||||
t.Errorf("expected new_position 42, got %d", gotPayload.Comments[0].NewPosition)
|
||||
}
|
||||
if gotPayload.Comments[1].Body != "[MINOR] Style issue" {
|
||||
t.Errorf("unexpected body: %s", gotPayload.Comments[1].Body)
|
||||
}
|
||||
}
|
||||
|
||||
func TestPostReview_NilComments(t *testing.T) {
|
||||
var gotPayload map[string]any
|
||||
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
json.NewDecoder(r.Body).Decode(&gotPayload)
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.WriteHeader(200)
|
||||
json.NewEncoder(w).Encode(map[string]any{
|
||||
"id": 100,
|
||||
"body": "test",
|
||||
"user": map[string]any{"login": "bot"},
|
||||
})
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "all good", nil)
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
|
||||
// With nil comments, the field should be omitted (omitempty)
|
||||
comments, ok := gotPayload["comments"]
|
||||
if ok && comments != nil {
|
||||
arr, isArr := comments.([]any)
|
||||
if isArr && len(arr) > 0 {
|
||||
t.Error("expected no comments in payload when nil passed")
|
||||
}
|
||||
}
|
||||
}
|
||||
+24
-192
@@ -1,6 +1,4 @@
|
||||
// Package llm provides clients for LLM chat completion APIs.
|
||||
//
|
||||
// Supports OpenAI-compatible (default) and Anthropic Messages API providers.
|
||||
// Package llm provides a client for OpenAI-compatible chat completion APIs.
|
||||
package llm
|
||||
|
||||
import (
|
||||
@@ -14,37 +12,24 @@ import (
|
||||
"time"
|
||||
)
|
||||
|
||||
// Provider identifies which API format to use.
|
||||
type Provider string
|
||||
|
||||
const (
|
||||
// ProviderOpenAI uses the OpenAI-compatible chat/completions endpoint.
|
||||
ProviderOpenAI Provider = "openai"
|
||||
// ProviderAnthropic uses the Anthropic Messages API endpoint.
|
||||
ProviderAnthropic Provider = "anthropic"
|
||||
)
|
||||
|
||||
// Client calls an LLM chat completion API.
|
||||
// Client calls an OpenAI-compatible chat completion API.
|
||||
// A Client is safe for concurrent use by multiple goroutines after construction.
|
||||
// WithTimeout, WithTemperature, and WithProvider must be called during setup,
|
||||
// before concurrent use.
|
||||
// WithTimeout and WithTemperature must be called during setup, before concurrent use.
|
||||
type Client struct {
|
||||
baseURL string
|
||||
apiKey string
|
||||
model string
|
||||
temperature float64
|
||||
provider Provider
|
||||
http *http.Client
|
||||
}
|
||||
|
||||
// NewClient creates a new LLM client. Default provider is OpenAI-compatible.
|
||||
// NewClient creates a new LLM client.
|
||||
func NewClient(baseURL, apiKey, model string) *Client {
|
||||
return &Client{
|
||||
baseURL: strings.TrimRight(baseURL, "/"),
|
||||
apiKey: apiKey,
|
||||
model: model,
|
||||
provider: ProviderOpenAI,
|
||||
http: &http.Client{Timeout: 5 * time.Minute},
|
||||
baseURL: strings.TrimRight(baseURL, "/"),
|
||||
apiKey: apiKey,
|
||||
model: model,
|
||||
http: &http.Client{Timeout: 5 * time.Minute},
|
||||
}
|
||||
}
|
||||
|
||||
@@ -60,79 +45,20 @@ func (c *Client) WithTemperature(t float64) *Client {
|
||||
return c
|
||||
}
|
||||
|
||||
// WithProvider sets the API provider format (openai or anthropic).
|
||||
func (c *Client) WithProvider(p Provider) *Client {
|
||||
c.provider = p
|
||||
return c
|
||||
}
|
||||
|
||||
// Message represents a chat message.
|
||||
type Message struct {
|
||||
Role string `json:"role"`
|
||||
Content string `json:"content"`
|
||||
}
|
||||
|
||||
// Complete sends a chat completion request and returns the assistant's response content.
|
||||
// The first message with role "system" is treated as the system prompt.
|
||||
func (c *Client) Complete(ctx context.Context, messages []Message) (string, error) {
|
||||
var result string
|
||||
var err error
|
||||
|
||||
for attempt := 0; attempt < 2; attempt++ {
|
||||
switch c.provider {
|
||||
case ProviderAnthropic:
|
||||
result, err = c.completeAnthropic(ctx, messages)
|
||||
default:
|
||||
result, err = c.completeOpenAI(ctx, messages)
|
||||
}
|
||||
|
||||
if err == nil {
|
||||
return result, nil
|
||||
}
|
||||
|
||||
// Only retry on response body read errors (transient network issues).
|
||||
// Do not retry on context cancellation, status errors, or parse errors
|
||||
// that indicate a structural API problem.
|
||||
if !isRetryableError(err) {
|
||||
return "", err
|
||||
}
|
||||
|
||||
if attempt == 0 && ctx.Err() == nil {
|
||||
// Brief pause before retry to allow transient issues to resolve.
|
||||
time.Sleep(500 * time.Millisecond)
|
||||
}
|
||||
}
|
||||
|
||||
return "", err
|
||||
}
|
||||
|
||||
// isRetryableError returns true for transient errors worth retrying.
|
||||
func isRetryableError(err error) bool {
|
||||
if err == nil {
|
||||
return false
|
||||
}
|
||||
s := err.Error()
|
||||
// Body read failures (connection reset, truncation)
|
||||
if strings.Contains(s, "read response") {
|
||||
return true
|
||||
}
|
||||
// Unexpected body length (our content-length validation)
|
||||
if strings.Contains(s, "body length mismatch") {
|
||||
return true
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// --- OpenAI-compatible implementation ---
|
||||
|
||||
// ChatRequest is the OpenAI request payload.
|
||||
// ChatRequest is the request payload.
|
||||
type ChatRequest struct {
|
||||
Model string `json:"model"`
|
||||
Messages []Message `json:"messages"`
|
||||
Temperature float64 `json:"temperature,omitempty"`
|
||||
}
|
||||
|
||||
// ChatResponse is the OpenAI response.
|
||||
// ChatResponse is the response from the API.
|
||||
type ChatResponse struct {
|
||||
Choices []struct {
|
||||
Message struct {
|
||||
@@ -141,7 +67,8 @@ type ChatResponse struct {
|
||||
} `json:"choices"`
|
||||
}
|
||||
|
||||
func (c *Client) completeOpenAI(ctx context.Context, messages []Message) (string, error) {
|
||||
// Complete sends a chat completion request and returns the assistant's response content.
|
||||
func (c *Client) Complete(ctx context.Context, messages []Message) (string, error) {
|
||||
reqBody := ChatRequest{
|
||||
Model: c.model,
|
||||
Temperature: c.temperature,
|
||||
@@ -154,132 +81,37 @@ func (c *Client) completeOpenAI(ctx context.Context, messages []Message) (string
|
||||
}
|
||||
|
||||
url := c.baseURL + "/chat/completions"
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(data))
|
||||
req, err := http.NewRequestWithContext(ctx, "POST", url, bytes.NewReader(data))
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("create request: %w", err)
|
||||
}
|
||||
req.Header.Set("Authorization", "Bearer "+c.apiKey)
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
|
||||
return c.doRequest(req, func(body []byte) (string, error) {
|
||||
var resp ChatResponse
|
||||
if err := json.Unmarshal(body, &resp); err != nil {
|
||||
return "", fmt.Errorf("parse response: %w", err)
|
||||
}
|
||||
if len(resp.Choices) == 0 {
|
||||
return "", fmt.Errorf("no choices in LLM response")
|
||||
}
|
||||
return resp.Choices[0].Message.Content, nil
|
||||
})
|
||||
}
|
||||
|
||||
// --- Anthropic Messages API implementation ---
|
||||
|
||||
type anthropicRequest struct {
|
||||
Model string `json:"model"`
|
||||
MaxTokens int `json:"max_tokens"`
|
||||
System string `json:"system,omitempty"`
|
||||
Messages []anthropicMsg `json:"messages"`
|
||||
Temperature float64 `json:"temperature,omitempty"`
|
||||
}
|
||||
|
||||
type anthropicMsg struct {
|
||||
Role string `json:"role"`
|
||||
Content string `json:"content"`
|
||||
}
|
||||
|
||||
type anthropicResponse struct {
|
||||
Content []struct {
|
||||
Type string `json:"type"`
|
||||
Text string `json:"text"`
|
||||
} `json:"content"`
|
||||
}
|
||||
|
||||
func (c *Client) completeAnthropic(ctx context.Context, messages []Message) (string, error) {
|
||||
// Extract system message (first message with role "system")
|
||||
var system string
|
||||
var userMessages []anthropicMsg
|
||||
for _, m := range messages {
|
||||
if m.Role == "system" {
|
||||
system = m.Content
|
||||
} else {
|
||||
userMessages = append(userMessages, anthropicMsg{
|
||||
Role: m.Role,
|
||||
Content: m.Content,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
reqBody := anthropicRequest{
|
||||
Model: c.model,
|
||||
MaxTokens: 8192,
|
||||
System: system,
|
||||
Messages: userMessages,
|
||||
}
|
||||
if c.temperature > 0 {
|
||||
reqBody.Temperature = c.temperature
|
||||
}
|
||||
|
||||
data, err := json.Marshal(reqBody)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("marshal request: %w", err)
|
||||
}
|
||||
|
||||
url := c.baseURL + "/messages"
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(data))
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("create request: %w", err)
|
||||
}
|
||||
req.Header.Set("x-api-key", c.apiKey)
|
||||
req.Header.Set("anthropic-version", "2023-06-01")
|
||||
req.Header.Set("Content-Type", "application/json")
|
||||
|
||||
return c.doRequest(req, func(body []byte) (string, error) {
|
||||
var resp anthropicResponse
|
||||
if err := json.Unmarshal(body, &resp); err != nil {
|
||||
return "", fmt.Errorf("parse response: %w", err)
|
||||
}
|
||||
if len(resp.Content) == 0 {
|
||||
return "", fmt.Errorf("no content in Anthropic response")
|
||||
}
|
||||
// Concatenate all text blocks
|
||||
var sb strings.Builder
|
||||
for _, block := range resp.Content {
|
||||
if block.Type == "text" {
|
||||
sb.WriteString(block.Text)
|
||||
}
|
||||
}
|
||||
result := sb.String()
|
||||
if result == "" {
|
||||
return "", fmt.Errorf("no text content in Anthropic response")
|
||||
}
|
||||
return result, nil
|
||||
})
|
||||
}
|
||||
|
||||
// --- Shared HTTP execution ---
|
||||
|
||||
func (c *Client) doRequest(req *http.Request, parse func([]byte) (string, error)) (string, error) {
|
||||
resp, err := c.http.Do(req)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("LLM request: %w", err)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
|
||||
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
||||
body, _ := io.ReadAll(resp.Body)
|
||||
return "", fmt.Errorf("LLM API error (status %d): %s", resp.StatusCode, string(body))
|
||||
}
|
||||
|
||||
body, err := io.ReadAll(resp.Body)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("read response: %w", err)
|
||||
}
|
||||
|
||||
// Validate body length against Content-Length header when present.
|
||||
// A mismatch indicates the response was truncated in transit.
|
||||
if cl := resp.ContentLength; cl > 0 && int64(len(body)) < cl {
|
||||
return "", fmt.Errorf("body length mismatch: Content-Length=%d, received=%d", cl, len(body))
|
||||
var chatResp ChatResponse
|
||||
if err := json.Unmarshal(body, &chatResp); err != nil {
|
||||
return "", fmt.Errorf("parse response: %w", err)
|
||||
}
|
||||
|
||||
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
||||
return "", fmt.Errorf("LLM API error (status %d): %s", resp.StatusCode, string(body))
|
||||
if len(chatResp.Choices) == 0 {
|
||||
return "", fmt.Errorf("no choices in LLM response")
|
||||
}
|
||||
|
||||
return parse(body)
|
||||
return chatResp.Choices[0].Message.Content, nil
|
||||
}
|
||||
|
||||
@@ -3,7 +3,6 @@ package llm
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
@@ -209,218 +208,3 @@ func TestWithTimeout(t *testing.T) {
|
||||
t.Error("expected timeout error with 50ms timeout and 200ms server delay")
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
func TestComplete_Anthropic_Success(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
if r.URL.Path != "/messages" {
|
||||
t.Errorf("unexpected path: %s", r.URL.Path)
|
||||
}
|
||||
if r.Header.Get("x-api-key") != "test-key" {
|
||||
t.Errorf("expected x-api-key header, got %q", r.Header.Get("x-api-key"))
|
||||
}
|
||||
if r.Header.Get("anthropic-version") != "2023-06-01" {
|
||||
t.Errorf("expected anthropic-version header, got %q", r.Header.Get("anthropic-version"))
|
||||
}
|
||||
|
||||
var req map[string]interface{}
|
||||
json.NewDecoder(r.Body).Decode(&req)
|
||||
|
||||
if req["system"] != "You are helpful" {
|
||||
t.Errorf("expected system prompt, got %v", req["system"])
|
||||
}
|
||||
msgs := req["messages"].([]interface{})
|
||||
if len(msgs) != 1 {
|
||||
t.Errorf("expected 1 user message, got %d", len(msgs))
|
||||
}
|
||||
if req["max_tokens"] != float64(8192) {
|
||||
t.Errorf("expected max_tokens 8192, got %v", req["max_tokens"])
|
||||
}
|
||||
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.Write([]byte(`{"content":[{"type":"text","text":"Hello from Claude!"}]}`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-key", "claude-sonnet").WithProvider(ProviderAnthropic)
|
||||
got, err := client.Complete(context.Background(), []Message{
|
||||
{Role: "system", Content: "You are helpful"},
|
||||
{Role: "user", Content: "Hi"},
|
||||
})
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if got != "Hello from Claude!" {
|
||||
t.Errorf("expected %q, got %q", "Hello from Claude!", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestComplete_Anthropic_NoContent(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.Write([]byte(`{"content":[]}`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-key", "claude-sonnet").WithProvider(ProviderAnthropic)
|
||||
_, err := client.Complete(context.Background(), []Message{{Role: "user", Content: "Hi"}})
|
||||
if err == nil {
|
||||
t.Fatal("expected error for empty content, got nil")
|
||||
}
|
||||
}
|
||||
|
||||
func TestComplete_Anthropic_APIError(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(http.StatusBadRequest)
|
||||
w.Write([]byte(`{"error":{"message":"invalid request"}}`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-key", "claude-sonnet").WithProvider(ProviderAnthropic)
|
||||
_, err := client.Complete(context.Background(), []Message{{Role: "user", Content: "Hi"}})
|
||||
if err == nil {
|
||||
t.Fatal("expected error for 400, got nil")
|
||||
}
|
||||
}
|
||||
|
||||
func TestWithProvider(t *testing.T) {
|
||||
client := NewClient("http://example.com", "key", "model")
|
||||
if client.provider != ProviderOpenAI {
|
||||
t.Errorf("expected default provider openai, got %s", client.provider)
|
||||
}
|
||||
result := client.WithProvider(ProviderAnthropic)
|
||||
if result != client {
|
||||
t.Error("WithProvider should return the same client for chaining")
|
||||
}
|
||||
if client.provider != ProviderAnthropic {
|
||||
t.Errorf("expected provider anthropic, got %s", client.provider)
|
||||
}
|
||||
}
|
||||
|
||||
func TestComplete_RetryOnBodyReadError(t *testing.T) {
|
||||
attempts := 0
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
attempts++
|
||||
if attempts == 1 {
|
||||
// First attempt: send headers then close connection abruptly
|
||||
// Simulate by writing partial response and flushing with wrong Content-Length
|
||||
w.Header().Set("Content-Length", "1000")
|
||||
w.WriteHeader(http.StatusOK)
|
||||
w.Write([]byte(`{"choices":[{"message":{"con`))
|
||||
// The test HTTP server will close the connection after handler returns,
|
||||
// but Content-Length mismatch means client gets fewer bytes than expected
|
||||
return
|
||||
}
|
||||
// Second attempt: succeed
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
json.NewEncoder(w).Encode(ChatResponse{
|
||||
Choices: []struct {
|
||||
Message struct {
|
||||
Content string `json:"content"`
|
||||
} `json:"message"`
|
||||
}{{Message: struct {
|
||||
Content string `json:"content"`
|
||||
}{Content: "success"}}},
|
||||
})
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "key", "model")
|
||||
got, err := client.Complete(context.Background(), []Message{{Role: "user", Content: "Hi"}})
|
||||
if err != nil {
|
||||
t.Fatalf("expected retry to succeed, got error: %v", err)
|
||||
}
|
||||
if got != "success" {
|
||||
t.Errorf("expected %q, got %q", "success", got)
|
||||
}
|
||||
if attempts != 2 {
|
||||
t.Errorf("expected 2 attempts, got %d", attempts)
|
||||
}
|
||||
}
|
||||
|
||||
func TestComplete_ContentLengthMismatch(t *testing.T) {
|
||||
attempts := 0
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
attempts++
|
||||
if attempts == 1 {
|
||||
// Claim Content-Length is larger than actual body
|
||||
w.Header().Set("Content-Length", "500")
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
w.WriteHeader(http.StatusOK)
|
||||
// Write less than 500 bytes
|
||||
w.Write([]byte(`{"choices":[{"message":{"content":"partial"}}]}`))
|
||||
return
|
||||
}
|
||||
// Second attempt succeeds
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
json.NewEncoder(w).Encode(ChatResponse{
|
||||
Choices: []struct {
|
||||
Message struct {
|
||||
Content string `json:"content"`
|
||||
} `json:"message"`
|
||||
}{{Message: struct {
|
||||
Content string `json:"content"`
|
||||
}{Content: "complete"}}},
|
||||
})
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "key", "model")
|
||||
got, err := client.Complete(context.Background(), []Message{{Role: "user", Content: "Hi"}})
|
||||
if err != nil {
|
||||
t.Fatalf("expected retry to succeed on content-length mismatch, got: %v", err)
|
||||
}
|
||||
if got != "complete" {
|
||||
t.Errorf("expected %q, got %q", "complete", got)
|
||||
}
|
||||
}
|
||||
|
||||
func TestComplete_NoRetryOnAPIError(t *testing.T) {
|
||||
attempts := 0
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
attempts++
|
||||
w.WriteHeader(http.StatusBadRequest)
|
||||
w.Write([]byte(`{"error":"bad request"}`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "key", "model")
|
||||
_, err := client.Complete(context.Background(), []Message{{Role: "user", Content: "Hi"}})
|
||||
if err == nil {
|
||||
t.Fatal("expected error for 400, got nil")
|
||||
}
|
||||
if attempts != 1 {
|
||||
t.Errorf("should not retry on API errors, got %d attempts", attempts)
|
||||
}
|
||||
}
|
||||
|
||||
func TestIsRetryableError(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
err string
|
||||
expected bool
|
||||
}{
|
||||
{"nil formatted", "", false},
|
||||
{"read response error", "read response: unexpected EOF", true},
|
||||
{"body length mismatch", "body length mismatch: Content-Length=1000, received=500", true},
|
||||
{"API error", "LLM API error (status 400): bad request", false},
|
||||
{"parse error", "parse response: unexpected end of JSON input", false},
|
||||
{"request error", "LLM request: connection refused", false},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
if tt.err == "" {
|
||||
if isRetryableError(nil) {
|
||||
t.Error("nil error should not be retryable")
|
||||
}
|
||||
return
|
||||
}
|
||||
err := fmt.Errorf("%s", tt.err)
|
||||
got := isRetryableError(err)
|
||||
if got != tt.expected {
|
||||
t.Errorf("isRetryableError(%q) = %v, want %v", tt.err, got, tt.expected)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
+14
-36
@@ -7,40 +7,8 @@ import (
|
||||
|
||||
// FormatMarkdown formats a ReviewResult into the markdown body for a Gitea review.
|
||||
func FormatMarkdown(result *ReviewResult, reviewerName string) string {
|
||||
return FormatMarkdownWithDisplay(result, reviewerName, reviewerName)
|
||||
}
|
||||
|
||||
// GiteaEvent converts the verdict to the Gitea API event string.
|
||||
func GiteaEvent(verdict string) string {
|
||||
switch verdict {
|
||||
case "APPROVE":
|
||||
return "APPROVED"
|
||||
case "REQUEST_CHANGES":
|
||||
return "REQUEST_CHANGES"
|
||||
default:
|
||||
return "COMMENT"
|
||||
}
|
||||
}
|
||||
|
||||
// FormatMarkdownWithDisplay formats a ReviewResult with separate display name and sentinel name.
|
||||
// Note: displayName is not HTML-escaped as Gitea sanitizes rendered Markdown.
|
||||
// Persona display names are controlled by repo owners (trusted input).
|
||||
// displayName is used for the header title, sentinelName is used for the cleanup sentinel.
|
||||
// If displayName is empty, sentinelName is used for both.
|
||||
func FormatMarkdownWithDisplay(result *ReviewResult, displayName, sentinelName string) string {
|
||||
var sb strings.Builder
|
||||
|
||||
// Use display name for header, or fall back to sentinel name
|
||||
headerName := displayName
|
||||
if headerName == "" {
|
||||
headerName = sentinelName
|
||||
}
|
||||
|
||||
if headerName != "" {
|
||||
title := CapitalizeFirst(headerName)
|
||||
sb.WriteString(fmt.Sprintf("# %s Review\n\n", title))
|
||||
}
|
||||
|
||||
sb.WriteString("## Summary\n\n")
|
||||
sb.WriteString(result.Summary)
|
||||
sb.WriteString("\n\n")
|
||||
@@ -60,11 +28,21 @@ func FormatMarkdownWithDisplay(result *ReviewResult, displayName, sentinelName s
|
||||
sb.WriteString("## Recommendation\n\n")
|
||||
sb.WriteString(fmt.Sprintf("**%s** — %s\n", result.Verdict, result.Recommendation))
|
||||
|
||||
if sentinelName != "" {
|
||||
sb.WriteString(fmt.Sprintf("\n---\n*Review by %s*\n", headerName))
|
||||
// Hidden sentinel for identifying this bot's reviews during cleanup
|
||||
sb.WriteString(fmt.Sprintf("\n<!-- review-bot:%s -->\n", sentinelName))
|
||||
if reviewerName != "" {
|
||||
sb.WriteString(fmt.Sprintf("\n---\n*Review by %s*\n", reviewerName))
|
||||
}
|
||||
|
||||
return sb.String()
|
||||
}
|
||||
|
||||
// GiteaEvent converts the verdict to the Gitea API event string.
|
||||
func GiteaEvent(verdict string) string {
|
||||
switch verdict {
|
||||
case "APPROVE":
|
||||
return "APPROVED"
|
||||
case "REQUEST_CHANGES":
|
||||
return "REQUEST_CHANGES"
|
||||
default:
|
||||
return "COMMENT"
|
||||
}
|
||||
}
|
||||
|
||||
@@ -116,101 +116,3 @@ func TestGiteaEvent(t *testing.T) {
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestFormatMarkdown_Sentinel(t *testing.T) {
|
||||
result := &ReviewResult{
|
||||
Verdict: "APPROVE",
|
||||
Summary: "All good.",
|
||||
Recommendation: "Merge it.",
|
||||
}
|
||||
output := FormatMarkdown(result, "security")
|
||||
if !strings.Contains(output, "<!-- review-bot:security -->") {
|
||||
t.Error("expected sentinel comment in output")
|
||||
}
|
||||
|
||||
// Empty reviewer name should NOT have sentinel
|
||||
output2 := FormatMarkdown(result, "")
|
||||
if strings.Contains(output2, "<!-- review-bot") {
|
||||
t.Error("should not contain sentinel when reviewer name is empty")
|
||||
}
|
||||
}
|
||||
|
||||
func TestFormatMarkdown_RoleTitle(t *testing.T) {
|
||||
result := &ReviewResult{
|
||||
Verdict: "APPROVE",
|
||||
Summary: "All good.",
|
||||
Recommendation: "Merge it.",
|
||||
}
|
||||
|
||||
// With reviewer name: should have title header
|
||||
output := FormatMarkdown(result, "security")
|
||||
if !strings.Contains(output, "# Security Review\n") {
|
||||
t.Error("expected '# Security Review' header when reviewer name is set")
|
||||
}
|
||||
|
||||
output2 := FormatMarkdown(result, "gpt")
|
||||
if !strings.Contains(output2, "# Gpt Review\n") {
|
||||
t.Error("expected '# Gpt Review' header")
|
||||
}
|
||||
|
||||
// Without reviewer name: no title header
|
||||
output3 := FormatMarkdown(result, "")
|
||||
if strings.Contains(output3, "# ") && strings.Contains(output3, " Review\n") {
|
||||
t.Error("should not contain role title header when reviewer name is empty")
|
||||
}
|
||||
}
|
||||
|
||||
func TestFormatMarkdownWithDisplay(t *testing.T) {
|
||||
result := &ReviewResult{
|
||||
Verdict: "APPROVE",
|
||||
Summary: "Test summary",
|
||||
Findings: nil,
|
||||
Recommendation: "Test recommendation",
|
||||
}
|
||||
|
||||
t.Run("with display name", func(t *testing.T) {
|
||||
body := FormatMarkdownWithDisplay(result, "Security Specialist", "security")
|
||||
|
||||
// Header should use display name
|
||||
if !strings.Contains(body, "# Security Specialist Review") {
|
||||
t.Error("header should use display name")
|
||||
}
|
||||
|
||||
// Sentinel should use sentinel name
|
||||
if !strings.Contains(body, "<!-- review-bot:security -->") {
|
||||
t.Error("sentinel should use sentinel name")
|
||||
}
|
||||
|
||||
// Footer "Review by" should use display name
|
||||
if !strings.Contains(body, "*Review by Security Specialist*") {
|
||||
t.Error("footer should use display name")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("without display name", func(t *testing.T) {
|
||||
body := FormatMarkdownWithDisplay(result, "", "reviewer")
|
||||
|
||||
// Should fall back to sentinel name for header
|
||||
if !strings.Contains(body, "# Reviewer Review") {
|
||||
t.Error("header should fall back to sentinel name")
|
||||
}
|
||||
|
||||
if !strings.Contains(body, "<!-- review-bot:reviewer -->") {
|
||||
t.Error("sentinel should use sentinel name")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("empty both names", func(t *testing.T) {
|
||||
body := FormatMarkdownWithDisplay(result, "", "")
|
||||
|
||||
// Should not have header
|
||||
if strings.Contains(body, "# ") && strings.Contains(body, " Review") {
|
||||
t.Error("should not have header when both names empty")
|
||||
}
|
||||
|
||||
// Should not have sentinel
|
||||
if strings.Contains(body, "<!-- review-bot:") {
|
||||
t.Error("should not have sentinel when sentinel name empty")
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
+1
-240
@@ -29,19 +29,7 @@ func ParseResponse(response string) (*ReviewResult, error) {
|
||||
|
||||
var result ReviewResult
|
||||
if err := json.Unmarshal([]byte(cleaned), &result); err != nil {
|
||||
// LLMs sometimes produce JSON with unescaped quotes inside string values.
|
||||
// Try to repair before giving up.
|
||||
repaired := repairJSON(cleaned)
|
||||
if err2 := json.Unmarshal([]byte(repaired), &result); err2 != nil {
|
||||
// Include diagnostic info: lengths help identify truncation
|
||||
rawLen := len(response)
|
||||
cleanedLen := len(cleaned)
|
||||
preview := cleaned
|
||||
if len(preview) > 200 {
|
||||
preview = preview[:100] + "..." + preview[len(preview)-100:]
|
||||
}
|
||||
return nil, fmt.Errorf("parse LLM response as JSON: %w\nRaw length: %d, cleaned length: %d\nCleaned preview: %s", err, rawLen, cleanedLen, preview)
|
||||
}
|
||||
return nil, fmt.Errorf("parse LLM response as JSON: %w\nRaw response: %s", err, response)
|
||||
}
|
||||
|
||||
// Validate verdict
|
||||
@@ -86,230 +74,3 @@ func extractJSON(s string) string {
|
||||
s = strings.TrimSpace(s)
|
||||
return s
|
||||
}
|
||||
|
||||
// repairJSON attempts to fix common LLM JSON issues:
|
||||
// - Unescaped double quotes inside string values
|
||||
//
|
||||
// Strategy: walk the JSON structurally. Object keys are parsed normally (LLMs
|
||||
// get those right). For string VALUES, we find all candidate closing quotes and
|
||||
// pick the LAST one that leaves valid JSON structure afterward — maximizing
|
||||
// string content, which is the correct bias for the "LLM put unescaped quotes
|
||||
// in a string value" failure mode.
|
||||
func repairJSON(s string) string {
|
||||
runes := []rune(s)
|
||||
var out strings.Builder
|
||||
out.Grow(len(s) + 64)
|
||||
|
||||
i := 0
|
||||
for i < len(runes) {
|
||||
c := runes[i]
|
||||
|
||||
if c != '"' {
|
||||
out.WriteRune(c)
|
||||
i++
|
||||
continue
|
||||
}
|
||||
|
||||
// We hit an opening quote. Determine if this is a key or a value.
|
||||
// Keys: the standard JSON parser in LLMs gets keys right, so we parse
|
||||
// them normally (first unescaped quote closes).
|
||||
// Values: may contain unescaped quotes — use the repair heuristic.
|
||||
isValue := isValuePosition(runes, i)
|
||||
|
||||
if !isValue {
|
||||
// Parse key/simple string normally
|
||||
out.WriteRune('"')
|
||||
i++
|
||||
for i < len(runes) {
|
||||
ch := runes[i]
|
||||
if ch == '\\' && i+1 < len(runes) {
|
||||
out.WriteRune(ch)
|
||||
i++
|
||||
out.WriteRune(runes[i])
|
||||
i++
|
||||
continue
|
||||
}
|
||||
if ch == '"' {
|
||||
out.WriteRune('"')
|
||||
i++
|
||||
break
|
||||
}
|
||||
out.WriteRune(ch)
|
||||
i++
|
||||
}
|
||||
continue
|
||||
}
|
||||
|
||||
// Value string — find the correct close using last-valid-candidate heuristic
|
||||
out.WriteRune('"')
|
||||
i++
|
||||
|
||||
closeIdx := findClosingQuote(runes, i)
|
||||
|
||||
// Write everything between open and close, escaping interior quotes
|
||||
for j := i; j < closeIdx; j++ {
|
||||
ch := runes[j]
|
||||
if ch == '\\' && j+1 < closeIdx {
|
||||
// Already-escaped sequence — pass through
|
||||
out.WriteRune(ch)
|
||||
j++
|
||||
out.WriteRune(runes[j])
|
||||
} else if ch == '"' {
|
||||
out.WriteRune('\\')
|
||||
out.WriteRune('"')
|
||||
} else {
|
||||
out.WriteRune(ch)
|
||||
}
|
||||
}
|
||||
|
||||
// Write the closing quote
|
||||
out.WriteRune('"')
|
||||
i = closeIdx + 1
|
||||
}
|
||||
|
||||
return out.String()
|
||||
}
|
||||
|
||||
// isValuePosition determines if the quote at position i is opening a JSON value
|
||||
// string (as opposed to an object key). We only apply repair to values that
|
||||
// follow ':' since those are the free-text fields where LLMs produce unescaped
|
||||
// quotes. Array elements and keys are left alone (parsed normally).
|
||||
func isValuePosition(runes []rune, i int) bool {
|
||||
// Look backward, skipping whitespace, for the preceding structural char
|
||||
j := i - 1
|
||||
for j >= 0 && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
|
||||
j--
|
||||
}
|
||||
if j < 0 {
|
||||
return false
|
||||
}
|
||||
// After ':' → definitely a value
|
||||
return runes[j] == ':'
|
||||
}
|
||||
|
||||
// findClosingQuote finds the index of the true closing quote for a JSON string
|
||||
// value starting at position start (the character after the opening quote).
|
||||
// It collects all unescaped quote candidates and returns the FIRST one that
|
||||
// produces valid JSON continuation (deeper lookahead verifies the next token).
|
||||
func findClosingQuote(runes []rune, start int) int {
|
||||
// Collect all candidate positions for the closing quote.
|
||||
var candidates []int
|
||||
for j := start; j < len(runes); j++ {
|
||||
if runes[j] == '\\' {
|
||||
j++ // skip escaped character
|
||||
continue
|
||||
}
|
||||
if runes[j] == '"' {
|
||||
candidates = append(candidates, j)
|
||||
}
|
||||
}
|
||||
|
||||
if len(candidates) == 0 {
|
||||
return len(runes)
|
||||
}
|
||||
|
||||
if len(candidates) == 1 {
|
||||
return candidates[0]
|
||||
}
|
||||
|
||||
// Try candidates from FIRST to LAST. The correct closing quote is the
|
||||
// earliest one that produces valid JSON structure after it (verified by
|
||||
// deeper lookahead that checks the next token is a valid JSON start).
|
||||
for _, idx := range candidates {
|
||||
if isValidJSONAfterClose(runes, idx+1) {
|
||||
return idx
|
||||
}
|
||||
}
|
||||
|
||||
// Fallback: return the last candidate
|
||||
return candidates[len(candidates)-1]
|
||||
}
|
||||
|
||||
// isValidJSONAfterClose checks whether the runes after a candidate closing quote
|
||||
// look like valid JSON continuation for a VALUE string. Since we only use this
|
||||
// for value positions, ':' is NOT a valid continuation (values are never keys).
|
||||
// Checks deeper structure to avoid being fooled by JSON-like content in strings.
|
||||
func isValidJSONAfterClose(runes []rune, pos int) bool {
|
||||
j := pos
|
||||
for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
|
||||
j++
|
||||
}
|
||||
|
||||
if j >= len(runes) {
|
||||
return true
|
||||
}
|
||||
|
||||
next := runes[j]
|
||||
if next == '}' || next == ']' {
|
||||
// Closing a container. Verify what follows the close is also valid:
|
||||
// another structural char, comma, or EOF.
|
||||
return isValidAfterContainerClose(runes, j+1)
|
||||
}
|
||||
if next == ',' {
|
||||
// After comma, must be followed by a valid JSON token
|
||||
j++
|
||||
for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
|
||||
j++
|
||||
}
|
||||
if j >= len(runes) {
|
||||
return false // trailing comma with nothing after — invalid
|
||||
}
|
||||
return isJSONTokenStart(runes, j)
|
||||
}
|
||||
// ':' is NOT valid here — we're in a value position, not a key.
|
||||
// Any other character is also invalid.
|
||||
return false
|
||||
}
|
||||
|
||||
// isValidAfterContainerClose checks that after a } or ], the continuation is
|
||||
// structurally valid: more closes, comma+token, or EOF.
|
||||
func isValidAfterContainerClose(runes []rune, pos int) bool {
|
||||
j := pos
|
||||
for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
|
||||
j++
|
||||
}
|
||||
if j >= len(runes) {
|
||||
return true
|
||||
}
|
||||
next := runes[j]
|
||||
if next == '}' || next == ']' {
|
||||
return isValidAfterContainerClose(runes, j+1)
|
||||
}
|
||||
if next == ',' {
|
||||
j++
|
||||
for j < len(runes) && (runes[j] == ' ' || runes[j] == '\t' || runes[j] == '\n' || runes[j] == '\r') {
|
||||
j++
|
||||
}
|
||||
if j >= len(runes) {
|
||||
return false
|
||||
}
|
||||
return isJSONTokenStart(runes, j)
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
// isJSONTokenStart returns true if the rune could begin a JSON value or key.
|
||||
// For keywords (true/false/null), verifies the full keyword is present.
|
||||
func isJSONTokenStart(runes []rune, pos int) bool {
|
||||
if pos >= len(runes) {
|
||||
return false
|
||||
}
|
||||
r := runes[pos]
|
||||
switch {
|
||||
case r == '"': // string
|
||||
return true
|
||||
case r == '{' || r == '[': // object or array
|
||||
return true
|
||||
case r == 't': // true
|
||||
return pos+4 <= len(runes) && string(runes[pos:pos+4]) == "true"
|
||||
case r == 'f': // false
|
||||
return pos+5 <= len(runes) && string(runes[pos:pos+5]) == "false"
|
||||
case r == 'n': // null
|
||||
return pos+4 <= len(runes) && string(runes[pos:pos+4]) == "null"
|
||||
case r >= '0' && r <= '9': // number
|
||||
return true
|
||||
case r == '-': // negative number
|
||||
return true
|
||||
}
|
||||
return false
|
||||
}
|
||||
|
||||
@@ -1,7 +1,6 @@
|
||||
package review
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"testing"
|
||||
)
|
||||
|
||||
@@ -113,112 +112,3 @@ func TestParseResponse_MarkdownFencesNoLang(t *testing.T) {
|
||||
t.Errorf("expected APPROVE, got %q", result.Verdict)
|
||||
}
|
||||
}
|
||||
|
||||
func TestParseResponse_UnescapedQuotesInStrings(t *testing.T) {
|
||||
// Real failure from CI: Sonnet puts unescaped quotes like (e.g. "28") in findings
|
||||
input := `{"verdict": "APPROVE", "summary": "Clean PR", "findings": [{"severity": "NIT", "file": "ci/Dockerfile", "line": 14, "finding": "The comment says OTP_VERSION is the major version (e.g. \"28\") but it actually contains unescaped quotes like (e.g. "28") which breaks JSON"}], "recommendation": "Ship it"}`
|
||||
|
||||
result, err := ParseResponse(input)
|
||||
if err != nil {
|
||||
t.Fatalf("expected repair to handle unescaped quotes, got error: %v", err)
|
||||
}
|
||||
if result.Verdict != "APPROVE" {
|
||||
t.Errorf("expected APPROVE, got %q", result.Verdict)
|
||||
}
|
||||
if len(result.Findings) != 1 {
|
||||
t.Fatalf("expected 1 finding, got %d", len(result.Findings))
|
||||
}
|
||||
}
|
||||
|
||||
func TestRepairJSON_NoOpOnValid(t *testing.T) {
|
||||
valid := `{"key": "value", "num": 42}`
|
||||
result := repairJSON(valid)
|
||||
if result != valid {
|
||||
t.Errorf("repairJSON should not modify valid JSON\n got: %s\n want: %s", result, valid)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRepairJSON_FixesUnescapedQuotes(t *testing.T) {
|
||||
// Interior quote followed by non-structural character
|
||||
input := `{"msg": "use "foo" here"}`
|
||||
result := repairJSON(input)
|
||||
|
||||
// Should be parseable now
|
||||
var m map[string]interface{}
|
||||
if err := json.Unmarshal([]byte(result), &m); err != nil {
|
||||
t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRepairJSON_InteriorQuoteBeforeComma(t *testing.T) {
|
||||
// Bug reported by reviewer: interior quoted word immediately before a comma
|
||||
input := `{"msg": "say "yes", and go"}`
|
||||
result := repairJSON(input)
|
||||
|
||||
var m map[string]interface{}
|
||||
if err := json.Unmarshal([]byte(result), &m); err != nil {
|
||||
t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
|
||||
}
|
||||
// The full string content should be preserved
|
||||
msg, ok := m["msg"].(string)
|
||||
if !ok {
|
||||
t.Fatal("msg field missing or not a string")
|
||||
}
|
||||
if msg != `say "yes", and go` {
|
||||
t.Errorf("unexpected msg content: %q", msg)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRepairJSON_InteriorQuoteBeforeCloseBrace(t *testing.T) {
|
||||
// Bug reported by reviewer: JSON-shaped syntax inside string values
|
||||
input := `{"msg": "input map {"key": "val"} caused error"}`
|
||||
result := repairJSON(input)
|
||||
|
||||
var m map[string]interface{}
|
||||
if err := json.Unmarshal([]byte(result), &m); err != nil {
|
||||
t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRepairJSON_MultipleFields(t *testing.T) {
|
||||
// Multiple string fields with unescaped quotes in different positions
|
||||
input := `{"a": "hello "world"", "b": "foo"}`
|
||||
result := repairJSON(input)
|
||||
|
||||
var m map[string]interface{}
|
||||
if err := json.Unmarshal([]byte(result), &m); err != nil {
|
||||
t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
|
||||
}
|
||||
if _, ok := m["b"]; !ok {
|
||||
t.Error("expected 'b' field to be preserved")
|
||||
}
|
||||
}
|
||||
|
||||
func TestRepairJSON_PreservesEscapedQuotes(t *testing.T) {
|
||||
// Already-escaped quotes should not be double-escaped
|
||||
input := `{"msg": "already \"escaped\" here"}`
|
||||
result := repairJSON(input)
|
||||
|
||||
if result != input {
|
||||
t.Errorf("repairJSON should not modify already-escaped quotes\n got: %s\n want: %s", result, input)
|
||||
}
|
||||
|
||||
var m map[string]interface{}
|
||||
if err := json.Unmarshal([]byte(result), &m); err != nil {
|
||||
t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
|
||||
}
|
||||
}
|
||||
|
||||
func TestRepairJSON_ComplexNestedContent(t *testing.T) {
|
||||
// Combines both reviewer bugs: quoted words before commas AND JSON-like content
|
||||
input := `{"verdict": "APPROVE", "findings": [{"finding": "The map {"key": "val"} and (e.g. "28") and say "yes", then stop"}]}`
|
||||
result := repairJSON(input)
|
||||
|
||||
var parsed map[string]interface{}
|
||||
if err := json.Unmarshal([]byte(result), &parsed); err != nil {
|
||||
t.Fatalf("repaired JSON should parse, got: %v\nrepaired: %s", err, result)
|
||||
}
|
||||
if parsed["verdict"] != "APPROVE" {
|
||||
t.Errorf("expected verdict APPROVE, got %v", parsed["verdict"])
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1,112 +0,0 @@
|
||||
package review
|
||||
|
||||
import (
|
||||
"embed"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"os"
|
||||
"strings"
|
||||
"unicode/utf8"
|
||||
)
|
||||
|
||||
//go:embed personas/*.json
|
||||
var embeddedPersonas embed.FS
|
||||
|
||||
// Persona defines a specialized review role with focused expertise.
|
||||
type Persona struct {
|
||||
Name string `json:"name"`
|
||||
DisplayName string `json:"display_name"`
|
||||
ModelPref string `json:"model_preference,omitempty"`
|
||||
Identity string `json:"identity"`
|
||||
Focus []string `json:"focus"`
|
||||
Ignore []string `json:"ignore"`
|
||||
Severity Severity `json:"severity"`
|
||||
OutputFormat string `json:"output_format,omitempty"`
|
||||
}
|
||||
|
||||
// Severity defines what constitutes each severity level for this persona.
|
||||
// These are prompt guidance for the LLM, not output format changes.
|
||||
type Severity struct {
|
||||
Major string `json:"major"`
|
||||
Minor string `json:"minor"`
|
||||
Nit string `json:"nit"`
|
||||
}
|
||||
|
||||
// LoadPersona loads a persona from a JSON file path.
|
||||
func LoadPersona(path string) (*Persona, error) {
|
||||
data, err := os.ReadFile(path)
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("read persona file %s: %w", path, err)
|
||||
}
|
||||
return parsePersona(data, path)
|
||||
}
|
||||
|
||||
// LoadBuiltinPersona loads a built-in persona by name.
|
||||
// Returns an error if the persona doesn't exist.
|
||||
func LoadBuiltinPersona(name string) (*Persona, error) {
|
||||
filename := name + ".json"
|
||||
data, err := embeddedPersonas.ReadFile("personas/" + filename) // embed.FS paths use forward slashes per io/fs spec
|
||||
if err != nil {
|
||||
available := ListBuiltinPersonas()
|
||||
return nil, fmt.Errorf("unknown built-in persona %q (available: %s)", name, strings.Join(available, ", "))
|
||||
}
|
||||
return parsePersona(data, "builtin:"+name)
|
||||
}
|
||||
|
||||
// ListBuiltinPersonas returns the names of all built-in personas.
|
||||
// Returns an empty slice if the embedded directory cannot be read.
|
||||
func ListBuiltinPersonas() []string {
|
||||
entries, err := embeddedPersonas.ReadDir("personas")
|
||||
if err != nil {
|
||||
return []string{}
|
||||
}
|
||||
var names []string
|
||||
for _, e := range entries {
|
||||
if e.IsDir() {
|
||||
continue
|
||||
}
|
||||
name := e.Name()
|
||||
if strings.HasSuffix(name, ".json") {
|
||||
names = append(names, strings.TrimSuffix(name, ".json"))
|
||||
}
|
||||
}
|
||||
return names
|
||||
}
|
||||
|
||||
func parsePersona(data []byte, source string) (*Persona, error) {
|
||||
var p Persona
|
||||
if err := json.Unmarshal(data, &p); err != nil {
|
||||
return nil, fmt.Errorf("parse persona %s: %w", source, err)
|
||||
}
|
||||
if err := validatePersona(&p, source); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
return &p, nil
|
||||
}
|
||||
|
||||
func validatePersona(p *Persona, source string) error {
|
||||
if p.Name == "" {
|
||||
return fmt.Errorf("persona %s: name is required", source)
|
||||
}
|
||||
if p.Identity == "" {
|
||||
return fmt.Errorf("persona %s: identity is required", source)
|
||||
}
|
||||
// DisplayName defaults to Name if not set
|
||||
if p.DisplayName == "" {
|
||||
p.DisplayName = p.Name
|
||||
}
|
||||
return nil
|
||||
}
|
||||
|
||||
// CapitalizeFirst capitalizes the first rune of a string in a Unicode-safe way.
|
||||
// Returns the original string if it's empty.
|
||||
func CapitalizeFirst(s string) string {
|
||||
if s == "" {
|
||||
return s
|
||||
}
|
||||
r, size := utf8.DecodeRuneInString(s)
|
||||
if r == utf8.RuneError {
|
||||
return s
|
||||
}
|
||||
return strings.ToUpper(string(r)) + s[size:]
|
||||
}
|
||||
@@ -1,104 +0,0 @@
|
||||
package review
|
||||
|
||||
import (
|
||||
"fmt"
|
||||
"strings"
|
||||
)
|
||||
|
||||
// BuildPersonaSystemPrompt constructs a system prompt from a persona definition.
|
||||
// This replaces BuildSystemBase when a persona is provided.
|
||||
func BuildPersonaSystemPrompt(p *Persona) string {
|
||||
var sb strings.Builder
|
||||
|
||||
// Identity section
|
||||
sb.WriteString(p.Identity)
|
||||
sb.WriteString("\n\n")
|
||||
|
||||
// Focus section
|
||||
if len(p.Focus) > 0 {
|
||||
sb.WriteString("## Focus Areas\n\n")
|
||||
sb.WriteString("Concentrate your review on:\n")
|
||||
for _, f := range p.Focus {
|
||||
sb.WriteString(fmt.Sprintf("- %s\n", f))
|
||||
}
|
||||
sb.WriteString("\n")
|
||||
}
|
||||
|
||||
// Ignore section
|
||||
if len(p.Ignore) > 0 {
|
||||
sb.WriteString("## Explicitly Out of Scope\n\n")
|
||||
sb.WriteString("Do NOT comment on:\n")
|
||||
for _, i := range p.Ignore {
|
||||
sb.WriteString(fmt.Sprintf("- %s\n", i))
|
||||
}
|
||||
sb.WriteString("\n")
|
||||
}
|
||||
|
||||
// Severity calibration
|
||||
if p.Severity.Major != "" || p.Severity.Minor != "" || p.Severity.Nit != "" {
|
||||
sb.WriteString("## Severity Calibration\n\n")
|
||||
sb.WriteString("Use these severity definitions for YOUR domain:\n")
|
||||
if p.Severity.Major != "" {
|
||||
sb.WriteString(fmt.Sprintf("- **MAJOR**: %s\n", p.Severity.Major))
|
||||
}
|
||||
if p.Severity.Minor != "" {
|
||||
sb.WriteString(fmt.Sprintf("- **MINOR**: %s\n", p.Severity.Minor))
|
||||
}
|
||||
if p.Severity.Nit != "" {
|
||||
sb.WriteString(fmt.Sprintf("- **NIT**: %s\n", p.Severity.Nit))
|
||||
}
|
||||
sb.WriteString("\n")
|
||||
}
|
||||
|
||||
// Output format instructions (shared schema from prompt.go)
|
||||
sb.WriteString("## Review Instructions\n\n")
|
||||
sb.WriteString("CONTEXT:\n")
|
||||
sb.WriteString("- You will receive the full content of modified files for reference, followed by the diff showing what changed.\n")
|
||||
sb.WriteString("- The diff shows ONLY what was added/removed. The full file content provides complete context.\n")
|
||||
sb.WriteString("- Focus your review on the CHANGES (the diff), using the full files for context.\n\n")
|
||||
sb.WriteString("Your task:\n")
|
||||
sb.WriteString("1. Review the diff for issues within YOUR focus areas only.\n")
|
||||
sb.WriteString("2. Consider the CI status — if CI has failed, that is an automatic REQUEST_CHANGES regardless of code quality.\n")
|
||||
sb.WriteString("3. Output your review as structured JSON (and ONLY JSON, no markdown fences or other text).\n\n")
|
||||
sb.WriteString("Output format:\n")
|
||||
sb.WriteString(outputSchemaJSON)
|
||||
sb.WriteString("\n\n")
|
||||
sb.WriteString(verdictRules)
|
||||
sb.WriteString("\n- Only report findings within your focus areas. Ignore everything else.\n")
|
||||
sb.WriteString("- Line numbers should reference the new file line numbers from the diff headers.\n")
|
||||
sb.WriteString("- If the diff has no changes relevant to your focus areas, APPROVE with no findings.\n")
|
||||
|
||||
// Custom output format if provided
|
||||
if p.OutputFormat != "" {
|
||||
sb.WriteString("\n\n## Additional Output Guidelines\n\n")
|
||||
sb.WriteString(p.OutputFormat)
|
||||
}
|
||||
|
||||
return sb.String()
|
||||
}
|
||||
|
||||
// BuildSystemPromptWithPersona constructs the full system prompt, using either
|
||||
// a persona or the default generic prompt. This is a convenience wrapper that
|
||||
// combines BuildPersonaSystemPrompt (or BuildSystemBase) with patterns and conventions.
|
||||
// It is exported for use by callers who want one-shot prompt assembly.
|
||||
func BuildSystemPromptWithPersona(persona *Persona, conventions, patterns string) string {
|
||||
var base string
|
||||
if persona != nil {
|
||||
base = BuildPersonaSystemPrompt(persona)
|
||||
} else {
|
||||
base = BuildSystemBase()
|
||||
}
|
||||
|
||||
var sb strings.Builder
|
||||
sb.WriteString(base)
|
||||
|
||||
if patterns != "" {
|
||||
sb.WriteString(fmt.Sprintf("\n\n## Language Patterns & Idioms\n\nUse the following patterns as review criteria. Code that violates these established patterns is a finding:\n\n%s\n", patterns))
|
||||
}
|
||||
|
||||
if conventions != "" {
|
||||
sb.WriteString(fmt.Sprintf("\n\n## Repository Conventions\n\nThe repository has the following coding conventions that must be respected:\n\n%s\n", conventions))
|
||||
}
|
||||
|
||||
return sb.String()
|
||||
}
|
||||
@@ -1,157 +0,0 @@
|
||||
package review
|
||||
|
||||
import (
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestBuildPersonaSystemPrompt(t *testing.T) {
|
||||
p := &Persona{
|
||||
Name: "security",
|
||||
DisplayName: "Security Specialist",
|
||||
Identity: "You are a security specialist.",
|
||||
Focus: []string{"injection attacks", "auth bypass"},
|
||||
Ignore: []string{"code style", "performance"},
|
||||
Severity: Severity{
|
||||
Major: "exploitable vulnerabilities",
|
||||
Minor: "defense in depth",
|
||||
Nit: "theoretical risks",
|
||||
},
|
||||
}
|
||||
|
||||
prompt := BuildPersonaSystemPrompt(p)
|
||||
|
||||
// Check identity is included
|
||||
if !strings.Contains(prompt, "You are a security specialist.") {
|
||||
t.Error("prompt should contain identity")
|
||||
}
|
||||
|
||||
// Check focus areas
|
||||
if !strings.Contains(prompt, "Focus Areas") {
|
||||
t.Error("prompt should contain Focus Areas section")
|
||||
}
|
||||
if !strings.Contains(prompt, "injection attacks") {
|
||||
t.Error("prompt should contain focus item")
|
||||
}
|
||||
|
||||
// Check ignore section
|
||||
if !strings.Contains(prompt, "Out of Scope") {
|
||||
t.Error("prompt should contain Out of Scope section")
|
||||
}
|
||||
if !strings.Contains(prompt, "code style") {
|
||||
t.Error("prompt should contain ignore item")
|
||||
}
|
||||
|
||||
// Check severity calibration
|
||||
if !strings.Contains(prompt, "Severity Calibration") {
|
||||
t.Error("prompt should contain Severity Calibration section")
|
||||
}
|
||||
if !strings.Contains(prompt, "exploitable vulnerabilities") {
|
||||
t.Error("prompt should contain major severity definition")
|
||||
}
|
||||
|
||||
// Check JSON output format is included
|
||||
if !strings.Contains(prompt, `"verdict"`) {
|
||||
t.Error("prompt should contain JSON output format")
|
||||
}
|
||||
if !strings.Contains(prompt, "APPROVE") {
|
||||
t.Error("prompt should mention APPROVE verdict")
|
||||
}
|
||||
}
|
||||
|
||||
func TestBuildPersonaSystemPromptMinimal(t *testing.T) {
|
||||
// Minimal persona with only required fields
|
||||
p := &Persona{
|
||||
Name: "minimal",
|
||||
Identity: "You are a minimal reviewer.",
|
||||
}
|
||||
|
||||
prompt := BuildPersonaSystemPrompt(p)
|
||||
|
||||
// Should still work without optional fields
|
||||
if !strings.Contains(prompt, "You are a minimal reviewer.") {
|
||||
t.Error("prompt should contain identity")
|
||||
}
|
||||
|
||||
// Should not have empty sections
|
||||
if strings.Contains(prompt, "Focus Areas") && !strings.Contains(prompt, "Concentrate your review on:") {
|
||||
t.Error("should not have Focus Areas header without content")
|
||||
}
|
||||
}
|
||||
|
||||
func TestBuildSystemPromptWithPersona(t *testing.T) {
|
||||
t.Run("with persona", func(t *testing.T) {
|
||||
p := &Persona{
|
||||
Name: "test",
|
||||
Identity: "Test persona identity.",
|
||||
Focus: []string{"testing"},
|
||||
}
|
||||
|
||||
prompt := BuildSystemPromptWithPersona(p, "test conventions", "test patterns")
|
||||
|
||||
if !strings.Contains(prompt, "Test persona identity.") {
|
||||
t.Error("should contain persona identity")
|
||||
}
|
||||
if !strings.Contains(prompt, "test conventions") {
|
||||
t.Error("should contain conventions")
|
||||
}
|
||||
if !strings.Contains(prompt, "test patterns") {
|
||||
t.Error("should contain patterns")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("without persona", func(t *testing.T) {
|
||||
prompt := BuildSystemPromptWithPersona(nil, "test conventions", "test patterns")
|
||||
|
||||
// Should use default system base
|
||||
if !strings.Contains(prompt, "expert code reviewer") {
|
||||
t.Error("should contain default system base when no persona")
|
||||
}
|
||||
if !strings.Contains(prompt, "test conventions") {
|
||||
t.Error("should contain conventions")
|
||||
}
|
||||
})
|
||||
|
||||
t.Run("empty conventions and patterns", func(t *testing.T) {
|
||||
p := &Persona{
|
||||
Name: "test",
|
||||
Identity: "Test identity.",
|
||||
}
|
||||
|
||||
prompt := BuildSystemPromptWithPersona(p, "", "")
|
||||
|
||||
if strings.Contains(prompt, "Language Patterns") {
|
||||
t.Error("should not contain patterns section when empty")
|
||||
}
|
||||
if strings.Contains(prompt, "Repository Conventions") {
|
||||
t.Error("should not contain conventions section when empty")
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
func TestPersonaPromptContainsOutputRules(t *testing.T) {
|
||||
p := &Persona{
|
||||
Name: "test",
|
||||
Identity: "Test.",
|
||||
}
|
||||
|
||||
prompt := BuildPersonaSystemPrompt(p)
|
||||
|
||||
// Must contain the critical output rules
|
||||
requiredStrings := []string{
|
||||
"APPROVE",
|
||||
"REQUEST_CHANGES",
|
||||
"MAJOR",
|
||||
"MINOR",
|
||||
"NIT",
|
||||
"verdict",
|
||||
"findings",
|
||||
"CI",
|
||||
}
|
||||
|
||||
for _, s := range requiredStrings {
|
||||
if !strings.Contains(prompt, s) {
|
||||
t.Errorf("prompt should contain %q", s)
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1,239 +0,0 @@
|
||||
package review
|
||||
|
||||
import (
|
||||
"os"
|
||||
"path/filepath"
|
||||
"strings"
|
||||
"testing"
|
||||
)
|
||||
|
||||
func TestLoadBuiltinPersona(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
personaName string
|
||||
wantErr bool
|
||||
wantDisplay string
|
||||
}{
|
||||
{
|
||||
name: "security persona",
|
||||
personaName: "security",
|
||||
wantErr: false,
|
||||
wantDisplay: "Security Specialist",
|
||||
},
|
||||
{
|
||||
name: "architect persona",
|
||||
personaName: "architect",
|
||||
wantErr: false,
|
||||
wantDisplay: "Software Architect",
|
||||
},
|
||||
{
|
||||
name: "docs persona",
|
||||
personaName: "docs",
|
||||
wantErr: false,
|
||||
wantDisplay: "Documentation Reviewer",
|
||||
},
|
||||
{
|
||||
name: "unknown persona",
|
||||
personaName: "nonexistent",
|
||||
wantErr: true,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
p, err := LoadBuiltinPersona(tt.personaName)
|
||||
if tt.wantErr {
|
||||
if err == nil {
|
||||
t.Error("expected error, got nil")
|
||||
}
|
||||
return
|
||||
}
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
if p.Name != tt.personaName {
|
||||
t.Errorf("Name = %q, want %q", p.Name, tt.personaName)
|
||||
}
|
||||
if p.DisplayName != tt.wantDisplay {
|
||||
t.Errorf("DisplayName = %q, want %q", p.DisplayName, tt.wantDisplay)
|
||||
}
|
||||
if p.Identity == "" {
|
||||
t.Error("Identity should not be empty")
|
||||
}
|
||||
if len(p.Focus) == 0 {
|
||||
t.Error("Focus should not be empty")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestListBuiltinPersonas(t *testing.T) {
|
||||
names := ListBuiltinPersonas()
|
||||
if len(names) == 0 {
|
||||
t.Fatal("expected at least one built-in persona")
|
||||
}
|
||||
|
||||
// Check for expected personas
|
||||
expected := map[string]bool{"security": false, "architect": false, "docs": false}
|
||||
for _, name := range names {
|
||||
if _, ok := expected[name]; ok {
|
||||
expected[name] = true
|
||||
}
|
||||
}
|
||||
for name, found := range expected {
|
||||
if !found {
|
||||
t.Errorf("expected built-in persona %q not found", name)
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
func TestLoadPersonaFromJSONFile(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
path := filepath.Join(dir, "test.json")
|
||||
|
||||
content := `{
|
||||
"name": "test",
|
||||
"display_name": "Test Persona",
|
||||
"identity": "You are a test persona.\nMulti-line identity works.",
|
||||
"focus": ["testing", "validation"],
|
||||
"ignore": ["nothing"],
|
||||
"severity": {
|
||||
"major": "Big problems",
|
||||
"minor": "Small problems",
|
||||
"nit": "Tiny problems"
|
||||
}
|
||||
}`
|
||||
|
||||
if err := os.WriteFile(path, []byte(content), 0644); err != nil {
|
||||
t.Fatalf("failed to write test file: %v", err)
|
||||
}
|
||||
|
||||
p, err := LoadPersona(path)
|
||||
if err != nil {
|
||||
t.Fatalf("LoadPersona failed: %v", err)
|
||||
}
|
||||
|
||||
if p.Name != "test" {
|
||||
t.Errorf("Name = %q, want %q", p.Name, "test")
|
||||
}
|
||||
if p.DisplayName != "Test Persona" {
|
||||
t.Errorf("DisplayName = %q, want %q", p.DisplayName, "Test Persona")
|
||||
}
|
||||
if len(p.Focus) != 2 {
|
||||
t.Errorf("Focus len = %d, want 2", len(p.Focus))
|
||||
}
|
||||
if !strings.Contains(p.Identity, "Multi-line") {
|
||||
t.Error("Identity should contain multi-line content")
|
||||
}
|
||||
}
|
||||
|
||||
func TestLoadPersonaValidation(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
json string
|
||||
wantErr string
|
||||
}{
|
||||
{
|
||||
name: "missing name",
|
||||
json: `{"identity": "test"}`,
|
||||
wantErr: "name is required",
|
||||
},
|
||||
{
|
||||
name: "missing identity",
|
||||
json: `{"name": "test"}`,
|
||||
wantErr: "identity is required",
|
||||
},
|
||||
{
|
||||
name: "display_name defaults to name",
|
||||
json: `{"name": "test", "identity": "test identity"}`,
|
||||
// No error expected - should succeed
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
path := filepath.Join(dir, "test.json")
|
||||
if err := os.WriteFile(path, []byte(tt.json), 0644); err != nil {
|
||||
t.Fatalf("failed to write test file: %v", err)
|
||||
}
|
||||
|
||||
p, err := LoadPersona(path)
|
||||
if tt.wantErr != "" {
|
||||
if err == nil {
|
||||
t.Errorf("expected error containing %q, got nil", tt.wantErr)
|
||||
return
|
||||
}
|
||||
if !strings.Contains(err.Error(), tt.wantErr) {
|
||||
t.Errorf("error = %q, want containing %q", err.Error(), tt.wantErr)
|
||||
}
|
||||
return
|
||||
}
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error: %v", err)
|
||||
}
|
||||
// Check display_name defaulting
|
||||
if p.DisplayName == "" {
|
||||
t.Error("DisplayName should default to Name")
|
||||
}
|
||||
if p.DisplayName != p.Name {
|
||||
t.Errorf("DisplayName should default to Name, got %q", p.DisplayName)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestLoadPersonaFileNotFound(t *testing.T) {
|
||||
_, err := LoadPersona("/nonexistent/path/persona.json")
|
||||
if err == nil {
|
||||
t.Error("expected error for nonexistent file")
|
||||
}
|
||||
}
|
||||
|
||||
func TestLoadPersonaInvalidJSON(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
path := filepath.Join(dir, "invalid.json")
|
||||
if err := os.WriteFile(path, []byte("not valid json {"), 0644); err != nil {
|
||||
t.Fatalf("failed to write test file: %v", err)
|
||||
}
|
||||
|
||||
_, err := LoadPersona(path)
|
||||
if err == nil {
|
||||
t.Error("expected error for invalid JSON")
|
||||
}
|
||||
}
|
||||
|
||||
func TestCapitalizeFirst(t *testing.T) {
|
||||
tests := []struct {
|
||||
input string
|
||||
want string
|
||||
}{
|
||||
{"hello", "Hello"},
|
||||
{"Hello", "Hello"},
|
||||
{"HELLO", "HELLO"},
|
||||
{"a", "A"},
|
||||
{"", ""},
|
||||
{"日本語", "日本語"}, // Non-ASCII: Japanese doesn't have case
|
||||
{"über", "Über"}, // German umlaut
|
||||
{"élève", "Élève"}, // French accent
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.input, func(t *testing.T) {
|
||||
got := CapitalizeFirst(tt.input)
|
||||
if got != tt.want {
|
||||
t.Errorf("CapitalizeFirst(%q) = %q, want %q", tt.input, got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestListBuiltinPersonasReturnsEmptySlice(t *testing.T) {
|
||||
// ListBuiltinPersonas should return an empty slice (not nil) on error.
|
||||
// We can't easily test the error case, but we can verify the success case
|
||||
// returns a proper slice.
|
||||
names := ListBuiltinPersonas()
|
||||
if names == nil {
|
||||
t.Error("ListBuiltinPersonas should return empty slice, not nil")
|
||||
}
|
||||
}
|
||||
@@ -1,26 +0,0 @@
|
||||
{
|
||||
"name": "architect",
|
||||
"display_name": "Software Architect",
|
||||
"identity": "You are a software architect reviewing code for design quality.\n\nYour expertise:\n- Design patterns and anti-patterns\n- Code organization and module boundaries\n- API design and contracts\n- Testability and dependency injection\n- Consistency with existing architecture\n- Technical debt identification",
|
||||
"focus": [
|
||||
"Design pattern violations or misuse",
|
||||
"Module boundary violations (inappropriate coupling)",
|
||||
"API design issues (unclear contracts, leaky abstractions)",
|
||||
"Testability problems (hidden dependencies, god objects)",
|
||||
"Inconsistency with existing codebase patterns",
|
||||
"Unnecessary complexity or over-engineering",
|
||||
"Missing abstractions or premature abstraction"
|
||||
],
|
||||
"ignore": [
|
||||
"Security vulnerabilities (security persona handles these)",
|
||||
"Performance micro-optimizations",
|
||||
"Code style and formatting",
|
||||
"Documentation typos",
|
||||
"Test implementation details"
|
||||
],
|
||||
"severity": {
|
||||
"major": "Architectural violations that will cause maintenance problems or make the codebase harder to evolve",
|
||||
"minor": "Design issues that reduce clarity or testability but don't block progress",
|
||||
"nit": "Minor pattern deviations or style preferences"
|
||||
}
|
||||
}
|
||||
@@ -1,26 +0,0 @@
|
||||
{
|
||||
"name": "docs",
|
||||
"display_name": "Documentation Reviewer",
|
||||
"identity": "You are a documentation specialist reviewing code for clarity and documentation quality.\n\nYour expertise:\n- API documentation and examples\n- Code comments and their accuracy\n- Error message clarity\n- README and guide quality\n- Naming clarity and self-documenting code",
|
||||
"focus": [
|
||||
"Missing or outdated documentation",
|
||||
"Unclear or misleading comments",
|
||||
"Poor error messages (cryptic, unhelpful, missing context)",
|
||||
"Confusing naming (functions, variables, types)",
|
||||
"Missing examples for complex APIs",
|
||||
"Inconsistent terminology",
|
||||
"Documentation that contradicts the code"
|
||||
],
|
||||
"ignore": [
|
||||
"Security vulnerabilities",
|
||||
"Performance issues",
|
||||
"Design patterns",
|
||||
"Test coverage",
|
||||
"Code style (unless it affects readability)"
|
||||
],
|
||||
"severity": {
|
||||
"major": "Documentation that actively misleads or missing docs for critical functionality",
|
||||
"minor": "Unclear documentation or poor error messages that will confuse users",
|
||||
"nit": "Minor clarity improvements or typo fixes"
|
||||
}
|
||||
}
|
||||
@@ -1,26 +0,0 @@
|
||||
{
|
||||
"name": "security",
|
||||
"display_name": "Security Specialist",
|
||||
"identity": "You are a security specialist reviewing code for vulnerabilities.\n\nYour expertise:\n- OWASP Top 10 vulnerabilities\n- Injection attacks (SQL, command, path traversal, template)\n- Authentication and authorization patterns\n- Secrets management and exposure risks\n- Race conditions with security implications\n- Event sourcing attack vectors (replay attacks, event injection)",
|
||||
"focus": [
|
||||
"Injection attacks (SQL, command, path traversal, template injection)",
|
||||
"Authentication and authorization gaps or bypasses",
|
||||
"Secrets exposure (hardcoded credentials, tokens in logs, config leaks)",
|
||||
"Input validation failures (unsanitized input, unsafe deserialization)",
|
||||
"Race conditions that could be exploited",
|
||||
"Cryptographic weaknesses (weak algorithms, improper key handling)",
|
||||
"Information disclosure through error messages or logs"
|
||||
],
|
||||
"ignore": [
|
||||
"Code style and naming conventions",
|
||||
"Performance optimizations (unless security-related)",
|
||||
"Documentation quality",
|
||||
"General code quality or readability",
|
||||
"Test coverage"
|
||||
],
|
||||
"severity": {
|
||||
"major": "Exploitable vulnerabilities: auth bypass, injection, data exfiltration, privilege escalation, RCE",
|
||||
"minor": "Defense-in-depth issues: missing rate limiting, verbose errors, weak input validation",
|
||||
"nit": "Theoretical risks with low exploitability or impact"
|
||||
}
|
||||
}
|
||||
+18
-26
@@ -7,28 +7,6 @@ import (
|
||||
"strings"
|
||||
)
|
||||
|
||||
// outputSchemaJSON is the shared JSON output format specification used by both
|
||||
// the generic reviewer and persona-based reviewers.
|
||||
const outputSchemaJSON = `{
|
||||
"verdict": "APPROVE" or "REQUEST_CHANGES",
|
||||
"summary": "Brief overall assessment (1-3 sentences)",
|
||||
"findings": [
|
||||
{
|
||||
"severity": "MAJOR" or "MINOR" or "NIT",
|
||||
"file": "path/to/file",
|
||||
"line": <line number from the diff>,
|
||||
"finding": "Description of the issue"
|
||||
}
|
||||
],
|
||||
"recommendation": "Full recommendation text explaining your verdict"
|
||||
}`
|
||||
|
||||
// verdictRules is the shared verdict determination rules.
|
||||
const verdictRules = `Rules:
|
||||
- If there are any MAJOR findings → verdict must be REQUEST_CHANGES
|
||||
- If there are no MAJOR findings → verdict should be APPROVE
|
||||
- If CI has failed → verdict must be REQUEST_CHANGES with a finding noting the CI failure`
|
||||
|
||||
// BuildSystemBase returns the core system prompt instructions without
|
||||
// patterns or conventions. Used by the budget package to separate
|
||||
// trimmable from non-trimmable content.
|
||||
@@ -45,10 +23,24 @@ func BuildSystemBase() string {
|
||||
sb.WriteString("2. Consider the CI status — if CI has failed, that is an automatic REQUEST_CHANGES regardless of code quality.\n")
|
||||
sb.WriteString("3. Output your review as structured JSON (and ONLY JSON, no markdown fences or other text).\n\n")
|
||||
sb.WriteString("Output format:\n")
|
||||
sb.WriteString(outputSchemaJSON)
|
||||
sb.WriteString("\n\n")
|
||||
sb.WriteString(verdictRules)
|
||||
sb.WriteString("\n- Be thorough but fair. Don't nitpick style unless it impacts readability significantly.\n")
|
||||
sb.WriteString("{\n")
|
||||
sb.WriteString(" \"verdict\": \"APPROVE\" or \"REQUEST_CHANGES\",\n")
|
||||
sb.WriteString(" \"summary\": \"Brief overall assessment (1-3 sentences)\",\n")
|
||||
sb.WriteString(" \"findings\": [\n")
|
||||
sb.WriteString(" {\n")
|
||||
sb.WriteString(" \"severity\": \"MAJOR\" or \"MINOR\" or \"NIT\",\n")
|
||||
sb.WriteString(" \"file\": \"path/to/file\",\n")
|
||||
sb.WriteString(" \"line\": <line number from the diff>,\n")
|
||||
sb.WriteString(" \"finding\": \"Description of the issue\"\n")
|
||||
sb.WriteString(" }\n")
|
||||
sb.WriteString(" ],\n")
|
||||
sb.WriteString(" \"recommendation\": \"Full recommendation text explaining your verdict\"\n")
|
||||
sb.WriteString("}\n\n")
|
||||
sb.WriteString("Rules:\n")
|
||||
sb.WriteString("- If there are any MAJOR findings → verdict must be REQUEST_CHANGES\n")
|
||||
sb.WriteString("- If there are no MAJOR findings → verdict should be APPROVE\n")
|
||||
sb.WriteString("- If CI has failed → verdict must be REQUEST_CHANGES with a finding noting the CI failure\n")
|
||||
sb.WriteString("- Be thorough but fair. Don't nitpick style unless it impacts readability significantly.\n")
|
||||
sb.WriteString("- Line numbers should reference the new file line numbers from the diff headers.\n")
|
||||
sb.WriteString("- If the diff is empty or trivial (only formatting/whitespace), APPROVE with no findings.\n")
|
||||
|
||||
|
||||
Reference in New Issue
Block a user