Compare commits
2 Commits
| Author | SHA1 | Date | |
|---|---|---|---|
 Rodin
|
29ab19c94d | ||
 Rodin
|
dd003c66d5 |
@@ -0,0 +1,203 @@
|
||||
# Composite action for Gitea Actions and GitHub Actions runners.
|
||||
# Supports dual-platform deployment: reviews can be triggered from GitHub (mirrored repo)
|
||||
# or Gitea, but always post results to the Gitea PR.
|
||||
# Requirements: python3, sha256sum, curl (all present on ubuntu-* runners).
|
||||
name: 'AI Code Review'
|
||||
description: 'Run AI-powered code review on a pull request using review-bot'
|
||||
|
||||
inputs:
|
||||
gitea-url:
|
||||
description: 'Gitea instance URL for API calls and releases (defaults to https://gitea.weiker.me)'
|
||||
required: false
|
||||
default: 'https://gitea.weiker.me'
|
||||
repo:
|
||||
description: 'Repository (owner/name, defaults to rodin/review-bot for version lookup)'
|
||||
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 (not required for aicore provider)'
|
||||
required: false
|
||||
default: ''
|
||||
llm-api-key:
|
||||
description: 'LLM API key (not required for aicore provider)'
|
||||
required: false
|
||||
default: ''
|
||||
llm-model:
|
||||
description: 'LLM model name'
|
||||
required: true
|
||||
llm-provider:
|
||||
description: 'LLM API provider: openai, anthropic, or aicore (default openai)'
|
||||
required: false
|
||||
default: 'openai'
|
||||
aicore-client-id:
|
||||
description: 'SAP AI Core client ID (required for aicore provider)'
|
||||
required: false
|
||||
default: ''
|
||||
aicore-client-secret:
|
||||
description: 'SAP AI Core client secret (required for aicore provider)'
|
||||
required: false
|
||||
default: ''
|
||||
aicore-auth-url:
|
||||
description: 'SAP AI Core authentication URL (required for aicore provider)'
|
||||
required: false
|
||||
default: ''
|
||||
aicore-api-url:
|
||||
description: 'SAP AI Core API URL (required for aicore provider)'
|
||||
required: false
|
||||
default: ''
|
||||
aicore-resource-group:
|
||||
description: 'SAP AI Core resource group (default: default)'
|
||||
required: false
|
||||
default: 'default'
|
||||
conventions-file:
|
||||
description: 'Path to conventions file in the repo (e.g. CLAUDE.md)'
|
||||
required: false
|
||||
default: ''
|
||||
patterns-repo:
|
||||
description: 'Comma-separated repos with language patterns (e.g. rodin/elixir-patterns,rodin/phoenix-conventions)'
|
||||
required: false
|
||||
default: ''
|
||||
patterns-files:
|
||||
description: 'Comma-separated file paths or directories to fetch from patterns repos'
|
||||
required: false
|
||||
default: 'README.md'
|
||||
temperature:
|
||||
description: 'LLM temperature (0 = server default)'
|
||||
required: false
|
||||
default: '0'
|
||||
timeout:
|
||||
description: 'LLM request timeout in seconds (default 300)'
|
||||
required: false
|
||||
default: '300'
|
||||
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'
|
||||
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'
|
||||
steps:
|
||||
- name: Determine version
|
||||
id: version
|
||||
shell: bash
|
||||
run: |
|
||||
# Use explicit gitea-url input, falling back to default (https://gitea.weiker.me)
|
||||
GITEA_URL="${{ inputs.gitea-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: |
|
||||
# Use explicit gitea-url input for release downloads
|
||||
GITEA_URL="${{ inputs.gitea-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:
|
||||
# Always use Gitea API - reviews are posted to Gitea regardless of where workflow runs
|
||||
GITHUB_SERVER_URL: ${{ inputs.gitea-url }}
|
||||
GITHUB_REPOSITORY: ${{ 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 }}
|
||||
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 }}
|
||||
AICORE_CLIENT_ID: ${{ inputs.aicore-client-id }}
|
||||
AICORE_CLIENT_SECRET: ${{ inputs.aicore-client-secret }}
|
||||
AICORE_AUTH_URL: ${{ inputs.aicore-auth-url }}
|
||||
AICORE_API_URL: ${{ inputs.aicore-api-url }}
|
||||
AICORE_RESOURCE_GROUP: ${{ inputs.aicore-resource-group }}
|
||||
run: |
|
||||
ARGS=""
|
||||
if [ "${{ inputs.dry-run }}" = "true" ]; then
|
||||
ARGS="--dry-run"
|
||||
fi
|
||||
${{ runner.temp }}/review-bot $ARGS
|
||||
@@ -0,0 +1,74 @@
|
||||
name: CI
|
||||
|
||||
on:
|
||||
push:
|
||||
branches: [main]
|
||||
pull_request:
|
||||
types: [opened, synchronize]
|
||||
|
||||
jobs:
|
||||
test:
|
||||
runs-on: ubuntu-24.04
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
- uses: actions/setup-go@v5
|
||||
with:
|
||||
go-version: '1.26'
|
||||
- run: go test ./...
|
||||
- run: go vet ./...
|
||||
- run: go build -o review-bot ./cmd/review-bot
|
||||
|
||||
# Self-review using native SAP AI Core provider
|
||||
# Models must match SAP AI Core deployments
|
||||
# Available models: gpt-5, anthropic--claude-4.6-sonnet, anthropic--claude-4.6-opus
|
||||
# Removed gpt-4.1, gpt-5-mini, gpt-4.1-mini - not deployed on AI Core
|
||||
#
|
||||
# SECURITY: This job runs on pull_request and has access to secrets.
|
||||
# We restrict to same-repo PRs only (no forks) since this is a private repo
|
||||
# where PRs only come from trusted actors (rodin/aweiker).
|
||||
review:
|
||||
runs-on: ubuntu-24.04
|
||||
if: github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository
|
||||
needs: test
|
||||
strategy:
|
||||
matrix:
|
||||
include:
|
||||
- name: sonnet
|
||||
token_secret: SONNET_REVIEW_TOKEN
|
||||
model: anthropic--claude-4.6-sonnet
|
||||
- name: gpt
|
||||
token_secret: GPT_REVIEW_TOKEN
|
||||
model: gpt-5
|
||||
- name: security
|
||||
token_secret: SECURITY_REVIEW_TOKEN
|
||||
model: gpt-5
|
||||
patterns_repo: rodin/security-patterns
|
||||
patterns_files: "."
|
||||
system_prompt_file: SECURITY_REVIEW.md
|
||||
steps:
|
||||
- uses: actions/checkout@v4
|
||||
- uses: actions/setup-go@v5
|
||||
with:
|
||||
go-version: '1.26'
|
||||
- run: go build -o review-bot ./cmd/review-bot
|
||||
- name: Run ${{ matrix.name }} review
|
||||
env:
|
||||
# Use Gitea API - reviews are posted to Gitea, not GitHub
|
||||
GITHUB_SERVER_URL: https://gitea.weiker.me
|
||||
GITHUB_REPOSITORY: rodin/review-bot
|
||||
PR_NUMBER: ${{ github.event.pull_request.number }}
|
||||
REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }}
|
||||
REVIEWER_NAME: ${{ matrix.name }}
|
||||
LLM_PROVIDER: aicore
|
||||
LLM_MODEL: ${{ matrix.model }}
|
||||
AICORE_CLIENT_ID: ${{ secrets.AICORE_CLIENT_ID }}
|
||||
AICORE_CLIENT_SECRET: ${{ secrets.AICORE_CLIENT_SECRET }}
|
||||
AICORE_AUTH_URL: ${{ secrets.AICORE_AUTH_URL }}
|
||||
AICORE_API_URL: ${{ secrets.AICORE_API_URL }}
|
||||
AICORE_RESOURCE_GROUP: ${{ secrets.AICORE_RESOURCE_GROUP }}
|
||||
CONVENTIONS_FILE: "CONVENTIONS.md"
|
||||
PATTERNS_REPO: ${{ matrix.patterns_repo || 'rodin/go-patterns' }}
|
||||
PATTERNS_FILES: ${{ matrix.patterns_files || 'README.md,patterns/' }}
|
||||
LLM_TIMEOUT: "600"
|
||||
SYSTEM_PROMPT_FILE: ${{ matrix.system_prompt_file }}
|
||||
run: ./review-bot
|
||||
@@ -0,0 +1,43 @@
|
||||
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: |
|
||||
# NOTE: This workflow runs on the GitHub mirror but updates the Gitea PR.
|
||||
# PR numbers may differ between GitHub and Gitea mirrors in edge cases.
|
||||
# For this repo, PRs are created on Gitea and mirrored, so numbers match.
|
||||
PR_NUMBER=${{ github.event.pull_request.number }}
|
||||
AUTHOR=${{ github.event.pull_request.user.login }}
|
||||
READY_LABEL_ID=38
|
||||
SELF_REVIEWED_LABEL_ID=37
|
||||
|
||||
# INTENTIONAL: Hardcoded Gitea URL because we always update the Gitea PR,
|
||||
# not GitHub. The mirror relationship means we want changes on Gitea.
|
||||
# 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}"
|
||||
@@ -0,0 +1,98 @@
|
||||
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 vet ./...
|
||||
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}
|
||||
# Releases are created on Gitea, not GitHub - use explicit Gitea URL
|
||||
GITEA_URL="https://gitea.weiker.me"
|
||||
REPO="rodin/review-bot"
|
||||
|
||||
# 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 (idempotent: delete existing asset with same name first)
|
||||
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)" \
|
||||
--data-binary "@${file}"
|
||||
done
|
||||
|
||||
echo "Release ${VERSION} created with assets"
|
||||
+1
-1
@@ -9,7 +9,7 @@
|
||||
|
||||
| Package | Use Case | Scope |
|
||||
|---------|----------|-------|
|
||||
| `github.com/goccy/go-yaml` | YAML parsing and AST inspection (subpkgs: `ast`, `parser`) | production |
|
||||
| `gopkg.in/yaml.v3` | YAML parsing (persona files, config) | production |
|
||||
| `github.com/google/go-cmp` | Test comparisons (`cmp.Diff`) | test only |
|
||||
|
||||
**Any import not in this table or the Go standard library is forbidden.**
|
||||
|
||||
+9
-18
@@ -54,8 +54,8 @@ func main() {
|
||||
logFormat := flag.String("log-format", envOrDefault("LOG_FORMAT", "text"), "Log output format: text or json")
|
||||
verbosity := flag.String("verbosity", envOrDefault("LOG_VERBOSITY", "info"), "Log verbosity: debug, info, warn, error")
|
||||
// CLI flags
|
||||
giteaURL := flag.String("gitea-url", envOrDefault("GITEA_URL", ""), "Gitea instance URL")
|
||||
repo := flag.String("repo", envOrDefault("GITEA_REPO", ""), "Repository (owner/name)")
|
||||
giteaURL := flag.String("gitea-url", envOrDefault("GITEA_URL", envOrDefault("GITHUB_SERVER_URL", "")), "Gitea instance URL")
|
||||
repo := flag.String("repo", envOrDefault("GITEA_REPO", envOrDefault("GITHUB_REPOSITORY", "")), "Repository (owner/name)")
|
||||
prNum := flag.String("pr", envOrDefault("PR_NUMBER", ""), "Pull request number")
|
||||
reviewerName := flag.String("reviewer-name", envOrDefault("REVIEWER_NAME", ""), "Reviewer display name")
|
||||
reviewerToken := flag.String("reviewer-token", envOrDefault("REVIEWER_TOKEN", ""), "Gitea token for posting review")
|
||||
@@ -65,7 +65,7 @@ func main() {
|
||||
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", ""), "Comma-separated file paths to fetch from patterns repo (empty = all files)")
|
||||
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)")
|
||||
@@ -523,25 +523,11 @@ func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, re
|
||||
// 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.
|
||||
// If patternsFiles is empty, all files from the repo root are fetched.
|
||||
func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patternsFiles string) string {
|
||||
var sb strings.Builder
|
||||
|
||||
repos := strings.Split(patternsRepo, ",")
|
||||
|
||||
// Build the list of paths to fetch
|
||||
var paths []string
|
||||
if patternsFiles == "" {
|
||||
// Empty patternsFiles means "fetch all files from repo root"
|
||||
paths = []string{""}
|
||||
} else {
|
||||
for _, p := range strings.Split(patternsFiles, ",") {
|
||||
p = strings.TrimSpace(p)
|
||||
if p != "" {
|
||||
paths = append(paths, p)
|
||||
}
|
||||
}
|
||||
}
|
||||
paths := strings.Split(patternsFiles, ",")
|
||||
|
||||
for _, repoRef := range repos {
|
||||
if ctx.Err() != nil {
|
||||
@@ -562,6 +548,11 @@ func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patt
|
||||
var repoSkippedFiles []string
|
||||
|
||||
for _, path := range paths {
|
||||
path = strings.TrimSpace(path)
|
||||
if path == "" {
|
||||
continue
|
||||
}
|
||||
|
||||
files, err := client.GetAllFilesInPath(ctx, owner, repo, path)
|
||||
if err != nil {
|
||||
slog.Warn("could not fetch patterns", "path", path, "repo", repoRef, "error", err)
|
||||
|
||||
@@ -504,52 +504,6 @@ func TestIsPatternFile(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestBuildPatternPaths verifies the path-building logic for fetchPatterns.
|
||||
// Empty patternsFiles means "fetch all from root" (represented as [""]).
|
||||
func TestBuildPatternPaths(t *testing.T) {
|
||||
buildPaths := func(patternsFiles string) []string {
|
||||
if patternsFiles == "" {
|
||||
return []string{""}
|
||||
}
|
||||
var paths []string
|
||||
for _, p := range strings.Split(patternsFiles, ",") {
|
||||
p = strings.TrimSpace(p)
|
||||
if p != "" {
|
||||
paths = append(paths, p)
|
||||
}
|
||||
}
|
||||
return paths
|
||||
}
|
||||
|
||||
tests := []struct {
|
||||
name string
|
||||
input string
|
||||
want []string
|
||||
}{
|
||||
{"empty fetches root", "", []string{""}},
|
||||
{"single file", "README.md", []string{"README.md"}},
|
||||
{"multiple files", "README.md,PATTERNS.md", []string{"README.md", "PATTERNS.md"}},
|
||||
{"trims whitespace", " foo.md , bar.md ", []string{"foo.md", "bar.md"}},
|
||||
{"skips empty between commas", "foo.md,,bar.md", []string{"foo.md", "bar.md"}},
|
||||
{"directory path", "patterns/", []string{"patterns/"}},
|
||||
}
|
||||
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
got := buildPaths(tc.input)
|
||||
if len(got) != len(tc.want) {
|
||||
t.Errorf("buildPaths(%q) = %v, want %v", tc.input, got, tc.want)
|
||||
return
|
||||
}
|
||||
for i := range got {
|
||||
if got[i] != tc.want[i] {
|
||||
t.Errorf("buildPaths(%q)[%d] = %q, want %q", tc.input, i, got[i], tc.want[i])
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestEvaluateCIStatus(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
|
||||
@@ -9,7 +9,7 @@ JSON is awkward for persona files that contain multi-line text (identity, severi
|
||||
- Backwards compatibility: existing JSON personas must continue to work
|
||||
- Security: protect against DoS via deeply nested YAML (AIKIDO-2024-10486)
|
||||
- Consistency: use `.yaml` extension (not `.yml`)
|
||||
- Library: use `github.com/goccy/go-yaml` v1.16.0+ (approved in CONVENTIONS.md); we implement custom AST-based depth/node-count checks for precise alias-aware validation
|
||||
- Library: use `gopkg.in/yaml.v3` (approved in CONVENTIONS.md) with explicit depth limiting
|
||||
|
||||
## Proposed Approach
|
||||
|
||||
@@ -33,16 +33,37 @@ func parsePersona(data []byte, source string) (*Persona, error) {
|
||||
|
||||
### YAML Parsing with Depth Protection
|
||||
|
||||
We implement a custom AST-based depth/node-count walk (`checkYAMLDepth` in
|
||||
`review/persona.go`) rather than relying on library decoder options. Key design
|
||||
decisions:
|
||||
```go
|
||||
func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
|
||||
var node yaml.Node
|
||||
dec := yaml.NewDecoder(bytes.NewReader(data))
|
||||
if err := dec.Decode(&node); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := checkYAMLDepth(&node, 0, maxDepth); err != nil {
|
||||
return err
|
||||
}
|
||||
return node.Decode(out)
|
||||
}
|
||||
|
||||
- **Library:** `github.com/goccy/go-yaml` with `ast.Node`-based traversal
|
||||
- **Dual-map tracking:** `validated` (depth-aware short-circuit) + `visiting` (cycle detection)
|
||||
- **Node-count limit:** Conservative overcounting bounds total validation work
|
||||
- **Alias-aware depth:** Aliases increment depth and are re-checked when encountered at greater depths
|
||||
func checkYAMLDepth(node *yaml.Node, depth, maxDepth int) error {
|
||||
if depth > maxDepth {
|
||||
return fmt.Errorf("YAML nesting depth exceeds maximum (%d)", maxDepth)
|
||||
}
|
||||
// Handle alias nodes by following the Alias pointer
|
||||
if node.Kind == yaml.AliasNode && node.Alias != nil {
|
||||
return checkYAMLDepth(node.Alias, depth, maxDepth)
|
||||
}
|
||||
for _, child := range node.Content {
|
||||
if err := checkYAMLDepth(child, depth+1, maxDepth); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
return nil
|
||||
}
|
||||
```
|
||||
|
||||
See `review/persona.go:checkYAMLDepth` for the authoritative implementation.
|
||||
The `gopkg.in/yaml.v3` library does not have built-in depth protection, so we implement explicit depth checking by first decoding into a `yaml.Node`, walking the tree to verify depth (including alias resolution), then decoding into the target struct.
|
||||
|
||||
## State/Data Model
|
||||
|
||||
@@ -53,7 +74,7 @@ No new state. Same `Persona` struct, just different parsing.
|
||||
| Error | Handling |
|
||||
|-------|----------|
|
||||
| Invalid YAML syntax | Return parse error with source file |
|
||||
| Deeply nested YAML | Custom AST walk (`checkYAMLDepth`) rejects before decode |
|
||||
| Deeply nested YAML | Library rejects (v1.16.0+ fix) |
|
||||
| Unknown extension | Fall back to JSON parsing |
|
||||
| Missing required fields | Validation rejects after parse |
|
||||
|
||||
|
||||
@@ -2,4 +2,4 @@ module gitea.weiker.me/rodin/review-bot
|
||||
|
||||
go 1.26.2
|
||||
|
||||
require github.com/goccy/go-yaml v1.19.2
|
||||
require gopkg.in/yaml.v3 v3.0.1
|
||||
|
||||
@@ -1,2 +1,4 @@
|
||||
github.com/goccy/go-yaml v1.19.2 h1:PmFC1S6h8ljIz6gMRBopkjP1TVT7xuwrButHID66PoM=
|
||||
github.com/goccy/go-yaml v1.19.2/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7LkFRi1kA=
|
||||
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
|
||||
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
|
||||
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
|
||||
gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
|
||||
|
||||
+38
-146
@@ -5,15 +5,12 @@ import (
|
||||
"embed"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"io"
|
||||
"os"
|
||||
"sort"
|
||||
"strings"
|
||||
"unicode/utf8"
|
||||
|
||||
"github.com/goccy/go-yaml"
|
||||
"github.com/goccy/go-yaml/ast"
|
||||
"github.com/goccy/go-yaml/parser"
|
||||
"gopkg.in/yaml.v3"
|
||||
)
|
||||
|
||||
//go:embed personas/*.yaml
|
||||
@@ -121,7 +118,9 @@ func ListBuiltinPersonas() []string {
|
||||
default:
|
||||
continue
|
||||
}
|
||||
seen[personaName] = true
|
||||
if !seen[personaName] {
|
||||
seen[personaName] = true
|
||||
}
|
||||
}
|
||||
names := make([]string, 0, len(seen))
|
||||
for name := range seen {
|
||||
@@ -143,19 +142,10 @@ func parsePersona(data []byte, source string) (*Persona, error) {
|
||||
err = unmarshalYAMLWithDepthLimit(data, &p, MaxYAMLDepth)
|
||||
} else {
|
||||
// Use json.Decoder with DisallowUnknownFields for consistency with
|
||||
// YAML's Strict() - both reject unknown fields to catch typos.
|
||||
// YAML's KnownFields(true) - both reject unknown fields to catch typos.
|
||||
dec := json.NewDecoder(bytes.NewReader(data))
|
||||
dec.DisallowUnknownFields()
|
||||
err = dec.Decode(&p)
|
||||
if err == nil {
|
||||
// Reject trailing content after the first valid JSON object.
|
||||
// Without this check, input like `{"name":"x"}garbage` would
|
||||
// silently succeed because Decoder stops after one object.
|
||||
var dummy json.RawMessage
|
||||
if err2 := dec.Decode(&dummy); err2 != io.EOF {
|
||||
err = fmt.Errorf("unexpected trailing content after JSON object")
|
||||
}
|
||||
}
|
||||
}
|
||||
if err != nil {
|
||||
return nil, fmt.Errorf("parse persona %s: %w", source, err)
|
||||
@@ -166,164 +156,70 @@ func parsePersona(data []byte, source string) (*Persona, error) {
|
||||
return &p, nil
|
||||
}
|
||||
|
||||
// unmarshalYAMLWithDepthLimit unmarshals YAML data with three safety checks:
|
||||
// - Depth limiting: rejects AST trees exceeding maxDepth to prevent stack exhaustion.
|
||||
// - Multi-document rejection: prevents silent data loss from ignored extra documents.
|
||||
// - Strict field checking: rejects unknown YAML keys to catch typos early.
|
||||
// unmarshalYAMLWithDepthLimit unmarshals YAML data with explicit depth limiting
|
||||
// and strict field checking. This protects against stack exhaustion from deeply
|
||||
// nested structures and catches typos in field names.
|
||||
// Multi-document YAML files are rejected to prevent silent data loss.
|
||||
func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
|
||||
// First pass: parse into AST to check depth limits, node counts, and
|
||||
// multi-document rejection. This prevents stack exhaustion before we
|
||||
// attempt to decode into structs.
|
||||
file, err := parser.ParseBytes(data, 0)
|
||||
if err != nil {
|
||||
// First pass: decode into a yaml.Node to check depth limits and node counts.
|
||||
// This prevents stack exhaustion before we attempt to decode into structs.
|
||||
var node yaml.Node
|
||||
dec := yaml.NewDecoder(bytes.NewReader(data))
|
||||
if err := dec.Decode(&node); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Reject empty YAML input (whitespace-only, comment-only, or truly empty files).
|
||||
// The parser returns a single doc with nil body for these cases.
|
||||
if len(file.Docs) == 0 || file.Docs[0].Body == nil {
|
||||
return fmt.Errorf("empty YAML document")
|
||||
}
|
||||
|
||||
// Reject multi-document YAML files - silently ignoring additional documents
|
||||
// could lead to confusing behavior where users think their changes take effect.
|
||||
if len(file.Docs) > 1 {
|
||||
var extra yaml.Node
|
||||
if dec.Decode(&extra) == nil {
|
||||
return fmt.Errorf("multi-document YAML is not supported; only single-document files are allowed")
|
||||
}
|
||||
|
||||
nodeCount := 0
|
||||
if err := checkYAMLDepth(file.Docs[0].Body, 0, maxDepth, MaxYAMLNodes, make(map[ast.Node]int), make(map[ast.Node]bool), &nodeCount); err != nil {
|
||||
if err := checkYAMLDepth(&node, 0, maxDepth, MaxYAMLNodes, make(map[*yaml.Node]struct{}), &nodeCount); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
// Second pass: decode with strict field checking enabled.
|
||||
// Strict() rejects unknown keys, catching typos like "focuss" or "identiy".
|
||||
//
|
||||
// Safety note: goccy/go-yaml's decoder does not expand YAML aliases
|
||||
// recursively — it resolves them via the pre-built AST, which our first
|
||||
// pass already depth-checked. Alias chains that would exceed depth limits
|
||||
// are caught above; the decoder merely reads the resolved scalar values.
|
||||
dec := yaml.NewDecoder(bytes.NewReader(data), yaml.Strict())
|
||||
return dec.Decode(out)
|
||||
// KnownFields(true) rejects unknown keys, catching typos like "focuss" or "identiy".
|
||||
// We must re-decode from the original data because yaml.Node.Decode() doesn't
|
||||
// support the KnownFields option.
|
||||
strictDec := yaml.NewDecoder(bytes.NewReader(data))
|
||||
strictDec.KnownFields(true)
|
||||
return strictDec.Decode(out)
|
||||
}
|
||||
|
||||
// checkYAMLDepth recursively checks that YAML AST nodes don't exceed the depth
|
||||
// limit or the total node count limit. It uses two tracking maps:
|
||||
// - validated: maps each node to the maximum depth at which it was previously
|
||||
// checked. If a node is revisited at a deeper depth (e.g., via an alias),
|
||||
// we re-check it to ensure the combined effective depth doesn't exceed limits.
|
||||
// - visiting: per-path recursion stack for true cycle detection. A node on the
|
||||
// current path is a cycle (alias loop); we return nil to avoid infinite recursion.
|
||||
//
|
||||
// This design prevents the alias depth bypass where an anchored subtree validated
|
||||
// at a shallow depth could be referenced via alias at a greater depth, effectively
|
||||
// exceeding MaxYAMLDepth.
|
||||
func checkYAMLDepth(node ast.Node, depth, maxDepth, maxNodes int, validated map[ast.Node]int, visiting map[ast.Node]bool, nodeCount *int) error {
|
||||
if node == nil {
|
||||
return nil
|
||||
}
|
||||
|
||||
// checkYAMLDepth recursively checks that YAML nodes don't exceed the depth limit
|
||||
// or the total node count limit. It also detects alias cycles to prevent infinite
|
||||
// recursion from crafted YAML with self-referential aliases.
|
||||
func checkYAMLDepth(node *yaml.Node, depth, maxDepth, maxNodes int, seen map[*yaml.Node]struct{}, nodeCount *int) error {
|
||||
if depth > maxDepth {
|
||||
return fmt.Errorf("YAML nesting depth exceeds maximum (%d)", maxDepth)
|
||||
}
|
||||
|
||||
// Cycle detection: if we're currently visiting this node on the current
|
||||
// recursion path, it's a cycle (e.g., alias pointing to an ancestor).
|
||||
// Return nil to break the cycle without error — cycles are a structural
|
||||
// property, not a depth violation.
|
||||
if visiting[node] {
|
||||
return nil
|
||||
}
|
||||
|
||||
// Track total nodes visited as defense-in-depth against wide-but-shallow attacks.
|
||||
// Placed after cycle detection but before the depth-aware short-circuit. This means
|
||||
// nodes revisited at shallower depths (via aliases) are counted each time they are
|
||||
// encountered — intentional conservative overcounting. This bounds the total work
|
||||
// performed during validation rather than tracking unique nodes, which is the safer
|
||||
// security posture for untrusted YAML input.
|
||||
*nodeCount++
|
||||
if *nodeCount > maxNodes {
|
||||
return fmt.Errorf("YAML node count exceeds maximum (%d)", maxNodes)
|
||||
}
|
||||
|
||||
// Depth-aware short-circuit: skip re-validation only when the current visit
|
||||
// depth is the same or shallower than the depth at which this node was
|
||||
// previously validated. A shallower (or equal) current depth means the
|
||||
// prior, deeper validation already covered any subtree depth violations.
|
||||
// If the current depth exceeds the previous validation depth (e.g., an alias
|
||||
// references this node deeper in the tree), we must re-traverse to ensure
|
||||
// the combined effective depth doesn't exceed maxDepth.
|
||||
//
|
||||
// Note: using ast.Node (interface) as map key relies on pointer identity,
|
||||
// which is correct because all goccy/go-yaml AST node types are pointer
|
||||
// receivers (*MappingNode, *SequenceNode, etc.), never value types.
|
||||
if prevDepth, ok := validated[node]; ok && depth <= prevDepth {
|
||||
return nil
|
||||
// Cycle detection: if we've seen this node before, we're in a cycle.
|
||||
if _, ok := seen[node]; ok {
|
||||
return nil // Already validated this subtree, skip to avoid infinite recursion.
|
||||
}
|
||||
validated[node] = depth
|
||||
seen[node] = struct{}{}
|
||||
|
||||
// Mark as visiting (on the current recursion path) for cycle detection.
|
||||
visiting[node] = true
|
||||
defer func() { visiting[node] = false }()
|
||||
// Handle alias nodes: follow the alias to its anchor target.
|
||||
// Increment depth when following aliases since they expand the effective structure.
|
||||
if node.Kind == yaml.AliasNode && node.Alias != nil {
|
||||
return checkYAMLDepth(node.Alias, depth+1, maxDepth, maxNodes, seen, nodeCount)
|
||||
}
|
||||
|
||||
// Walk children based on node type.
|
||||
switch n := node.(type) {
|
||||
case *ast.MappingNode:
|
||||
for _, value := range n.Values {
|
||||
if err := checkYAMLDepth(value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
case *ast.MappingValueNode:
|
||||
// Both Key and Value are visited at depth+1 relative to this
|
||||
// MappingValueNode. Since MappingNode visits its MappingValueNode
|
||||
// children at depth+1 as well, keys and values end up at depth+2
|
||||
// from the parent MappingNode. This is intentional: it mirrors the
|
||||
// actual nesting structure (mapping → key-value pair → key/value).
|
||||
if err := checkYAMLDepth(n.Key, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
|
||||
for _, child := range node.Content {
|
||||
if err := checkYAMLDepth(child, depth+1, maxDepth, maxNodes, seen, nodeCount); err != nil {
|
||||
return err
|
||||
}
|
||||
if err := checkYAMLDepth(n.Value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
|
||||
return err
|
||||
}
|
||||
case *ast.SequenceNode:
|
||||
for _, value := range n.Values {
|
||||
if err := checkYAMLDepth(value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
|
||||
return err
|
||||
}
|
||||
}
|
||||
case *ast.AliasNode:
|
||||
// Follow alias to its target, incrementing depth since aliases expand
|
||||
// the effective structure.
|
||||
if err := checkYAMLDepth(n.Value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
|
||||
return err
|
||||
}
|
||||
case *ast.AnchorNode:
|
||||
// Increment depth for anchor values as a conservative measure: the
|
||||
// anchor definition itself is structural, and treating it as a depth
|
||||
// level ensures that deeply nested anchors are caught at definition
|
||||
// time rather than only when referenced via alias. This +1 is
|
||||
// asymmetric with alias (which also increments) — by design, the
|
||||
// effective depth budget for anchored-then-aliased content is reduced
|
||||
// because both the definition site and the reference site each consume
|
||||
// a level, making deeply nested anchor/alias pairs hit the limit sooner.
|
||||
if err := checkYAMLDepth(n.Value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
|
||||
return err
|
||||
}
|
||||
case *ast.TagNode:
|
||||
if err := checkYAMLDepth(n.Value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
|
||||
return err
|
||||
}
|
||||
case *ast.MergeKeyNode:
|
||||
// MergeKeyNode represents the literal "<<" merge key token. It has no
|
||||
// child nodes — the value side of a merge (e.g., *alias) lives in the
|
||||
// parent MappingValueNode.Value, which is already recursed into above.
|
||||
// Explicitly listed here (rather than in the default case) to prevent
|
||||
// future library changes from silently bypassing depth checks.
|
||||
default:
|
||||
// Scalar leaf nodes (StringNode, IntegerNode, FloatNode, BoolNode,
|
||||
// NullNode, InfinityNode, NanNode, LiteralNode) have no children to
|
||||
// recurse into.
|
||||
}
|
||||
return nil
|
||||
}
|
||||
@@ -331,11 +227,7 @@ func checkYAMLDepth(node ast.Node, depth, maxDepth, maxNodes int, validated map[
|
||||
// ParsePersonaBytes parses persona data from bytes with a source label for errors.
|
||||
// This is useful for parsing personas fetched from external sources (e.g., Gitea API)
|
||||
// without requiring filesystem access. Format is detected by source extension.
|
||||
// Input is bounded by MaxPersonaFileSize to prevent resource exhaustion.
|
||||
func ParsePersonaBytes(data []byte, source string) (*Persona, error) {
|
||||
if len(data) > MaxPersonaFileSize {
|
||||
return nil, fmt.Errorf("persona data from %s exceeds maximum size (%d bytes, limit %d)", source, len(data), MaxPersonaFileSize)
|
||||
}
|
||||
return parsePersona(data, source)
|
||||
}
|
||||
|
||||
|
||||
+41
-222
@@ -7,7 +7,7 @@ import (
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/goccy/go-yaml/ast"
|
||||
"gopkg.in/yaml.v3"
|
||||
)
|
||||
|
||||
func TestLoadBuiltinPersona(t *testing.T) {
|
||||
@@ -459,14 +459,7 @@ func TestYAMLDeeplyNestedRejection(t *testing.T) {
|
||||
path := filepath.Join(dir, "deeply-nested.yaml")
|
||||
|
||||
// Build a deeply nested YAML structure that exceeds MaxYAMLDepth (20).
|
||||
// Depth accumulation trace for "nested: \n level0: \n level1: ...":
|
||||
// - Document root parsed at depth 0
|
||||
// - Root MappingNode children (MappingValueNodes) visited at depth 1
|
||||
// - "nested" MappingValueNode: key at depth 2, value at depth 2
|
||||
// - Each levelN adds depth via MappingValueNode traversal (key + value)
|
||||
// - Exact depth per level depends on AST structure (MappingNode wrapping),
|
||||
// but 25 levels reliably exceeds MaxYAMLDepth (20) with comfortable margin.
|
||||
// The test uses 25 levels rather than exactly 21 to avoid brittleness.
|
||||
// Each level adds 2 to the depth count (key + value mapping).
|
||||
var sb strings.Builder
|
||||
sb.WriteString("name: test\nidentity: test\nnested:\n")
|
||||
indent := " "
|
||||
@@ -490,35 +483,6 @@ func TestYAMLDeeplyNestedRejection(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestYAMLEmptyFileRejection(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
content string
|
||||
}{
|
||||
{"completely_empty", ""},
|
||||
{"whitespace_only", " \n\n "},
|
||||
{"comment_only", "# just a comment\n"},
|
||||
}
|
||||
|
||||
for _, tc := range tests {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
path := filepath.Join(dir, tc.name+".yaml")
|
||||
if err := os.WriteFile(path, []byte(tc.content), 0644); err != nil {
|
||||
t.Fatalf("failed to write test file: %v", err)
|
||||
}
|
||||
|
||||
_, err := LoadPersona(path)
|
||||
if err == nil {
|
||||
t.Fatal("expected error for empty YAML input, got nil")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "empty YAML document") {
|
||||
t.Errorf("expected error containing %q, got: %v", "empty YAML document", err)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestYAMLFileSizeLimit(t *testing.T) {
|
||||
dir := t.TempDir()
|
||||
path := filepath.Join(dir, "huge.yaml")
|
||||
@@ -540,41 +504,41 @@ func TestYAMLFileSizeLimit(t *testing.T) {
|
||||
|
||||
func TestYAMLAliasCycleDetection(t *testing.T) {
|
||||
// Test that our checkYAMLDepth function handles alias cycles gracefully
|
||||
// by using the visiting map to prevent infinite recursion.
|
||||
// by using the seen map to prevent infinite recursion.
|
||||
// We test this directly because go-yaml's parser handles most cycles
|
||||
// at parse time, but we need to ensure our checker is robust.
|
||||
|
||||
// Create a node structure where an alias points to a parent node,
|
||||
// simulating what could happen with crafted input.
|
||||
parent := &ast.MappingNode{
|
||||
Values: []*ast.MappingValueNode{
|
||||
{
|
||||
Key: &ast.StringNode{Value: "name"},
|
||||
Value: &ast.StringNode{Value: "test"},
|
||||
},
|
||||
// simulating what could happen with malicious input that bypasses
|
||||
// go-yaml's cycle detection.
|
||||
parent := &yaml.Node{
|
||||
Kind: yaml.MappingNode,
|
||||
Content: []*yaml.Node{
|
||||
{Kind: yaml.ScalarNode, Value: "name"},
|
||||
{Kind: yaml.ScalarNode, Value: "test"},
|
||||
{Kind: yaml.ScalarNode, Value: "nested"},
|
||||
},
|
||||
}
|
||||
|
||||
// Create a child that aliases back to the parent (artificial cycle)
|
||||
aliasToParent := &ast.AliasNode{
|
||||
Value: parent,
|
||||
aliasToParent := &yaml.Node{
|
||||
Kind: yaml.AliasNode,
|
||||
Alias: parent,
|
||||
}
|
||||
parent.Values = append(parent.Values, &ast.MappingValueNode{
|
||||
Key: &ast.StringNode{Value: "nested"},
|
||||
Value: aliasToParent,
|
||||
})
|
||||
parent.Content = append(parent.Content, aliasToParent)
|
||||
|
||||
nodeCount := 0
|
||||
validated := make(map[ast.Node]int)
|
||||
visiting := make(map[ast.Node]bool)
|
||||
seen := make(map[*yaml.Node]struct{})
|
||||
|
||||
// This should NOT hang or stack overflow - cycle detection prevents infinite recursion
|
||||
err := checkYAMLDepth(parent, 0, MaxYAMLDepth, MaxYAMLNodes, validated, visiting, &nodeCount)
|
||||
// This should NOT hang or stack overflow - the seen map prevents infinite recursion
|
||||
err := checkYAMLDepth(parent, 0, MaxYAMLDepth, MaxYAMLNodes, seen, &nodeCount)
|
||||
if err != nil {
|
||||
t.Errorf("unexpected error traversing cyclic structure: %v", err)
|
||||
}
|
||||
|
||||
// Verify we tracked the parent in the validated map
|
||||
if _, ok := validated[parent]; !ok {
|
||||
t.Error("parent node not tracked in validated map")
|
||||
// Verify we tracked the parent in the seen map
|
||||
if _, ok := seen[parent]; !ok {
|
||||
t.Error("parent node not tracked in seen map")
|
||||
}
|
||||
}
|
||||
|
||||
@@ -630,82 +594,36 @@ func TestYAMLNodeCountLimit(t *testing.T) {
|
||||
func TestCheckYAMLDepthCycleDetectionDirect(t *testing.T) {
|
||||
// Direct test of cycle detection in checkYAMLDepth by creating
|
||||
// a node structure with an artificial cycle.
|
||||
node := &ast.MappingNode{
|
||||
Values: []*ast.MappingValueNode{
|
||||
{
|
||||
Key: &ast.StringNode{Value: "key"},
|
||||
Value: &ast.StringNode{Value: "value"},
|
||||
},
|
||||
// This tests the seen map logic independent of go-yaml's parsing.
|
||||
node := &yaml.Node{
|
||||
Kind: yaml.MappingNode,
|
||||
Content: []*yaml.Node{
|
||||
{Kind: yaml.ScalarNode, Value: "key"},
|
||||
{Kind: yaml.ScalarNode, Value: "value"},
|
||||
},
|
||||
}
|
||||
|
||||
// Create a cycle by making a child reference the parent
|
||||
cycleChild := &ast.AliasNode{
|
||||
Value: node, // Points back to the parent
|
||||
cycleChild := &yaml.Node{
|
||||
Kind: yaml.AliasNode,
|
||||
Alias: node, // Points back to the parent
|
||||
}
|
||||
node.Values = append(node.Values, &ast.MappingValueNode{
|
||||
Key: &ast.StringNode{Value: "cyclic"},
|
||||
Value: cycleChild,
|
||||
})
|
||||
node.Content = append(node.Content,
|
||||
&yaml.Node{Kind: yaml.ScalarNode, Value: "cyclic"},
|
||||
cycleChild,
|
||||
)
|
||||
|
||||
nodeCount := 0
|
||||
validated := make(map[ast.Node]int)
|
||||
visiting := make(map[ast.Node]bool)
|
||||
err := checkYAMLDepth(node, 0, MaxYAMLDepth, MaxYAMLNodes, validated, visiting, &nodeCount)
|
||||
seen := make(map[*yaml.Node]struct{})
|
||||
err := checkYAMLDepth(node, 0, MaxYAMLDepth, MaxYAMLNodes, seen, &nodeCount)
|
||||
|
||||
// Should complete without infinite recursion due to cycle detection
|
||||
if err != nil {
|
||||
t.Errorf("unexpected error: %v", err)
|
||||
}
|
||||
// The validated map should contain multiple entries
|
||||
if len(validated) < 2 {
|
||||
t.Errorf("validated map has %d entries, expected at least 2", len(validated))
|
||||
}
|
||||
}
|
||||
|
||||
func TestYAMLAliasDepthBypass(t *testing.T) {
|
||||
// Test that an anchored subtree first validated at a shallow depth is
|
||||
// re-checked when referenced via alias at a deeper position. Without the
|
||||
// depth-aware validated map, the alias reference would skip re-checking
|
||||
// and allow the effective nesting to exceed MaxYAMLDepth.
|
||||
|
||||
dir := t.TempDir()
|
||||
path := filepath.Join(dir, "alias-depth-bypass.yaml")
|
||||
|
||||
// Build YAML with an anchor at shallow depth containing a subtree near the limit,
|
||||
// then reference it via alias deep enough that effective depth exceeds MaxYAMLDepth.
|
||||
var sb strings.Builder
|
||||
sb.WriteString("name: test\nidentity: test\n")
|
||||
|
||||
// Create the anchored subtree at depth 1 (key level) that nests 15 levels deep.
|
||||
sb.WriteString("anchor_key: &deep_anchor\n")
|
||||
for i := 0; i < 15; i++ {
|
||||
sb.WriteString(strings.Repeat(" ", i+1))
|
||||
sb.WriteString(fmt.Sprintf("level%d:\n", i))
|
||||
}
|
||||
sb.WriteString(strings.Repeat(" ", 16))
|
||||
sb.WriteString("leaf: value\n")
|
||||
|
||||
// Create a wrapper that nests 6 levels deep, then references the anchor.
|
||||
// Effective depth at alias target = 6 (wrapper nesting) + 1 (alias) + 15 (subtree) = 22 > 20
|
||||
sb.WriteString("wrapper:\n")
|
||||
for i := 0; i < 6; i++ {
|
||||
sb.WriteString(strings.Repeat(" ", i+1))
|
||||
sb.WriteString(fmt.Sprintf("n%d:\n", i))
|
||||
}
|
||||
sb.WriteString(strings.Repeat(" ", 7))
|
||||
sb.WriteString("alias_ref: *deep_anchor\n")
|
||||
|
||||
if err := os.WriteFile(path, []byte(sb.String()), 0644); err != nil {
|
||||
t.Fatalf("failed to write test file: %v", err)
|
||||
}
|
||||
|
||||
_, err := LoadPersona(path)
|
||||
if err == nil {
|
||||
t.Fatal("expected error for alias depth bypass, got nil")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "nesting depth exceeds") {
|
||||
t.Errorf("error = %q, want containing 'nesting depth exceeds'", err.Error())
|
||||
// The seen map should contain multiple entries
|
||||
if len(seen) < 2 {
|
||||
t.Errorf("seen map has %d entries, expected at least 2", len(seen))
|
||||
}
|
||||
}
|
||||
|
||||
@@ -858,102 +776,3 @@ identity: test identity
|
||||
t.Errorf("Name = %q, want %q", p.Name, "test")
|
||||
}
|
||||
}
|
||||
|
||||
func TestJSONTrailingContentRejected(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
content string
|
||||
}{
|
||||
{
|
||||
name: "trailing garbage after object",
|
||||
content: `{"name":"test","identity":"test identity"}garbage`,
|
||||
},
|
||||
{
|
||||
name: "two JSON objects",
|
||||
content: `{"name":"test","identity":"test identity"}{"name":"other"}`,
|
||||
},
|
||||
{
|
||||
name: "trailing array",
|
||||
content: `{"name":"test","identity":"test identity"}[]`,
|
||||
},
|
||||
}
|
||||
|
||||
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.content), 0644); err != nil {
|
||||
t.Fatalf("failed to write test file: %v", err)
|
||||
}
|
||||
|
||||
_, err := LoadPersona(path)
|
||||
if err == nil {
|
||||
t.Fatal("expected error for trailing content, got nil")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "trailing content") {
|
||||
t.Errorf("error = %q, want to contain 'trailing content'", err.Error())
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestParsePersonaBytesSizeLimit(t *testing.T) {
|
||||
// ParsePersonaBytes should reject input exceeding MaxPersonaFileSize
|
||||
oversized := make([]byte, MaxPersonaFileSize+1)
|
||||
for i := range oversized {
|
||||
oversized[i] = 'x'
|
||||
}
|
||||
|
||||
_, err := ParsePersonaBytes(oversized, "oversized.yaml")
|
||||
if err == nil {
|
||||
t.Fatal("expected error for oversized input, got nil")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "exceeds maximum size") {
|
||||
t.Errorf("error = %q, want to contain 'exceeds maximum size'", err.Error())
|
||||
}
|
||||
|
||||
// Just under the limit should not trigger size error (may fail parse, but not size)
|
||||
underLimit := []byte("name: test\nidentity: test persona\n")
|
||||
p, err := ParsePersonaBytes(underLimit, "valid.yaml")
|
||||
if err != nil {
|
||||
t.Fatalf("unexpected error for valid input: %v", err)
|
||||
}
|
||||
if p.Name != "test" {
|
||||
t.Errorf("Name = %q, want %q", p.Name, "test")
|
||||
}
|
||||
}
|
||||
|
||||
func TestYAMLMergeKeyDepthCheck(t *testing.T) {
|
||||
// Verify that YAML merge keys (<<: *alias) are properly handled by the
|
||||
// depth checker. The merge key content is in the MappingValueNode.Value
|
||||
// (an AliasNode), not in the MergeKeyNode itself.
|
||||
p, err := ParsePersonaBytes([]byte("name: merge-test\nidentity: test\n"), "merge.yaml")
|
||||
if err != nil {
|
||||
t.Fatalf("basic parse failed: %v", err)
|
||||
}
|
||||
if p.Name != "merge-test" {
|
||||
t.Errorf("Name = %q, want %q", p.Name, "merge-test")
|
||||
}
|
||||
|
||||
// Test that deeply nested merge keys still hit depth limit.
|
||||
// Build YAML with merge key content nested beyond MaxYAMLDepth.
|
||||
var sb strings.Builder
|
||||
sb.WriteString("name: deep-merge\nidentity: deep merge persona\n")
|
||||
sb.WriteString("anchor: &deep\n")
|
||||
indent := " "
|
||||
for i := 0; i < MaxYAMLDepth+5; i++ {
|
||||
sb.WriteString(indent)
|
||||
sb.WriteString(fmt.Sprintf("level%d:\n", i))
|
||||
indent += " "
|
||||
}
|
||||
sb.WriteString(indent + "leaf: value\n")
|
||||
sb.WriteString("target:\n <<: *deep\n")
|
||||
|
||||
_, err = ParsePersonaBytes([]byte(sb.String()), "deep-merge.yaml")
|
||||
if err == nil {
|
||||
t.Fatal("expected error for deeply nested merge key content, got nil")
|
||||
}
|
||||
if !strings.Contains(err.Error(), "depth") {
|
||||
t.Errorf("error = %q, want to contain 'depth'", err.Error())
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user