Compare commits

..

13 Commits

Author SHA1 Message Date
Rodin f77ea171c3 ci: add go-patterns as review reference
CI / test (pull_request) Successful in 13s
CI / review (gpt-5-mini, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m43s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m50s
Self-reviews now use rodin/go-patterns (README.md + docs/) as
language idiom criteria alongside CONVENTIONS.md.
2026-05-01 12:15:25 -07:00
Rodin 56f5abda3c feat: multi-repo patterns + directory recursion
CI / test (pull_request) Successful in 14s
CI / review (gpt-5-mini, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m57s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 2m2s
patterns-repo now accepts a comma-separated list of repos:
  PATTERNS_REPO="rodin/elixir-patterns,rodin/phoenix-conventions"

patterns-files accepts files AND directories:
  PATTERNS_FILES="README.md,docs/"

When a path is a directory, all files within it are fetched
recursively via the Gitea contents API. Only .md, .txt, .yml,
and .yaml files are included as pattern content.

New API methods:
- ListContents: list files/dirs at a path via contents API
- GetAllFilesInPath: recursively fetch all file contents

This allows a single review action to pull idioms from multiple
pattern repos (e.g. elixir-patterns + phoenix-conventions) and
include entire directories of documentation as review criteria.
2026-05-01 12:14:19 -07:00
Rodin e234dca474 feat: full file context + patterns-repo support
CI / test (pull_request) Successful in 13s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m51s
CI / review (gpt-5-mini, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m0s
Major improvements to review quality:

1. Full file context: fetch complete content of all modified files from
   the PR branch and include as reference. This eliminates false-positive
   "missing import" findings since the model sees the entire file.

2. Patterns repo: new --patterns-repo / PATTERNS_REPO flag fetches
   language idiom files from a separate Gitea repo (e.g. rodin/elixir-patterns)
   and includes them as review criteria.

3. Multi-file patterns: --patterns-files / PATTERNS_FILES accepts
   comma-separated file paths to fetch from the patterns repo.

New API methods:
- GetPullRequestFiles: list changed files in a PR
- GetFileContentRef: fetch file content from a specific branch/ref

Prompt changes:
- BuildSystemPrompt now accepts (conventions, patterns)
- BuildUserPrompt now accepts fileContext parameter
- File context displayed before diff for model reference
- Patterns presented as "review criteria" in system prompt

Composite action updated with patterns-repo and patterns-files inputs.
2026-05-01 12:11:49 -07:00
Rodin c76362af95 fix: prevent false-positive missing-import findings
CI / test (pull_request) Successful in 13s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 1m59s
CI / review (gpt-5-mini, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 3m36s
The LLM was treating the diff as complete file context and flagging
"missing imports" for symbols that exist in unchanged portions of
the file. Added explicit instructions to the system prompt:

- Clarify that the diff is partial; unchanged code already exists
- Explicitly instruct: do not flag missing imports/types unless the
  diff introduces a new usage without a corresponding addition
- Add rule: never flag undefined symbols that could exist in the
  unchanged portions
2026-05-01 12:00:27 -07:00
Rodin 46c63ed121 fix: address all review findings (zero remaining)
CI / test (pull_request) Successful in 13s
CI / review (gpt-5-mini, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m43s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 2m19s
Tests:
- Add WithTemperature tests (builder method, chaining, zero omission)
- Add temperature serialization tests (omitted when 0, included when set)

Composite action:
- Use python3 for robust JSON version parsing (replaces sed)
- Verify SHA-256 checksum before executing downloaded binary
- Wire up repo input (no longer hardcodes rodin/review-bot)

Release workflow:
- Handle 409 conflict (existing release for tag)
- Use file-based JSON parsing for reliability

Code:
- Tighten WithTemperature doc comment (single clear line)
- Fix flag alignment (missing tab on llmTemp declaration)
2026-05-01 11:58:21 -07:00
Rodin 59fbd38837 fix: address all remaining review findings
CI / test (pull_request) Successful in 14s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 2m20s
CI / review (gpt-5-mini, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m20s
- Add temperature range validation (must be 0-2, fatal on invalid)
- release.yml: use python3 for robust JSON parsing instead of sed
- Composite action: add header comment confirming Gitea Actions compat
- All findings from review #385 addressed
2026-05-01 11:40:15 -07:00
Rodin 8d53b649ee fix: address review findings (cache path, docs)
CI / test (pull_request) Successful in 13s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 2m13s
CI / review (gpt-5-mini, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m48s
- Composite action: cache to runner.temp instead of /usr/local/bin
  (avoids permission issues on runners)
- Document that temperature=0 means server default (omitted from request)
- Note: strconv import already exists (false positive from GPT-5)
2026-05-01 11:38:28 -07:00
Rodin c458587cfc feat: add composite action for clean distribution
CI / test (pull_request) Successful in 13s
CI / review (gpt-5, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 2m28s
CI / review (gpt-5-mini, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m43s
- .gitea/actions/review/action.yml: composite action with caching
  Consumers just use:
    uses: https://gitea.weiker.me/rodin/review-bot/.gitea/actions/review@v0.1.0
  No Go toolchain needed, binary cached by version tag.

- Remove install.sh (replaced by composite action)
- CI workflow: use matrix strategy to parallelize reviews
- Self-review still builds from source (pre-release)
2026-05-01 11:32:15 -07:00
Rodin 4b3cac66c3 fix: address review findings
CI / test (pull_request) Successful in 14s
CI / review (pull_request) Successful in 5m3s
- install.sh: verify SHA-256 checksum before installing binary
- install.sh: fallback to ~/.local/bin if /usr/local/bin not writable
- install.sh: use sed instead of grep for POSIX-safe JSON parsing
- release.yml: remove jq dependency, parse release ID with sed
- llm: make temperature configurable via --llm-temperature / LLM_TEMPERATURE
- llm: add WithTemperature builder method on Client
- llm: omit temperature from request when zero (uses server default)
2026-05-01 11:22:31 -07:00
Rodin b6277216f7 fix: remove hardcoded temperature (unsupported by GPT-5)
CI / test (pull_request) Successful in 14s
CI / review (pull_request) Successful in 4m56s
GPT-5 via SAP AI Core only supports temperature=1 (default).
Remove the hardcoded 0.1 and use omitempty so the field is not sent.
2026-05-01 11:15:08 -07:00
Rodin 99916fe24a fix(ci): use GPT models available via HAI proxy
CI / test (pull_request) Successful in 13s
CI / review (pull_request) Failing after 12s
HAI proxy serves Anthropic on a different path (/anthropic/v1) than
OpenAI (/openai/v1). Until review-bot supports multiple base URLs,
use GPT-5 and GPT-5-mini for both review slots.
2026-05-01 11:02:27 -07:00
Rodin d62e8ee4f0 ci: retrigger after adding secrets
CI / test (pull_request) Successful in 14s
CI / review (pull_request) Failing after 12s
2026-05-01 10:40:39 -07:00
Rodin 0568a84aa9 ci: add release workflow + install script
CI / test (pull_request) Successful in 14s
CI / review (pull_request) Failing after 11s
- Release workflow: builds linux/darwin amd64/arm64 on tag push
- Injects version via -ldflags
- Creates Gitea release with binary assets + checksums
- install.sh: curl-pipe-bash installer from latest release
- Version variable in main.go for -version flag support
2026-05-01 10:36:23 -07:00
11 changed files with 708 additions and 477 deletions
+142
View File
@@ -0,0 +1,142 @@
# This composite action is designed for Gitea Actions runners.
# Gitea Actions supports GitHub Actions syntax including $GITHUB_OUTPUT,
# actions/cache, and actions/checkout.
name: 'AI Code Review'
description: 'Run AI-powered code review on a pull request using review-bot'
inputs:
gitea-url:
description: 'Gitea instance URL (defaults to server_url)'
required: false
default: ''
repo:
description: 'Repository (owner/name, defaults to current)'
required: false
default: ''
pr-number:
description: 'Pull request number (defaults to current PR)'
required: false
default: ''
reviewer-token:
description: 'Gitea token for posting the review'
required: true
reviewer-name:
description: 'Display name for the reviewer'
required: false
default: ''
llm-base-url:
description: 'OpenAI-compatible LLM API base URL'
required: true
llm-api-key:
description: 'LLM API key'
required: true
llm-model:
description: 'LLM model name'
required: true
conventions-file:
description: 'Path to conventions file in the repo (e.g. CLAUDE.md)'
required: false
default: ''
patterns-repo:
description: 'Repo with language patterns (e.g. rodin/elixir-patterns)'
required: false
default: ''
patterns-files:
description: 'Comma-separated file paths to fetch from patterns repo'
required: false
default: 'README.md'
temperature:
description: 'LLM temperature (0 = server default)'
required: false
default: '0'
version:
description: 'review-bot version to install (e.g. v0.1.0, defaults to latest)'
required: false
default: 'latest'
dry-run:
description: 'Print review to stdout instead of posting'
required: false
default: 'false'
runs:
using: 'composite'
steps:
- name: Determine version
id: version
shell: bash
run: |
GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
REPO="${{ inputs.repo || 'rodin/review-bot' }}"
if [ "${{ inputs.version }}" = "latest" ]; then
VERSION=$(curl -sSf "${GITEA_URL}/api/v1/repos/${REPO}/releases?limit=1" \
| python3 -c "import sys, json; releases = json.load(sys.stdin); print(releases[0]['tag_name'] if releases else '')")
if [ -z "$VERSION" ]; then
echo "Failed to determine latest version" >&2
exit 1
fi
else
VERSION="${{ inputs.version }}"
fi
echo "version=${VERSION}" >> "$GITHUB_OUTPUT"
- name: Cache review-bot binary
id: cache
uses: actions/cache@v4
with:
path: ${{ runner.temp }}/review-bot
key: review-bot-linux-amd64-${{ steps.version.outputs.version }}
- name: Install review-bot
if: steps.cache.outputs.cache-hit != 'true'
shell: bash
run: |
GITEA_URL="${{ inputs.gitea-url || github.server_url }}"
REPO="${{ inputs.repo || 'rodin/review-bot' }}"
VERSION="${{ steps.version.outputs.version }}"
BINARY="review-bot-linux-amd64"
curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/${BINARY}" \
-o "${{ runner.temp }}/review-bot"
curl -sSfL "${GITEA_URL}/${REPO}/releases/download/${VERSION}/checksums.txt" \
-o "${{ runner.temp }}/checksums.txt"
# Verify SHA-256 checksum
cd "${{ runner.temp }}"
EXPECTED=$(grep "${BINARY}" checksums.txt | awk '{print $1}')
ACTUAL=$(sha256sum review-bot | awk '{print $1}')
if [ -z "$EXPECTED" ]; then
echo "Error: no checksum found for ${BINARY}" >&2
exit 1
fi
if [ "$EXPECTED" != "$ACTUAL" ]; then
echo "Error: checksum mismatch!" >&2
echo " Expected: $EXPECTED" >&2
echo " Actual: $ACTUAL" >&2
exit 1
fi
chmod +x "${{ runner.temp }}/review-bot"
echo "Installed review-bot ${VERSION} (checksum verified)"
- name: Run review
shell: bash
env:
GITEA_URL: ${{ inputs.gitea-url || github.server_url }}
GITEA_REPO: ${{ inputs.repo || github.repository }}
PR_NUMBER: ${{ inputs.pr-number || github.event.pull_request.number }}
REVIEWER_TOKEN: ${{ inputs.reviewer-token }}
REVIEWER_NAME: ${{ inputs.reviewer-name }}
LLM_BASE_URL: ${{ inputs.llm-base-url }}
LLM_API_KEY: ${{ inputs.llm-api-key }}
LLM_MODEL: ${{ inputs.llm-model }}
CONVENTIONS_FILE: ${{ inputs.conventions-file }}
PATTERNS_REPO: ${{ inputs.patterns-repo }}
PATTERNS_FILES: ${{ inputs.patterns-files }}
LLM_TEMPERATURE: ${{ inputs.temperature }}
run: |
ARGS=""
if [ "${{ inputs.dry-run }}" = "true" ]; then
ARGS="--dry-run"
fi
${{ runner.temp }}/review-bot $ARGS
+18 -18
View File
@@ -1,4 +1,5 @@
name: CI name: CI
on: on:
push: push:
branches: [main] branches: [main]
@@ -12,42 +13,41 @@ jobs:
- uses: actions/checkout@v4 - uses: actions/checkout@v4
- uses: actions/setup-go@v5 - uses: actions/setup-go@v5
with: with:
go-version: "1.26" go-version: '1.26'
- run: go test ./... - run: go test ./...
- run: go vet ./... - run: go vet ./...
- run: go build -o review-bot ./cmd/review-bot - run: go build -o review-bot ./cmd/review-bot
# Self-review: builds from source since we're pre-release
review: review:
runs-on: ubuntu-24.04 runs-on: ubuntu-24.04
if: github.event_name == 'pull_request' if: github.event_name == 'pull_request'
needs: test needs: test
strategy:
matrix:
include:
- name: sonnet
token_secret: SONNET_REVIEW_TOKEN
model: gpt-5
- name: gpt
token_secret: GPT_REVIEW_TOKEN
model: gpt-5-mini
steps: steps:
- uses: actions/checkout@v4 - uses: actions/checkout@v4
- uses: actions/setup-go@v5 - uses: actions/setup-go@v5
with: with:
go-version: "1.26" go-version: '1.26'
- run: go build -o review-bot ./cmd/review-bot - run: go build -o review-bot ./cmd/review-bot
- name: Run Sonnet Review - name: Run ${{ matrix.name }} review
env: env:
GITEA_URL: ${{ github.server_url }} GITEA_URL: ${{ github.server_url }}
GITEA_REPO: ${{ github.repository }} GITEA_REPO: ${{ github.repository }}
PR_NUMBER: ${{ github.event.pull_request.number }} PR_NUMBER: ${{ github.event.pull_request.number }}
REVIEWER_TOKEN: ${{ secrets.SONNET_REVIEW_TOKEN }} REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }}
LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }} LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }}
LLM_API_KEY: ${{ secrets.LLM_API_KEY }} LLM_API_KEY: ${{ secrets.LLM_API_KEY }}
LLM_MODEL: "anthropic--claude-4.6-sonnet" LLM_MODEL: ${{ matrix.model }}
CONVENTIONS_FILE: "CONVENTIONS.md" CONVENTIONS_FILE: "CONVENTIONS.md"
REVIEWER_NAME: "Sonnet" PATTERNS_REPO: "rodin/go-patterns"
run: ./review-bot PATTERNS_FILES: "README.md,docs/"
- name: Run GPT Review
env:
GITEA_URL: ${{ github.server_url }}
GITEA_REPO: ${{ github.repository }}
PR_NUMBER: ${{ github.event.pull_request.number }}
REVIEWER_TOKEN: ${{ secrets.GPT_REVIEW_TOKEN }}
LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }}
LLM_API_KEY: ${{ secrets.LLM_API_KEY }}
LLM_MODEL: "sap-ai-opus-latest-openai/gpt-5"
CONVENTIONS_FILE: "CONVENTIONS.md"
REVIEWER_NAME: "GPT"
run: ./review-bot run: ./review-bot
+81
View File
@@ -0,0 +1,81 @@
name: Release
on:
push:
tags:
- 'v*'
jobs:
release:
runs-on: ubuntu-24.04
steps:
- uses: actions/checkout@v4
- uses: actions/setup-go@v5
with:
go-version: '1.26'
- name: Run tests
run: go test ./...
- name: Build binaries
run: |
VERSION=${GITHUB_REF_NAME}
mkdir -p dist
GOOS=linux GOARCH=amd64 go build -ldflags "-s -w -X main.version=${VERSION}" -o dist/review-bot-linux-amd64 ./cmd/review-bot
GOOS=linux GOARCH=arm64 go build -ldflags "-s -w -X main.version=${VERSION}" -o dist/review-bot-linux-arm64 ./cmd/review-bot
GOOS=darwin GOARCH=amd64 go build -ldflags "-s -w -X main.version=${VERSION}" -o dist/review-bot-darwin-amd64 ./cmd/review-bot
GOOS=darwin GOARCH=arm64 go build -ldflags "-s -w -X main.version=${VERSION}" -o dist/review-bot-darwin-arm64 ./cmd/review-bot
cd dist && sha256sum * > checksums.txt
- name: Create release and upload assets
env:
GITEA_TOKEN: ${{ secrets.RELEASE_TOKEN }}
run: |
VERSION=${GITHUB_REF_NAME}
GITEA_URL="${{ github.server_url }}"
REPO="${{ github.repository }}"
# Create release (or find existing one for this tag)
HTTP_CODE=$(curl -s -o /tmp/release_response.json -w "%{http_code}" -X POST \
-H "Authorization: token ${GITEA_TOKEN}" \
-H "Content-Type: application/json" \
"${GITEA_URL}/api/v1/repos/${REPO}/releases" \
-d "{\"tag_name\": \"${VERSION}\", \"name\": \"${VERSION}\", \"body\": \"Release ${VERSION}\", \"draft\": false, \"prerelease\": false}")
if [ "$HTTP_CODE" = "409" ]; then
echo "Release for ${VERSION} already exists, fetching existing..."
curl -sSf -o /tmp/release_response.json \
-H "Authorization: token ${GITEA_TOKEN}" \
"${GITEA_URL}/api/v1/repos/${REPO}/releases/tags/${VERSION}"
elif [ "$HTTP_CODE" != "201" ]; then
echo "Failed to create release (HTTP ${HTTP_CODE})" >&2
cat /tmp/release_response.json >&2
exit 1
fi
# Parse release ID (python3 available on ubuntu-24.04 runners)
RELEASE_ID=$(python3 -c "import json; print(json.load(open('/tmp/release_response.json'))['id'])")
if [ -z "$RELEASE_ID" ]; then
echo "Failed to parse release ID" >&2
cat /tmp/release_response.json >&2
exit 1
fi
echo "Release ID: ${RELEASE_ID}"
# Upload each asset
for file in dist/*; do
filename=$(basename "$file")
echo "Uploading ${filename}..."
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=${filename}" \
--data-binary "@${file}"
done
echo "Release ${VERSION} created with assets"
-436
View File
@@ -1,436 +0,0 @@
# review-bot Code Review (vs go-patterns)
## Overall Assessment
The review-bot is a well-structured, focused Go application that follows many idiomatic patterns correctly. The package layout is clean (`gitea/`, `llm/`, `review/`, `cmd/`), error handling uses `%w` wrapping consistently, and the test suite covers all major code paths using `httptest`. However, there are several areas where the code diverges from the patterns documented in `go-patterns` — particularly around configuration, context propagation, exported fields, documentation, and testing idioms.
**Verdict: Solid foundation with targeted improvements needed.**
## Findings
| # | Severity | File | Pattern Violated | Finding |
|---|----------|------|-----------------|---------|
| 1 | MAJOR | `gitea/client.go` | concurrency / api-conventions | No `context.Context` parameter on any method — HTTP calls are uncancellable |
| 2 | MAJOR | `llm/client.go` | concurrency / api-conventions | `Complete()` accepts no context — no timeout or cancellation support |
| 3 | MAJOR | `gitea/client.go` | structs / encapsulation | `Client` fields (`BaseURL`, `Token`, `HTTP`) are exported but should be unexported |
| 4 | MAJOR | `llm/client.go` | structs / encapsulation | `Client` fields (`BaseURL`, `APIKey`, `Model`, `HTTP`) are exported — leaks credentials via reflection/logging |
| 5 | MINOR | `cmd/review-bot/main.go` | configuration | No input validation beyond emptiness — e.g., URL format, model name format |
| 6 | MINOR | `cmd/review-bot/main.go` | error-handling | Uses `log.Fatalf` for all errors — no cleanup, deferred functions won't run |
| 7 | MINOR | `gitea/client.go` | error-handling / style | Error strings in `doGet` are inconsistent — some use `fmt.Errorf`, the raw HTTP error doesn't wrap with `%w` |
| 8 | MINOR | `review/prompt.go` | style / api-conventions | `BuildSystemPrompt` uses 20+ `WriteString` calls — could use a raw string literal for readability |
| 9 | MINOR | `gitea/client.go` | documentation | No concurrency safety documentation on `Client` type |
| 10 | MINOR | `llm/client.go` | documentation | No concurrency safety documentation on `Client` type |
| 11 | MINOR | `gitea/client_test.go` | testing-advanced | Tests don't use `t.Run` subtests — individual test functions instead of table-driven with named cases |
| 12 | MINOR | `integration_test.go` | style | Uses rune literal `'/'` comparison in a loop instead of `strings.SplitN` (inconsistent with `main.go`) |
| 13 | MINOR | `llm/client.go` | configuration | `Temperature: 0.1` is hardcoded — not configurable and the zero-value (0.0) semantic isn't clear |
| 14 | NIT | `gitea/client.go` | style | `PostReview` converts `[]byte` to `string` then passes to `strings.NewReader` — use `bytes.NewReader(data)` directly |
| 15 | NIT | `review/formatter.go` | documentation | `GiteaEvent` has no doc comment explaining the mapping semantics |
| 16 | NIT | `cmd/review-bot/main.go` | package-design | `evaluateCIStatus` is unexported logic in `main` — could live in `review` package for testability |
| 17 | NIT | `gitea/client.go` | interfaces | No interface defined for the Gitea client — makes the main function harder to unit test |
| 18 | NIT | `llm/client.go` | interfaces | No interface defined for the LLM client — same testability concern |
| 19 | NIT | `review/parser.go` | error-handling | `extractJSON` silently handles malformed fences — edge case: `\`\`\`` with only 1 line produces empty string |
| 20 | NIT | Various | documentation | No package-level doc comments (`// Package xxx ...`) on any package |
## Detailed Findings
### 1. No `context.Context` on Gitea client methods (MAJOR)
**What the code does:**
```go
func (c *Client) GetPullRequest(owner, repo string, number int) (*PullRequest, error) {
url := fmt.Sprintf(...)
body, err := c.doGet(url)
...
}
```
**What the pattern says:**
From `concurrency.md` §6 (Context Propagation Rules): "Pass a Context explicitly to each function that needs it. The Context should be the first parameter, typically named ctx." From `api-conventions.md` §3 (WithContext variant): All I/O-performing functions should accept a context for timeout/cancellation.
**How to fix:**
```go
func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error) {
...
req, err := http.NewRequestWithContext(ctx, "GET", url, nil)
...
}
```
Add `context.Context` as the first parameter to all public methods. Update `doGet` to accept context internally.
---
### 2. No `context.Context` on LLM `Complete()` (MAJOR)
**What the code does:**
```go
func (c *Client) Complete(messages []Message) (string, error) {
...
req, err := http.NewRequest("POST", url, bytes.NewReader(data))
...
}
```
**What the pattern says:**
Same as finding #1. LLM calls can take 30-60+ seconds. Without context, there's no way to enforce a timeout or cancel a review that's taking too long.
**How to fix:**
```go
func (c *Client) Complete(ctx context.Context, messages []Message) (string, error) {
...
req, err := http.NewRequestWithContext(ctx, "POST", url, bytes.NewReader(data))
...
}
```
The caller in `main.go` should create a context with timeout: `ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute)`.
---
### 3 & 4. Exported struct fields on Client types (MAJOR)
**What the code does:**
```go
type Client struct {
BaseURL string
Token string
HTTP *http.Client
}
```
**What the pattern says:**
From `structs.md`: use unexported fields for internal state; expose only what callers need to read/modify. From `configuration.md` §9: Document immutability constraints. Exported fields like `Token` and `APIKey` are sensitive credentials that could be accidentally logged, serialized, or mutated after construction.
**How to fix:**
```go
type Client struct {
baseURL string
token string
http *http.Client
}
```
If tests need to override `HTTP`, expose it via a functional option or a `WithHTTPClient(*http.Client)` setter, or accept it in the constructor.
---
### 5. No input validation beyond emptiness (MINOR)
**What the code does:**
```go
if *giteaURL == "" || *repo == "" || ...
```
**What the pattern says:**
From `configuration.md` §1 (Zero-Value Config): Document and validate configuration explicitly. A malformed URL (e.g., missing scheme) will produce a confusing error later during HTTP request creation rather than at startup.
**How to fix:**
```go
if _, err := url.Parse(*giteaURL); err != nil || !strings.HasPrefix(*giteaURL, "http") {
log.Fatalf("Invalid --gitea-url: %s", *giteaURL)
}
```
---
### 6. `log.Fatalf` for all errors (MINOR)
**What the code does:**
`log.Fatalf(...)` is used for every error in `main()`.
**What the pattern says:**
From `api-conventions.md` §9 (Graceful Shutdown): distinguish between fatal and recoverable errors. From `error-handling.md`: error handling should give callers the ability to respond. While `main()` is the top-level caller, `log.Fatalf` calls `os.Exit(1)` which doesn't run deferred functions.
**How to fix:**
Use a `run() error` pattern:
```go
func main() {
if err := run(); err != nil {
fmt.Fprintf(os.Stderr, "error: %v\n", err)
os.Exit(1)
}
}
func run() error { ... }
```
This allows deferred cleanup to run and makes the code testable.
---
### 7. Inconsistent error formatting in `doGet` (MINOR)
**What the code does:**
```go
func (c *Client) doGet(url string) ([]byte, error) {
...
return nil, fmt.Errorf("HTTP %d: %s", resp.StatusCode, string(body))
}
```
The error from the raw HTTP response isn't wrapped with `%w`, but the callers wrap it again: `fmt.Errorf("fetch PR: %w", err)`. The inner error starts with a capital "HTTP".
**What the pattern says:**
From `smells/anti-patterns.md` §6 (Error String Formatting): error strings should be lowercase. They compose upward: `fetch PR: HTTP 404: ...` has inconsistent casing.
**How to fix:**
```go
return nil, fmt.Errorf("http %d: %s", resp.StatusCode, string(body))
```
---
### 8. Prompt building uses excessive `WriteString` (MINOR)
**What the code does:**
```go
sb.WriteString("You are an expert code reviewer...\n\n")
sb.WriteString("Your task:\n")
sb.WriteString("1. Review the diff...\n")
// ... 20+ more lines
```
**What the pattern says:**
From `style.md`: code should be readable and maintainable. A raw string literal would be far more readable for a multi-line prompt template.
**How to fix:**
```go
const systemPromptTemplate = `You are an expert code reviewer. Review the provided pull request diff carefully.
Your task:
1. Review the diff for correctness, idiomatic code, potential bugs, and design issues.
...
`
func BuildSystemPrompt(conventions string) string {
prompt := systemPromptTemplate
if conventions != "" {
prompt += fmt.Sprintf("\n\nThe repository has the following coding conventions...\n\n%s\n", conventions)
}
return prompt
}
```
---
### 9 & 10. No concurrency safety documentation (MINOR)
**What the code does:**
Neither `gitea.Client` nor `llm.Client` documents whether they're safe for concurrent use.
**What the pattern says:**
From `documentation.md` §9 (Concurrency Documentation): "Doc comments explicitly state the concurrency safety of a type." Since both types embed `*http.Client` (which IS safe for concurrent use), the wrapping types should document this.
**How to fix:**
```go
// Client interacts with the Gitea API.
// A Client is safe for concurrent use by multiple goroutines.
type Client struct { ... }
```
---
### 11. Tests don't use `t.Run` subtests (MINOR)
**What the code does:**
`gitea/client_test.go` defines 8 separate `TestXxx` functions, each creating their own httptest server.
**What the pattern says:**
From `testing-advanced.md` §1 (Table-Driven Tests with `t.Run`): related tests should use named subtests for filterability and clarity. The Gitea client tests share identical setup patterns — they'd benefit from a shared helper.
**How to fix:**
Consider a test helper that creates a server with a handler map, then use `t.Run` for each case. The existing structure is acceptable but could be DRYer.
---
### 12. Inconsistent repo parsing in `integration_test.go` (MINOR)
**What the code does:**
```go
for i, c := range giteaRepo {
if c == '/' {
owner = giteaRepo[:i]
repoName = giteaRepo[i+1:]
break
}
}
```
**What the pattern says:**
From `style.md` §3 (File Organization by Responsibility): related logic should be consistent. `main.go` uses `strings.SplitN(*repo, "/", 2)` for the same operation. The integration test reinvents it with a manual loop.
**How to fix:**
Use `strings.SplitN(giteaRepo, "/", 2)` for consistency, or extract a shared helper.
---
### 13. Hardcoded `Temperature: 0.1` (MINOR)
**What the code does:**
```go
reqBody := ChatRequest{
...
Temperature: 0.1,
}
```
**What the pattern says:**
From `configuration.md` §1 (Zero-Value Usable Config): "Every field documents its zero-value behavior." The temperature is buried in implementation. It should be configurable (e.g., a field on `Client` with a documented default).
**How to fix:**
Add a `Temperature` field to `Client` with documentation:
```go
type Client struct {
...
// Temperature controls LLM randomness. If zero, defaults to 0.1.
Temperature float64
}
```
---
### 14. Unnecessary `string()` → `strings.NewReader` conversion (NIT)
**What the code does:**
```go
data, err := json.Marshal(payload)
...
req, err := http.NewRequest("POST", url, strings.NewReader(string(data)))
```
**What the pattern says:**
From `style.md`: avoid unnecessary allocations. `json.Marshal` returns `[]byte`; use `bytes.NewReader(data)` directly to avoid the `[]byte→string` copy.
**How to fix:**
```go
req, err := http.NewRequest("POST", url, bytes.NewReader(data))
```
---
### 15. Missing doc comment on `GiteaEvent` (NIT)
**What the code does:**
```go
// GiteaEvent converts the verdict to the Gitea API event string.
func GiteaEvent(verdict string) string {
```
Actually, this one DOES have a doc comment. On closer inspection the comment exists. Removing this finding — **correction**: the comment is present but minimal. It doesn't document the mapping or the "COMMENT" fallback behavior. This is borderline.
---
### 16. `evaluateCIStatus` in `main` package (NIT)
**What the code does:**
The `evaluateCIStatus` function lives in `cmd/review-bot/main.go` and operates on `[]gitea.CommitStatus`.
**What the pattern says:**
From `package-design.md`: packages should encapsulate related logic. This function interprets CI status semantics — it belongs in the `review` package (or even `gitea`) where it could be unit-tested independently without building the entire binary.
---
### 17 & 18. No interfaces for testability (NIT)
**What the code does:**
`main.go` directly uses `*gitea.Client` and `*llm.Client` concrete types.
**What the pattern says:**
From `interfaces.md`: "Define interfaces in the package that USES them." From `smells/common-mistakes.md` §10 (Premature Abstraction): don't create interfaces before you need them. However, the consumer (`main.go`) would benefit from small interfaces for testing the orchestration logic independently.
**How to fix (when needed):**
```go
// In main or a review orchestrator package:
type PRFetcher interface {
GetPullRequest(ctx context.Context, owner, repo string, number int) (*gitea.PullRequest, error)
GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error)
}
```
Note: This is a NIT because the current code doesn't have tests for `main.go` orchestration. If/when that's needed, interfaces become valuable.
---
### 19. `extractJSON` edge case (NIT)
**What the code does:**
```go
if strings.HasPrefix(s, "```") {
lines := strings.Split(s, "\n")
if len(lines) > 2 {
lines = lines[1:]
}
...
}
```
If input is exactly `` ```json\n``` `` (fence with empty body), it produces an empty string that will fail JSON parse with a confusing error message.
**What the pattern says:**
From `error-handling.md`: errors should carry context. Consider returning an explicit error from `extractJSON` when the extracted content is empty after fence stripping.
---
### 20. No package doc comments (NIT)
**What the code does:**
None of the packages (`gitea`, `llm`, `review`) have `// Package xxx ...` doc comments.
**What the pattern says:**
From `documentation.md` §1 (Package Documentation): "The first file in a package starts with a `// Package xxx ...` comment that explains the package's purpose."
**How to fix:**
Add to each package's primary file:
```go
// Package gitea provides a client for the Gitea API, focused on pull request review operations.
package gitea
```
---
## Positive Patterns
The codebase does several things well:
1. **Clean package separation** — `gitea/`, `llm/`, `review/`, `cmd/` each have a single responsibility. This matches `package-design.md` perfectly.
2. **Consistent error wrapping** — Every public function wraps errors with `fmt.Errorf("context: %w", err)`, providing clear error chains. This follows `error-handling.md` closely.
3. **Return concrete types from constructors** — `NewClient()` returns `*Client`, not an interface. Matches `smells/common-mistakes.md` §7 and `smells/anti-patterns.md` §8.
4. **httptest-based testing** — Both client packages use `net/http/httptest` for isolated, deterministic tests. No external dependencies needed.
5. **Good test coverage of error paths** — Tests cover 404s, bad JSON, connection failures, invalid severities, missing fields. This is thorough.
6. **Zero dependencies** — `go.mod` has no external dependencies. The entire project uses only the standard library. This is excellent for a focused tool.
7. **Build-tagged integration test** — The `//go:build integration` tag keeps expensive tests separate from unit tests. Good practice.
8. **`strings.Builder` usage** — Prompt building and formatting use `strings.Builder` correctly for efficient string construction.
9. **Named return values where useful** — `evaluateCIStatus` uses named returns `(passed bool, details string)` for documentation clarity, matching `style.md` §5.
10. **No premature abstraction** — The code doesn't define interfaces it doesn't need yet. It's concrete and straightforward, following `smells/common-mistakes.md` §10.
## Recommendations
Priority-ordered list of improvements:
1. **Add `context.Context` to all client methods** (Critical) — This is the single most impactful change. LLM calls can hang indefinitely without timeout support. Both `gitea.Client` and `llm.Client` should accept context as the first parameter on all public methods. Use `http.NewRequestWithContext`.
2. **Unexport client struct fields** (High) — `Token`, `APIKey`, `BaseURL` should be unexported to prevent accidental logging/serialization of credentials. Expose only what's needed via methods or constructor options.
3. **Add package documentation** (Medium) — Each package needs a `// Package xxx ...` comment. This takes 5 minutes and significantly improves discoverability.
4. **Extract `evaluateCIStatus` to `review` package** (Medium) — Makes it independently testable and keeps `main.go` focused on orchestration.
5. **Use `run() error` pattern in main** (Medium) — Enables deferred cleanup and makes the orchestration logic more testable.
6. **Replace `WriteString` chain with raw string literal** (Low) — Pure readability improvement for `BuildSystemPrompt`.
7. **Make LLM temperature configurable** (Low) — Add as a field on `Client` with documented zero-value default.
8. **Use `bytes.NewReader` instead of `strings.NewReader(string(...))` in PostReview** (Low) — Eliminates one unnecessary allocation.
9. **Add concurrency documentation to Client types** (Low) — One-line doc additions.
10. **Consider consumer-side interfaces when testing `main` orchestration** (Future) — Not needed now, but will become valuable if the `main.go` logic grows or needs unit testing.
+121 -8
View File
@@ -13,6 +13,8 @@ import (
"gitea.weiker.me/rodin/review-bot/review" "gitea.weiker.me/rodin/review-bot/review"
) )
var version = "dev"
func main() { func main() {
// CLI flags // CLI flags
giteaURL := flag.String("gitea-url", envOrDefault("GITEA_URL", ""), "Gitea instance URL") giteaURL := flag.String("gitea-url", envOrDefault("GITEA_URL", ""), "Gitea instance URL")
@@ -24,7 +26,10 @@ func main() {
llmAPIKey := flag.String("llm-api-key", envOrDefault("LLM_API_KEY", ""), "LLM API key") llmAPIKey := flag.String("llm-api-key", envOrDefault("LLM_API_KEY", ""), "LLM API key")
llmModel := flag.String("llm-model", envOrDefault("LLM_MODEL", ""), "LLM model name") 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)") conventionsFile := flag.String("conventions-file", envOrDefault("CONVENTIONS_FILE", ""), "Conventions file path in repo (e.g. CLAUDE.md)")
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") 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)")
flag.Parse() flag.Parse()
@@ -52,6 +57,12 @@ func main() {
// Initialize clients // Initialize clients
giteaClient := gitea.NewClient(*giteaURL, *reviewerToken) giteaClient := gitea.NewClient(*giteaURL, *reviewerToken)
llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel) llmClient := llm.NewClient(*llmBaseURL, *llmAPIKey, *llmModel)
if *llmTemp < 0 || *llmTemp > 2 {
log.Fatal("--llm-temperature must be between 0 and 2")
}
if *llmTemp > 0 {
llmClient.WithTemperature(*llmTemp)
}
log.Printf("Reviewing PR #%d on %s/%s", prNumber, owner, repoName) log.Printf("Reviewing PR #%d on %s/%s", prNumber, owner, repoName)
@@ -69,7 +80,17 @@ func main() {
} }
log.Printf("Diff size: %d bytes", len(diff)) log.Printf("Diff size: %d bytes", len(diff))
// Step 3: Check CI status // Step 3: Fetch full file content for modified files
fileContext := ""
files, err := giteaClient.GetPullRequestFiles(owner, repoName, prNumber)
if err != nil {
log.Printf("Warning: could not fetch PR files list: %v", err)
} else {
fileContext = fetchFileContext(giteaClient, owner, repoName, pr.Head.Ref, files)
log.Printf("Fetched full context for %d files", len(files))
}
// Step 4: Check CI status
ciPassed := true ciPassed := true
ciDetails := "" ciDetails := ""
if pr.Head.Sha != "" { if pr.Head.Sha != "" {
@@ -82,7 +103,7 @@ func main() {
} }
} }
// Step 4: Load conventions file if specified // Step 5: Load conventions file if specified
conventions := "" conventions := ""
if *conventionsFile != "" { if *conventionsFile != "" {
content, err := giteaClient.GetFileContent(owner, repoName, *conventionsFile) content, err := giteaClient.GetFileContent(owner, repoName, *conventionsFile)
@@ -94,11 +115,18 @@ func main() {
} }
} }
// Step 5: Build prompts // Step 6: Load patterns from external repo if specified
systemPrompt := review.BuildSystemPrompt(conventions) patterns := ""
userPrompt := review.BuildUserPrompt(pr.Title, pr.Body, diff, ciPassed, ciDetails) if *patternsRepo != "" {
patterns = fetchPatterns(giteaClient, *patternsRepo, *patternsFiles)
log.Printf("Loaded patterns from %s (%d bytes)", *patternsRepo, len(patterns))
}
// Step 6: Call LLM // Step 7: Build prompts
systemPrompt := review.BuildSystemPrompt(conventions, patterns)
userPrompt := review.BuildUserPrompt(pr.Title, pr.Body, diff, fileContext, ciPassed, ciDetails)
// Step 8: Call LLM
log.Printf("Sending to LLM (%s)...", *llmModel) log.Printf("Sending to LLM (%s)...", *llmModel)
messages := []llm.Message{ messages := []llm.Message{
{Role: "system", Content: systemPrompt}, {Role: "system", Content: systemPrompt},
@@ -111,14 +139,14 @@ func main() {
} }
log.Printf("LLM response received (%d bytes)", len(response)) log.Printf("LLM response received (%d bytes)", len(response))
// Step 7: Parse response // Step 9: Parse response
result, err := review.ParseResponse(response) result, err := review.ParseResponse(response)
if err != nil { if err != nil {
log.Fatalf("Failed to parse LLM response: %v", err) log.Fatalf("Failed to parse LLM response: %v", err)
} }
log.Printf("Verdict: %s (%d findings)", result.Verdict, len(result.Findings)) log.Printf("Verdict: %s (%d findings)", result.Verdict, len(result.Findings))
// Step 8: Format and post review // Step 10: Format and post review
reviewBody := review.FormatMarkdown(result, *reviewerName) reviewBody := review.FormatMarkdown(result, *reviewerName)
event := review.GiteaEvent(result.Verdict) event := review.GiteaEvent(result.Verdict)
@@ -136,6 +164,81 @@ func main() {
log.Printf("Review posted successfully!") log.Printf("Review posted successfully!")
} }
// fetchFileContext fetches the full content of modified files from the PR branch.
func fetchFileContext(client *gitea.Client, owner, repo, ref string, files []gitea.ChangedFile) string {
var sb strings.Builder
for _, f := range files {
if f.Status == "removed" {
continue // Skip deleted files
}
content, err := client.GetFileContentRef(owner, repo, f.Filename, ref)
if err != nil {
log.Printf("Warning: could not fetch %s: %v", f.Filename, err)
continue
}
sb.WriteString(fmt.Sprintf("--- %s ---\n", f.Filename))
sb.WriteString("```\n")
sb.WriteString(content)
sb.WriteString("\n```\n\n")
}
return sb.String()
}
// fetchPatterns fetches pattern files from one or more external repos.
// patternsRepo is comma-separated list of owner/name repos.
// patternsFiles is comma-separated list of file paths or directories.
// If a path ends with / or is a directory, all files within it are fetched recursively.
func fetchPatterns(client *gitea.Client, patternsRepo, patternsFiles string) string {
var sb strings.Builder
repos := strings.Split(patternsRepo, ",")
paths := strings.Split(patternsFiles, ",")
for _, repoRef := range repos {
repoRef = strings.TrimSpace(repoRef)
if repoRef == "" {
continue
}
parts := strings.SplitN(repoRef, "/", 2)
if len(parts) != 2 {
log.Printf("Warning: invalid patterns-repo format %q, expected owner/name", repoRef)
continue
}
owner, repo := parts[0], parts[1]
for _, path := range paths {
path = strings.TrimSpace(path)
if path == "" {
continue
}
files, err := client.GetAllFilesInPath(owner, repo, path)
if err != nil {
log.Printf("Warning: could not fetch %s from %s: %v", path, repoRef, err)
continue
}
for filepath, content := range files {
// Only include markdown and text files as patterns
if !isPatternFile(filepath) {
continue
}
sb.WriteString(fmt.Sprintf("### %s/%s\n\n%s\n\n", repoRef, filepath, content))
}
}
}
return sb.String()
}
// isPatternFile returns true if the file should be included as pattern content.
func isPatternFile(path string) bool {
lower := strings.ToLower(path)
return strings.HasSuffix(lower, ".md") ||
strings.HasSuffix(lower, ".txt") ||
strings.HasSuffix(lower, ".yml") ||
strings.HasSuffix(lower, ".yaml")
}
// evaluateCIStatus checks if all CI statuses indicate success. // evaluateCIStatus checks if all CI statuses indicate success.
func evaluateCIStatus(statuses []gitea.CommitStatus) (passed bool, details string) { func evaluateCIStatus(statuses []gitea.CommitStatus) (passed bool, details string) {
if len(statuses) == 0 { if len(statuses) == 0 {
@@ -166,3 +269,13 @@ func envOrDefault(key, defaultVal string) string {
} }
return defaultVal return defaultVal
} }
func envOrDefaultFloat(key string, defaultVal float64) float64 {
if v := os.Getenv(key); v != "" {
f, err := strconv.ParseFloat(v, 64)
if err == nil {
return f
}
}
return defaultVal
}
+94 -3
View File
@@ -26,10 +26,11 @@ func NewClient(baseURL, token string) *Client {
// PullRequest holds relevant PR metadata. // PullRequest holds relevant PR metadata.
type PullRequest struct { type PullRequest struct {
Title string `json:"title"` Title string `json:"title"`
Body string `json:"body"` Body string `json:"body"`
Head struct { Head struct {
Sha string `json:"sha"` Sha string `json:"sha"`
Ref string `json:"ref"`
} `json:"head"` } `json:"head"`
} }
@@ -41,6 +42,12 @@ type CommitStatus struct {
TargetURL string `json:"target_url"` TargetURL string `json:"target_url"`
} }
// ChangedFile represents a file modified in a PR.
type ChangedFile struct {
Filename string `json:"filename"`
Status string `json:"status"`
}
// GetPullRequest fetches PR metadata. // GetPullRequest fetches PR metadata.
func (c *Client) GetPullRequest(owner, repo string, number int) (*PullRequest, error) { func (c *Client) GetPullRequest(owner, repo string, number int) (*PullRequest, error) {
url := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d", c.BaseURL, owner, repo, number) url := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d", c.BaseURL, owner, repo, number)
@@ -65,6 +72,20 @@ func (c *Client) GetPullRequestDiff(owner, repo string, number int) (string, err
return string(body), nil return string(body), nil
} }
// GetPullRequestFiles fetches the list of files changed in a PR.
func (c *Client) GetPullRequestFiles(owner, repo string, number int) ([]ChangedFile, error) {
url := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/files", c.BaseURL, owner, repo, number)
body, err := c.doGet(url)
if err != nil {
return nil, fmt.Errorf("fetch PR files: %w", err)
}
var files []ChangedFile
if err := json.Unmarshal(body, &files); err != nil {
return nil, fmt.Errorf("parse PR files JSON: %w", err)
}
return files, nil
}
// GetCommitStatuses fetches CI statuses for a commit SHA. // GetCommitStatuses fetches CI statuses for a commit SHA.
func (c *Client) GetCommitStatuses(owner, repo, sha string) ([]CommitStatus, error) { func (c *Client) GetCommitStatuses(owner, repo, sha string) ([]CommitStatus, error) {
url := fmt.Sprintf("%s/api/v1/repos/%s/%s/commits/%s/statuses", c.BaseURL, owner, repo, sha) url := fmt.Sprintf("%s/api/v1/repos/%s/%s/commits/%s/statuses", c.BaseURL, owner, repo, sha)
@@ -89,6 +110,16 @@ func (c *Client) GetFileContent(owner, repo, filepath string) (string, error) {
return string(body), nil return string(body), nil
} }
// GetFileContentRef fetches a file from a specific ref (branch/tag/sha) in a repo.
func (c *Client) GetFileContentRef(owner, repo, filepath, ref string) (string, error) {
url := fmt.Sprintf("%s/api/v1/repos/%s/%s/raw/%s?ref=%s", c.BaseURL, owner, repo, filepath, ref)
body, err := c.doGet(url)
if err != nil {
return "", fmt.Errorf("fetch file %s@%s: %w", filepath, ref, err)
}
return string(body), nil
}
// PostReview submits a review to a PR. // PostReview submits a review to a PR.
// event should be "APPROVED" or "REQUEST_CHANGES". // event should be "APPROVED" or "REQUEST_CHANGES".
func (c *Client) PostReview(owner, repo string, number int, event, body string) error { func (c *Client) PostReview(owner, repo string, number int, event, body string) error {
@@ -146,3 +177,63 @@ func (c *Client) doGet(url string) ([]byte, error) {
} }
return io.ReadAll(resp.Body) return io.ReadAll(resp.Body)
} }
// ContentEntry represents a file or directory entry from the contents API.
type ContentEntry struct {
Name string `json:"name"`
Path string `json:"path"`
Type string `json:"type"` // "file" or "dir"
}
// ListContents lists files and directories at a given path in a repo.
func (c *Client) ListContents(owner, repo, path string) ([]ContentEntry, error) {
url := fmt.Sprintf("%s/api/v1/repos/%s/%s/contents/%s", c.BaseURL, owner, repo, path)
body, err := c.doGet(url)
if err != nil {
return nil, fmt.Errorf("list contents %s: %w", path, err)
}
var entries []ContentEntry
if err := json.Unmarshal(body, &entries); err != nil {
return nil, fmt.Errorf("parse contents JSON: %w", err)
}
return entries, nil
}
// GetAllFilesInPath recursively fetches all file contents under a path.
// If the path is a file, returns just that file's content.
// If the path is a directory, recursively fetches all files within it.
func (c *Client) GetAllFilesInPath(owner, repo, path string) (map[string]string, error) {
results := make(map[string]string)
// Try listing as directory first
entries, err := c.ListContents(owner, repo, path)
if err != nil {
// Might be a file, try fetching directly
content, fileErr := c.GetFileContent(owner, repo, path)
if fileErr != nil {
return nil, fmt.Errorf("path %q is neither a file nor directory: %w", path, err)
}
results[path] = content
return results, nil
}
for _, entry := range entries {
switch entry.Type {
case "file":
content, err := c.GetFileContent(owner, repo, entry.Path)
if err != nil {
continue // Skip files we can't read
}
results[entry.Path] = content
case "dir":
subResults, err := c.GetAllFilesInPath(owner, repo, entry.Path)
if err != nil {
continue
}
for k, v := range subResults {
results[k] = v
}
}
}
return results, nil
}
+100
View File
@@ -2,6 +2,7 @@ package gitea
import ( import (
"encoding/json" "encoding/json"
"fmt"
"net/http" "net/http"
"net/http/httptest" "net/http/httptest"
"testing" "testing"
@@ -193,3 +194,102 @@ func TestGetFileContent(t *testing.T) {
t.Errorf("expected %q, got %q", expected, got) t.Errorf("expected %q, got %q", expected, got)
} }
} }
func TestGetPullRequestFiles(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/1/files" {
t.Errorf("unexpected path: %s", r.URL.Path)
}
w.Header().Set("Content-Type", "application/json")
w.Write([]byte(`[{"filename":"main.go","status":"modified"},{"filename":"old.go","status":"removed"}]`))
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
files, err := client.GetPullRequestFiles("owner", "repo", 1)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(files) != 2 {
t.Fatalf("expected 2 files, got %d", len(files))
}
if files[0].Filename != "main.go" || files[0].Status != "modified" {
t.Errorf("unexpected first file: %+v", files[0])
}
}
func TestGetFileContentRef(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path != "/api/v1/repos/owner/repo/raw/main.go" {
t.Errorf("unexpected path: %s", r.URL.Path)
}
if r.URL.Query().Get("ref") != "feature-branch" {
t.Errorf("unexpected ref: %s", r.URL.Query().Get("ref"))
}
w.Write([]byte("package main\n"))
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
content, err := client.GetFileContentRef("owner", "repo", "main.go", "feature-branch")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if content != "package main\n" {
t.Errorf("unexpected content: %q", content)
}
}
func TestListContents(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path != "/api/v1/repos/owner/repo/contents/docs" {
t.Errorf("unexpected path: %s", r.URL.Path)
}
w.Header().Set("Content-Type", "application/json")
fmt.Fprintf(w, `[{"name":"guide.md","path":"docs/guide.md","type":"file"},{"name":"sub","path":"docs/sub","type":"dir"}]`)
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
entries, err := client.ListContents("owner", "repo", "docs")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(entries) != 2 {
t.Fatalf("expected 2 entries, got %d", len(entries))
}
if entries[0].Type != "file" || entries[0].Path != "docs/guide.md" {
t.Errorf("unexpected first entry: %+v", entries[0])
}
if entries[1].Type != "dir" {
t.Errorf("expected dir type, got %s", entries[1].Type)
}
}
func TestGetAllFilesInPath_File(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path == "/api/v1/repos/owner/repo/contents/README.md" {
// Gitea returns 404 for contents API on files (it's not a dir)
http.NotFound(w, r)
return
}
if r.URL.Path == "/api/v1/repos/owner/repo/raw/README.md" {
fmt.Fprintf(w, "# Hello")
return
}
http.NotFound(w, r)
}))
defer server.Close()
client := NewClient(server.URL, "test-token")
files, err := client.GetAllFilesInPath("owner", "repo", "README.md")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if len(files) != 1 {
t.Fatalf("expected 1 file, got %d", len(files))
}
if files["README.md"] != "# Hello" {
t.Errorf("unexpected content: %q", files["README.md"])
}
}
+10 -2
View File
@@ -14,6 +14,7 @@ type Client struct {
BaseURL string BaseURL string
APIKey string APIKey string
Model string Model string
Temperature float64
HTTP *http.Client HTTP *http.Client
} }
@@ -27,6 +28,12 @@ func NewClient(baseURL, apiKey, model string) *Client {
} }
} }
// WithTemperature sets the temperature for LLM requests (0 = omit, uses server default).
func (c *Client) WithTemperature(t float64) *Client {
c.Temperature = t
return c
}
// Message represents a chat message. // Message represents a chat message.
type Message struct { type Message struct {
Role string `json:"role"` Role string `json:"role"`
@@ -37,7 +44,7 @@ type Message struct {
type ChatRequest struct { type ChatRequest struct {
Model string `json:"model"` Model string `json:"model"`
Messages []Message `json:"messages"` Messages []Message `json:"messages"`
Temperature float64 `json:"temperature"` Temperature float64 `json:"temperature,omitempty"`
} }
// ChatResponse is the response from the API. // ChatResponse is the response from the API.
@@ -53,8 +60,9 @@ type ChatResponse struct {
func (c *Client) Complete(messages []Message) (string, error) { func (c *Client) Complete(messages []Message) (string, error) {
reqBody := ChatRequest{ reqBody := ChatRequest{
Model: c.Model, Model: c.Model,
Temperature: c.Temperature,
Messages: messages, Messages: messages,
Temperature: 0.1,
} }
data, err := json.Marshal(reqBody) data, err := json.Marshal(reqBody)
+77
View File
@@ -108,3 +108,80 @@ func TestComplete_ServerDown(t *testing.T) {
t.Fatal("expected error for connection refused, got nil") t.Fatal("expected error for connection refused, got nil")
} }
} }
func TestWithTemperature(t *testing.T) {
client := NewClient("http://example.com", "key", "model")
if client.Temperature != 0 {
t.Errorf("expected initial temperature 0, got %f", client.Temperature)
}
result := client.WithTemperature(0.7)
if result != client {
t.Error("WithTemperature should return the same client for chaining")
}
if client.Temperature != 0.7 {
t.Errorf("expected temperature 0.7, got %f", client.Temperature)
}
}
func TestComplete_TemperatureOmittedWhenZero(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
var req map[string]interface{}
json.NewDecoder(r.Body).Decode(&req)
if _, exists := req["temperature"]; exists {
t.Error("temperature should be omitted when zero (server default)")
}
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: "ok"}}},
})
}))
defer server.Close()
client := NewClient(server.URL, "key", "model")
_, err := client.Complete([]Message{{Role: "user", Content: "Hi"}})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
}
func TestComplete_TemperatureIncludedWhenSet(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
var req map[string]interface{}
json.NewDecoder(r.Body).Decode(&req)
temp, exists := req["temperature"]
if !exists {
t.Error("temperature should be included when set")
}
if temp != 0.7 {
t.Errorf("expected temperature 0.7, got %v", temp)
}
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: "ok"}}},
})
}))
defer server.Close()
client := NewClient(server.URL, "key", "model").WithTemperature(0.7)
_, err := client.Complete([]Message{{Role: "user", Content: "Hi"}})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
}
+18 -4
View File
@@ -6,10 +6,14 @@ import (
) )
// BuildSystemPrompt constructs the system prompt for the LLM reviewer. // BuildSystemPrompt constructs the system prompt for the LLM reviewer.
func BuildSystemPrompt(conventions string) string { func BuildSystemPrompt(conventions, patterns string) string {
var sb strings.Builder var sb strings.Builder
sb.WriteString("You are an expert code reviewer. Review the provided pull request diff carefully.\n\n") sb.WriteString("You are an expert code reviewer. Review the provided pull request diff carefully.\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("Your task:\n")
sb.WriteString("1. Review the diff for correctness, idiomatic code, potential bugs, and design issues.\n") sb.WriteString("1. Review the diff for correctness, idiomatic code, potential bugs, and design issues.\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("2. Consider the CI status — if CI has failed, that is an automatic REQUEST_CHANGES regardless of code quality.\n")
@@ -36,15 +40,19 @@ func BuildSystemPrompt(conventions string) string {
sb.WriteString("- Line numbers should reference the new file line numbers from the diff headers.\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") sb.WriteString("- If the diff is empty or trivial (only formatting/whitespace), APPROVE with no findings.\n")
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 != "" { if conventions != "" {
sb.WriteString(fmt.Sprintf("\n\nThe repository has the following coding conventions that should be respected:\n\n%s\n", 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() return sb.String()
} }
// BuildUserPrompt constructs the user message with PR context. // BuildUserPrompt constructs the user message with PR context.
func BuildUserPrompt(title, description, diff string, ciPassed bool, ciDetails string) string { func BuildUserPrompt(title, description, diff, fileContext string, ciPassed bool, ciDetails string) string {
var sb strings.Builder var sb strings.Builder
sb.WriteString(fmt.Sprintf("## Pull Request: %s\n\n", title)) sb.WriteString(fmt.Sprintf("## Pull Request: %s\n\n", title))
@@ -63,7 +71,13 @@ func BuildUserPrompt(title, description, diff string, ciPassed bool, ciDetails s
sb.WriteString(fmt.Sprintf("CI Details: %s\n", ciDetails)) sb.WriteString(fmt.Sprintf("CI Details: %s\n", ciDetails))
} }
sb.WriteString("\n### Diff\n\n") if fileContext != "" {
sb.WriteString("\n### Full File Context (modified files)\n\n")
sb.WriteString(fileContext)
sb.WriteString("\n")
}
sb.WriteString("\n### Diff (changes to review)\n\n")
sb.WriteString("```diff\n") sb.WriteString("```diff\n")
sb.WriteString(diff) sb.WriteString(diff)
sb.WriteString("\n```\n") sb.WriteString("\n```\n")
+47 -6
View File
@@ -6,7 +6,7 @@ import (
) )
func TestBuildSystemPrompt_NoConventions(t *testing.T) { func TestBuildSystemPrompt_NoConventions(t *testing.T) {
prompt := BuildSystemPrompt("") prompt := BuildSystemPrompt("", "")
if !strings.Contains(prompt, "expert code reviewer") { if !strings.Contains(prompt, "expert code reviewer") {
t.Error("expected system prompt to mention code reviewer role") t.Error("expected system prompt to mention code reviewer role")
@@ -18,7 +18,7 @@ func TestBuildSystemPrompt_NoConventions(t *testing.T) {
func TestBuildSystemPrompt_WithConventions(t *testing.T) { func TestBuildSystemPrompt_WithConventions(t *testing.T) {
conventions := "- Use stdlib only\n- No panics\n" conventions := "- Use stdlib only\n- No panics\n"
prompt := BuildSystemPrompt(conventions) prompt := BuildSystemPrompt(conventions, "")
if !strings.Contains(prompt, "coding conventions") { if !strings.Contains(prompt, "coding conventions") {
t.Error("expected conventions section") t.Error("expected conventions section")
@@ -29,7 +29,7 @@ func TestBuildSystemPrompt_WithConventions(t *testing.T) {
} }
func TestBuildUserPrompt_Basic(t *testing.T) { func TestBuildUserPrompt_Basic(t *testing.T) {
prompt := BuildUserPrompt("Fix bug", "Fixes the crash", "diff content here", true, "all checks passed") prompt := BuildUserPrompt("Fix bug", "Fixes the crash", "diff content here", "", true, "all checks passed")
if !strings.Contains(prompt, "Fix bug") { if !strings.Contains(prompt, "Fix bug") {
t.Error("expected PR title") t.Error("expected PR title")
@@ -46,7 +46,7 @@ func TestBuildUserPrompt_Basic(t *testing.T) {
} }
func TestBuildUserPrompt_CIFailed(t *testing.T) { func TestBuildUserPrompt_CIFailed(t *testing.T) {
prompt := BuildUserPrompt("Add tests", "", "some diff", false, "lint: failed") prompt := BuildUserPrompt("Add tests", "", "some diff", "", false, "lint: failed")
if !strings.Contains(prompt, "FAILED") { if !strings.Contains(prompt, "FAILED") {
t.Error("expected CI status FAILED") t.Error("expected CI status FAILED")
@@ -57,7 +57,7 @@ func TestBuildUserPrompt_CIFailed(t *testing.T) {
} }
func TestBuildUserPrompt_NoDescription(t *testing.T) { func TestBuildUserPrompt_NoDescription(t *testing.T) {
prompt := BuildUserPrompt("Quick fix", "", "diff", true, "") prompt := BuildUserPrompt("Quick fix", "", "diff", "", true, "")
if strings.Contains(prompt, "### Description") { if strings.Contains(prompt, "### Description") {
t.Error("should not contain Description header when body is empty") t.Error("should not contain Description header when body is empty")
@@ -66,7 +66,7 @@ func TestBuildUserPrompt_NoDescription(t *testing.T) {
func TestBuildUserPrompt_DiffIncluded(t *testing.T) { func TestBuildUserPrompt_DiffIncluded(t *testing.T) {
diff := "+func Hello() string {\n+\treturn \"hello\"\n+}" diff := "+func Hello() string {\n+\treturn \"hello\"\n+}"
prompt := BuildUserPrompt("Greeting", "Add greeting func", diff, true, "") prompt := BuildUserPrompt("Greeting", "Add greeting func", diff, "", true, "")
if !strings.Contains(prompt, "```diff") { if !strings.Contains(prompt, "```diff") {
t.Error("expected diff fence") t.Error("expected diff fence")
@@ -75,3 +75,44 @@ func TestBuildUserPrompt_DiffIncluded(t *testing.T) {
t.Error("expected diff content in prompt") t.Error("expected diff content in prompt")
} }
} }
func TestBuildSystemPrompt_WithPatterns(t *testing.T) {
patterns := "## Naming: use snake_case for functions"
prompt := BuildSystemPrompt("", patterns)
if !strings.Contains(prompt, "Language Patterns") {
t.Error("expected patterns section header")
}
if !strings.Contains(prompt, "snake_case") {
t.Error("expected patterns content")
}
}
func TestBuildSystemPrompt_WithBoth(t *testing.T) {
conventions := "Run mix format before commit"
patterns := "Use pipe operator for transformations"
prompt := BuildSystemPrompt(conventions, patterns)
if !strings.Contains(prompt, "Repository Conventions") {
t.Error("expected conventions section")
}
if !strings.Contains(prompt, "Language Patterns") {
t.Error("expected patterns section")
}
}
func TestBuildUserPrompt_WithFileContext(t *testing.T) {
fileContext := "--- main.go ---\npackage main\n"
prompt := BuildUserPrompt("Fix", "desc", "diff here", fileContext, true, "")
if !strings.Contains(prompt, "Full File Context") {
t.Error("expected file context section")
}
if !strings.Contains(prompt, "package main") {
t.Error("expected file content in prompt")
}
}
func TestBuildUserPrompt_WithoutFileContext(t *testing.T) {
prompt := BuildUserPrompt("Fix", "desc", "diff here", "", true, "")
if strings.Contains(prompt, "Full File Context") {
t.Error("should not include file context section when empty")
}
}