fix(vcs): address PR #83 review findings (round 2)

- Extract named HeadRef and UserInfo structs from anonymous structs
  in PullRequest and Review (comments 16615, 16616)
- Change ReviewEventApprove value from "APPROVED" to "APPROVE" to
  represent the action, not the state; document adapter translation
  responsibility (comment 16621)
- Add doc comment on ReviewComment.CommitID noting optionality (16531)
- Move compile-time assertion from check.go (//go:build ignore) to
  check_test.go with a "phase2" build tag — removes gitea adapter
  import from the vcs package (comment 16622)
- check.go misleading comment was already fixed in prior commit (16532, 16539)
- Sha→SHA, typed ReviewEvent, duplicate package doc already resolved (16537, 16538, 16530)

.github/actions/review/action.yml

@@ -0,0 +1,200 @@
+# This composite action is designed for Gitea Actions runners.
+# Gitea Actions supports GitHub Actions syntax including $GITHUB_OUTPUT,
+# actions/cache, and actions/checkout.
+# Requirements: python3, sha256sum, curl (all present on ubuntu-* runners).
+name: 'AI Code Review'
+description: 'Run AI-powered code review on a pull request using review-bot'
+
+inputs:
+  gitea-url:
+    description: 'Gitea instance URL (defaults to server_url)'
+    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 (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: |
+        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:
+        GITHUB_SERVER_URL: ${{ inputs.gitea-url || github.server_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

.github/workflows/ci.yml

@@ -0,0 +1,69 @@
+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
+  review:
+    runs-on: ubuntu-24.04
+    if: github.event_name == 'pull_request'
+    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:
+          GITHUB_SERVER_URL: ${{ github.server_url }}
+          GITHUB_REPOSITORY: ${{ github.repository }}
+          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

.github/workflows/pr-ready-gate.yml

@@ -0,0 +1,38 @@
+name: PR Ready Gate
+
+on:
+  pull_request:
+    types: [synchronize]
+
+jobs:
+  clear-labels:
+    runs-on: ubuntu-24.04
+    # Always run - curl commands are safe if labels don't exist
+    steps:
+      - name: Remove ready and self-reviewed labels, reassign to author
+        env:
+          GITEA_TOKEN: ${{ secrets.RODIN_TOKEN }}
+        run: |
+          PR_NUMBER=${{ github.event.pull_request.number }}
+          AUTHOR=${{ github.event.pull_request.user.login }}
+          READY_LABEL_ID=38
+          SELF_REVIEWED_LABEL_ID=37
+          
+          # Remove ready label if present
+          curl -sS -X DELETE \
+            -H "Authorization: token $GITEA_TOKEN" \
+            "https://gitea.weiker.me/api/v1/repos/${{ github.repository }}/issues/${PR_NUMBER}/labels/${READY_LABEL_ID}" || true
+          
+          # Remove self-reviewed label if present
+          curl -sS -X DELETE \
+            -H "Authorization: token $GITEA_TOKEN" \
+            "https://gitea.weiker.me/api/v1/repos/${{ github.repository }}/issues/${PR_NUMBER}/labels/${SELF_REVIEWED_LABEL_ID}" || true
+          
+          # Reassign to author
+          curl -sS -X PATCH \
+            -H "Authorization: token $GITEA_TOKEN" \
+            -H "Content-Type: application/json" \
+            -d "{\"assignees\": [\"${AUTHOR}\"]}" \
+            "https://gitea.weiker.me/api/v1/repos/${{ github.repository }}/pulls/${PR_NUMBER}"
+          
+          echo "Cleared ready/self-reviewed labels and reassigned PR #${PR_NUMBER} to ${AUTHOR}"

.github/workflows/release.yml

@@ -0,0 +1,97 @@
+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}
+          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 (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"

CONVENTIONS.md

@@ -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.**

cmd/review-bot/main.go

@@ -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")
+	giteaURL := flag.String("gitea-url", envOrDefault("GITEA_URL", envOrDefault("GITHUB_SERVER_URL", "")), "Gitea instance URL")
-	repo := flag.String("repo", envOrDefault("GITEA_REPO", ""), "Repository (owner/name)")
+	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, ",")
-
+	paths := strings.Split(patternsFiles, ",")
-	// 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)
-			}
-		}
-	}
 
 	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)

cmd/review-bot/main_test.go

@@ -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

docs/DESIGN-57-yaml-persona.md

@@ -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
+```go
-`review/persona.go`) rather than relying on library decoder options. Key design
+func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
-decisions:
+    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
+func checkYAMLDepth(node *yaml.Node, depth, maxDepth int) error {
-- **Dual-map tracking:** `validated` (depth-aware short-circuit) + `visiting` (cycle detection)
+    if depth > maxDepth {
-- **Node-count limit:** Conservative overcounting bounds total validation work
+        return fmt.Errorf("YAML nesting depth exceeds maximum (%d)", maxDepth)
-- **Alias-aware depth:** Aliases increment depth and are re-checked when encountered at greater depths
+    }
+    // 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 |
 

docs/DESIGN-vcs-abstraction.md

@@ -0,0 +1,268 @@
+# GitHub Support for review-bot
+
+## Goal
+
+AI code reviews on GitHub PRs using SAP AI Core as the LLM provider.
+
+## Non-Goals
+
+- Auto-detection of platform (explicit `--provider` flag is fine)
+- Unifying into one abstraction layer for its own sake
+
+## Constraints
+
+1. **Same features on both platforms** — anything review-bot does on Gitea should work on GitHub
+2. **Testable** — small interfaces, dependency injection, no global state
+3. **Interface from working code** — extract from gitea/, don't invent in vacuum
+
+---
+
+## Part 1: Feature Inventory
+
+What does review-bot actually do?
+
+### Core Review Flow
+
+| Feature | Description |
+|---------|-------------|
+| Get PR metadata | Title, body, head SHA, base ref |
+| Get PR diff | Unified diff format |
+| Get PR files | List of changed files with status |
+| Get file content | Raw file at ref |
+| List directory | Enumerate files in path |
+| Post review | Body + inline comments + verdict |
+
+### Review Management
+
+| Feature | Description |
+|---------|-------------|
+| List reviews | Get existing reviews on PR |
+| Delete review | Remove old review before re-posting |
+| Get authenticated user | Who am I? |
+
+### Platform-Specific (not in shared interface)
+
+| Feature | Gitea | GitHub |
+|---------|-------|--------|
+| Resolve comment | Yes | No equivalent |
+| Timeline API | Yes | No equivalent |
+
+These stay on gitea.Client directly. Callers that need them type-assert.
+
+---
+
+## Part 2: GitHub API Mapping
+
+| Feature | Gitea API | GitHub API |
+|---------|-----------|------------|
+| Get PR | `GET /api/v1/repos/.../pulls/{n}` | `GET /repos/.../pulls/{n}` |
+| Get diff | `.diff` suffix | `Accept: application/vnd.github.diff` header |
+| Get files | `GET .../pulls/{n}/files` | Same |
+| Get file content | `GET .../raw/{path}?ref=` | `GET .../contents/{path}?ref=` + base64 decode |
+| List directory | `GET .../contents/{path}` | Same |
+| Post review | `POST .../pulls/{n}/reviews` | Same (adapter handles comment schema) |
+| List reviews | `GET .../pulls/{n}/reviews` | Same |
+| Delete review | `DELETE .../pulls/{n}/reviews/{id}` | Same |
+| Get user | `GET /api/v1/user` | `GET /user` |
+
+---
+
+## Part 3: Interface Design
+
+**Principle:** Extract from working gitea/ code. The interface is discovered, not invented.
+
+### Small, role-based interfaces
+
+```go
+// vcs/interfaces.go
+
+type PRReader interface {
+    GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error)
+    GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error)
+    GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error)
+}
+
+type FileReader interface {
+    GetFileContent(ctx context.Context, owner, repo, path, ref string) (string, error)
+    ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error)
+}
+
+type Reviewer interface {
+    PostReview(ctx context.Context, owner, repo string, number int, req ReviewRequest) (*Review, error)
+    ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error)
+    DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error
+}
+
+type Identity interface {
+    GetAuthenticatedUser(ctx context.Context) (string, error)
+}
+
+// Client combines all for callers that need everything
+type Client interface {
+    PRReader
+    FileReader
+    Reviewer
+    Identity
+}
+```
+
+### Types
+
+Use what gitea/ already has. Move to vcs/types.go or re-export.
+
+```go
+type PullRequest struct { ... }   // from gitea.PullRequest
+type ChangedFile struct { ... }   // from gitea.ChangedFile
+type ContentEntry struct { ... }  // from gitea.ContentEntry
+type Review struct { ... }        // from gitea.Review
+type ReviewRequest struct { ... } // new, for PostReview input
+type ReviewComment struct { ... } // from gitea.ReviewComment
+```
+
+### Adapter responsibilities
+
+Each adapter (gitea, github) handles:
+- API URL construction
+- Auth header format (`token` vs `Bearer`)
+- Request/response mapping
+- Comment schema translation (line numbers, commit IDs, etc.)
+
+---
+
+## Part 4: Test Plan
+
+### Unit Tests (mock HTTP)
+
+```
+github/
+  pr_test.go        # TestGetPullRequest, TestGetDiff, TestGetFiles
+  files_test.go     # TestGetFileContent, TestListContents
+  review_test.go    # TestPostReview, TestListReviews, TestDeleteReview
+  identity_test.go  # TestGetAuthenticatedUser
+```
+
+Per method: happy path, 404, 401, 429, malformed response.
+
+### Integration Tests
+
+Against github.com/aweiker/ai-core-review-bot:
+- Fetch real PR
+- Fetch real file
+- Post + delete review (clean up)
+
+### End-to-End
+
+Open PR on test repo, run full review-bot, verify review appears.
+
+---
+
+## Part 5: Implementation Phases
+
+### Phase 1: Extract interfaces from gitea/
+
+**Work:**
+- Create `vcs/interfaces.go` with interfaces extracted from gitea/client.go signatures
+- Create `vcs/types.go` — move or alias types from gitea/
+- Verify gitea.Client satisfies vcs.Client (compile-time check)
+
+**Exit criteria:** `var _ vcs.Client = (*gitea.Client)(nil)` compiles.
+
+---
+
+### Phase 2: Gitea adapter (if needed)
+
+**Work:**
+- If gitea.Client method signatures don't match exactly, create wrapper
+- Keep gitea/ working exactly as before
+
+**Exit criteria:** Existing tests pass. No behavior change.
+
+---
+
+### Phase 3: GitHub client — PRReader
+
+**Work:**
+- `github/client.go` — struct, constructor, HTTP helpers
+- `github/pr.go` — GetPullRequest, GetPullRequestDiff, GetPullRequestFiles
+- Unit tests
+
+**Exit criteria:** `go test ./github/...` passes for PR methods.
+
+---
+
+### Phase 4: GitHub client — FileReader
+
+**Work:**
+- `github/files.go` — GetFileContent, ListContents
+- Unit tests
+
+**Exit criteria:** Unit tests pass.
+
+---
+
+### Phase 5: GitHub client — Reviewer + Identity
+
+**Work:**
+- `github/review.go` — PostReview, ListReviews, DeleteReview
+- `github/identity.go` — GetAuthenticatedUser
+- Unit tests
+
+**Exit criteria:** Unit tests pass.
+
+---
+
+### Phase 6: Integration tests
+
+**Work:**
+- `integration/github_test.go`
+- Test against real GitHub
+
+**Exit criteria:** All integration tests pass.
+
+---
+
+### Phase 7: Wire into cmd/review-bot
+
+**Work:**
+- Add `--provider github|gitea` flag (default: gitea for backward compat)
+- Select client based on flag
+- Update to use vcs interfaces where it makes sense
+
+**Exit criteria:**
+- `./review-bot --provider github ...` works
+- `./review-bot --provider gitea ...` works (same as before)
+- Existing Gitea workflows unchanged
+
+---
+
+### Phase 8: GitHub Actions workflow + releases
+
+**Work:**
+- `.github/workflows/ci.yml` — test on PR
+- `.github/workflows/release.yml` — publish binary to GitHub releases
+- `.github/actions/review/action.yml` — composite action
+- Action downloads binary from github.com/aweiker/ai-core-review-bot releases
+
+**Exit criteria:** 
+- CI runs on github.com/aweiker/ai-core-review-bot
+- Release creates downloadable binary
+- Review action posts review successfully
+
+---
+
+## Part 6: Decisions
+
+| Question | Decision |
+|----------|----------|
+| Auth token | Workflow `GITHUB_TOKEN` (automatic) |
+| Binary distribution | GitHub releases on aweiker/ai-core-review-bot |
+| Comment schema | Adapter's job — translate ReviewComment to platform format |
+| Default provider | `gitea` for backward compatibility |
+| Shared types | vcs/types.go (extracted from gitea/) |
+| Platform-specific features | Stay on concrete client, not interface |
+
+---
+
+## Summary
+
+8 phases. Start by extracting interfaces from working gitea/ code, not inventing them. GitHub implements the same interfaces. Each phase has clear exit criteria.

gitea/client.go

@@ -11,7 +11,6 @@ import (
 	"fmt"
 	"io"
 	"log/slog"
-	"math"
 	"net"
 	"net/http"
 	"net/url"
@@ -48,12 +47,6 @@ func IsServerError(err error) bool {
 	return errors.As(err, &apiErr) && apiErr.StatusCode >= 500 && apiErr.StatusCode < 600
 }
 
-// DefaultMaxDiffSize is the default maximum diff size in bytes (10 MB).
-const DefaultMaxDiffSize = 10 * 1024 * 1024
-
-// ErrDiffTooLarge is returned when a PR diff exceeds the configured MaxDiffSize.
-var ErrDiffTooLarge = errors.New("diff size exceeds maximum allowed size")
-
 // Client interacts with the Gitea API.
 // A Client is safe for concurrent use by multiple goroutines.
 type Client struct {
@@ -68,14 +61,6 @@ type Client struct {
 	// This field must be configured before the first request is made.
 	// Modifying it while requests are in flight is not safe.
 	RetryBackoff []time.Duration
-
-	// MaxDiffSize is the maximum number of bytes allowed when fetching a PR diff.
-	// If zero, defaults to DefaultMaxDiffSize (10 MB). Set to any negative value
-	// (or math.MaxInt64) to disable the limit.
-	//
-	// This field must be configured before the first request is made.
-	// Modifying it while requests are in flight is not safe.
-	MaxDiffSize int64
 }
 
 // NewClient creates a new Gitea API client.
@@ -140,32 +125,13 @@ func (c *Client) GetPullRequest(ctx context.Context, owner, repo string, number
 }
 
 // GetPullRequestDiff fetches the unified diff for a PR.
-// It enforces MaxDiffSize to prevent unbounded memory allocation.
-// Returns ErrDiffTooLarge if the diff exceeds the configured limit.
 func (c *Client) GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error) {
 	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d.diff", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
-
-	maxSize := c.MaxDiffSize
-	if maxSize == 0 {
-		maxSize = DefaultMaxDiffSize
-	}
-
-	// When the limit is disabled (negative) or set to math.MaxInt64 (which
-	// would overflow the +1 detection and silently disable enforcement),
-	// use the standard unlimited doGet path.
-	if maxSize < 0 || maxSize == math.MaxInt64 {
 	body, err := c.doGet(ctx, reqURL)
 	if err != nil {
 		return "", fmt.Errorf("fetch diff: %w", err)
 	}
 	return string(body), nil
-	}
-
-	body, err := c.doGetLimited(ctx, reqURL, maxSize)
-	if err != nil {
-		return "", fmt.Errorf("fetch diff: %w", err)
-	}
-	return string(body), nil
 }
 
 // GetPullRequestFiles fetches the list of files changed in a PR.
@@ -326,9 +292,9 @@ func isRetriableSyscallError(err error) bool {
 	return true
 }
 
-// redactURL strips query parameters and userinfo credentials from a URL for
+// redactURL strips query parameters from a URL for safe logging.
-// safe logging. This prevents accidental exposure of sensitive data (tokens in
+// This prevents accidental exposure of sensitive data that future callers
-// query strings, or user:pass in the authority) in log output.
+// might pass via query strings.
 func redactURL(rawURL string) string {
 	parsed, err := url.Parse(rawURL)
 	if err != nil {
@@ -336,9 +302,6 @@ func redactURL(rawURL string) string {
 		// potentially logging something sensitive.
 		return "[invalid URL]"
 	}
-	if parsed.User != nil {
-		parsed.User = url.User("REDACTED")
-	}
 	if parsed.RawQuery != "" {
 		parsed.RawQuery = "[redacted]"
 	}
@@ -359,12 +322,10 @@ func sanitizeErrorForLog(err error) string {
 	return err.Error()
 }
 
-// doGetWithReader performs an HTTP GET request with retry on 5xx errors and
+// doGet performs an HTTP GET request with retry on 5xx errors and temporary
-// temporary network errors. Retries up to 3 times with exponential backoff
+// network errors. Retries up to 3 times with exponential backoff (1s, 2s delays
-// (1s, 2s delays by default; configurable via Client.RetryBackoff for testing).
+// by default; configurable via Client.RetryBackoff for testing).
-// The readBody function is called with the response body on success (2xx) and
+func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
-// is responsible for reading and closing it.
-func (c *Client) doGetWithReader(ctx context.Context, reqURL string, readBody func(io.ReadCloser) ([]byte, error)) ([]byte, error) {
 	const maxAttempts = 3
 	// backoff[i] is the delay before attempt i+1 (i.e., after attempt i fails).
 	// First attempt (i=0) has no delay; retries wait 1s then 2s by default.
@@ -429,7 +390,12 @@ func (c *Client) doGetWithReader(ctx context.Context, reqURL string, readBody fu
 			return nil, lastErr
 		}
 		if resp.StatusCode >= 200 && resp.StatusCode < 300 {
-			return readBody(resp.Body)
+			body, err := io.ReadAll(resp.Body)
+			resp.Body.Close()
+			if err != nil {
+				return nil, err
+			}
+			return body, nil
 		}
 
 		// Error path: limit how much we read from potentially malicious server
@@ -447,39 +413,6 @@ func (c *Client) doGetWithReader(ctx context.Context, reqURL string, readBody fu
 	return nil, lastErr
 }
 
-// doGet performs an HTTP GET request with retry, reading the full response body.
-func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
-	return c.doGetWithReader(ctx, reqURL, func(body io.ReadCloser) ([]byte, error) {
-		defer body.Close()
-		return io.ReadAll(body)
-	})
-}
-
-// doGetLimited performs an HTTP GET request with retry but enforces a maximum
-// response body size. Returns ErrDiffTooLarge if the response exceeds maxBytes.
-// It reads maxBytes+1 (clamped to avoid overflow) to detect truncation without
-// buffering the entire body.
-func (c *Client) doGetLimited(ctx context.Context, reqURL string, maxBytes int64) ([]byte, error) {
-	return c.doGetWithReader(ctx, reqURL, func(body io.ReadCloser) ([]byte, error) {
-		defer body.Close()
-		// Read up to maxBytes+1 to detect overflow.
-		// Clamp to prevent integer overflow when maxBytes == math.MaxInt64.
-		limitBytes := maxBytes + 1
-		if limitBytes <= 0 {
-			limitBytes = math.MaxInt64
-		}
-		limited := io.LimitReader(body, limitBytes)
-		data, err := io.ReadAll(limited)
-		if err != nil {
-			return nil, err
-		}
-		if int64(len(data)) > maxBytes {
-			return nil, fmt.Errorf("%w: response exceeds %d bytes", ErrDiffTooLarge, maxBytes)
-		}
-		return data, nil
-	})
-}
-
 // escapePath escapes each segment of a relative file path for use in URLs.
 // Slashes are preserved as path separators; other special characters are escaped.
 // Input should be a relative path (no leading slash). Already-encoded segments

gitea/client_test.go

@@ -1092,21 +1092,6 @@ func TestRedactURL(t *testing.T) {
 			input: "",
 			want:  "",
 		},
-		{
-			name:  "with userinfo - redacts credentials",
-			input: "https://admin:secret@gitea.example.com/api/v1/repos",
-			want:  "https://REDACTED@gitea.example.com/api/v1/repos",
-		},
-		{
-			name:  "with userinfo and query params",
-			input: "https://user:pass@example.com/path?token=abc",
-			want:  "https://REDACTED@example.com/path?[redacted]",
-		},
-		{
-			name:  "username only - no password",
-			input: "https://user@example.com/path",
-			want:  "https://REDACTED@example.com/path",
-		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

gitea/diff_size_test.go

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

github/client.go

@@ -1,260 +0,0 @@
-// Package github provides a client for the GitHub API.
-// It supports pull request operations, file content retrieval,
-// and review submission for both github.com and GitHub Enterprise.
-package github
-
-import (
-	"context"
-	"errors"
-	"fmt"
-	"io"
-	"net/http"
-	"strconv"
-	"strings"
-	"time"
-)
-
-const (
-	defaultBaseURL = "https://api.github.com"
-
-	// maxRetryAttempts is the number of times doRequest will attempt a request.
-	maxRetryAttempts = 3
-
-	// maxRetryAfter caps the maximum delay from a Retry-After header to prevent
-	// a server from stalling the client indefinitely.
-	maxRetryAfter = 60 * time.Second
-
-	// maxErrorBodyBytes limits how much of an error response body we read
-	// to protect against malicious servers sending unbounded data.
-	maxErrorBodyBytes = 64 * 1024 // 64 KB
-
-	// maxResponseBodyBytes limits how much of a successful response body we read
-	// for defense-in-depth against servers returning excessively large payloads.
-	maxResponseBodyBytes = 10 * 1024 * 1024 // 10 MB
-)
-
-// APIError represents an HTTP error response from the GitHub API.
-// It carries the status code so callers can distinguish between
-// different failure modes (e.g. 404 vs 500).
-//
-// The Body field stores up to 64 KiB of the raw response for programmatic
-// inspection. Error() truncates to 200 bytes for safe logging, but callers
-// should avoid logging or propagating Body directly in production since it may
-// contain sensitive details from the upstream server.
-type APIError struct {
-	StatusCode int
-	Body       string
-}
-
-func (e *APIError) Error() string {
-	body := e.Body
-	if len(body) > 200 {
-		body = body[:200] + "...(truncated)"
-	}
-	// Sanitize newlines to prevent log injection from upstream response bodies.
-	body = strings.ReplaceAll(body, "\n", " ")
-	body = strings.ReplaceAll(body, "\r", " ")
-	return fmt.Sprintf("HTTP %d: %s", e.StatusCode, body)
-}
-
-// IsNotFound reports whether an error is an API 404 response.
-func IsNotFound(err error) bool {
-	if apiErr, ok := asAPIError(err); ok {
-		return apiErr.StatusCode == http.StatusNotFound
-	}
-	return false
-}
-
-// IsUnauthorized reports whether an error is an API 401 response.
-func IsUnauthorized(err error) bool {
-	if apiErr, ok := asAPIError(err); ok {
-		return apiErr.StatusCode == http.StatusUnauthorized
-	}
-	return false
-}
-
-func asAPIError(err error) (*APIError, bool) {
-	if err == nil {
-		return nil, false
-	}
-	var target *APIError
-	if errors.As(err, &target) {
-		return target, true
-	}
-	return nil, false
-}
-
-// Client interacts with the GitHub API.
-// A Client is safe for concurrent use by multiple goroutines.
-// SetHTTPClient and SetRetryBackoff are intended for test setup only and must
-// be called before any goroutines issue requests; they have no synchronization.
-type Client struct {
-	// TODO: baseURL is populated by NewClient but not yet consumed by doRequest/doGet.
-	// Higher-level exported methods (GetPullRequest, etc.) will use it to
-	// construct request URLs; remove this field if those methods end up
-	// accepting full URLs instead.
-	baseURL string
-	token   string
-	httpClient *http.Client
-
-	// retryBackoff defines the delays between retry attempts for 429 responses.
-	// retryBackoff[i] is the delay before attempt i+1 (after attempt i fails).
-	// If nil, defaults to {1s, 2s}.
-	retryBackoff []time.Duration
-
-	// now returns the current time. Defaults to time.Now.
-	// Override in tests to control HTTP-date Retry-After calculations.
-	now func() time.Time
-}
-
-// NewClient creates a new GitHub API client.
-// If baseURL is empty, it defaults to https://api.github.com.
-// For GitHub Enterprise, pass the API base URL (e.g. https://github.concur.com/api/v3).
-func NewClient(token, baseURL string) *Client {
-	if baseURL == "" {
-		baseURL = defaultBaseURL
-	}
-	return &Client{
-		baseURL: strings.TrimRight(baseURL, "/"),
-		token:   token,
-		httpClient: &http.Client{Timeout: 30 * time.Second},
-		now:     time.Now,
-	}
-}
-
-// SetHTTPClient sets the underlying HTTP client used for requests.
-// This is intended for testing to inject mock transports.
-func (c *Client) SetHTTPClient(hc *http.Client) {
-	c.httpClient = hc
-}
-
-// SetRetryBackoff sets the delays between retry attempts.
-// This is intended for testing to speed up retry tests.
-//
-// Note: if an empty non-nil slice is provided, Retry-After delays parsed from
-// server responses will be computed and capped but not applied (because
-// attempt < len(backoff) is always false). This is acceptable for the
-// test-only use case but callers should be aware of this edge case.
-func (c *Client) SetRetryBackoff(backoff []time.Duration) {
-	c.retryBackoff = backoff
-}
-
-// parseRetryAfter parses a Retry-After header value, supporting both integer
-// seconds (e.g. "120") and HTTP-date format (e.g. "Thu, 01 Dec 2025 16:00:00 GMT")
-// as specified in RFC 7231 §7.1.3.
-//
-// For integer values, it returns the duration directly.
-// For HTTP-date values, it computes the delay as the difference between the
-// parsed time and now. If the date is in the past, it returns 0.
-//
-// Returns (0, false) if the value cannot be parsed as either format.
-func (c *Client) parseRetryAfter(value string) (time.Duration, bool) {
-	value = strings.TrimSpace(value)
-
-	// Try integer seconds first (most common from GitHub).
-	// RFC 7231 allows delta-seconds of 0 to indicate immediate retry.
-	if seconds, err := strconv.Atoi(value); err == nil && seconds >= 0 {
-		return time.Duration(seconds) * time.Second, true
-	}
-
-	// Try HTTP-date format (RFC 7231 §7.1.3).
-	// http.ParseTime handles RFC 1123, RFC 850, and ASCTIME formats.
-	if retryAt, err := http.ParseTime(value); err == nil {
-		delay := retryAt.Sub(c.now())
-		if delay < 0 {
-			delay = 0
-		}
-		return delay, true
-	}
-
-	return 0, false
-}
-
-// doRequest performs an HTTP request with retry on 429 rate limit responses.
-// It respects the Retry-After header when present, supporting both integer
-// seconds and HTTP-date formats (capped at maxRetryAfter).
-func (c *Client) doRequest(ctx context.Context, method, reqURL string, accept string) ([]byte, error) {
-	var backoff []time.Duration
-	if c.retryBackoff != nil {
-		backoff = append([]time.Duration(nil), c.retryBackoff...)
-	} else {
-		backoff = []time.Duration{1 * time.Second, 2 * time.Second}
-	}
-
-	var lastErr error
-	for attempt := 0; attempt < maxRetryAttempts; attempt++ {
-		if attempt > 0 {
-			var delay time.Duration
-			if attempt-1 < len(backoff) {
-				delay = backoff[attempt-1]
-			}
-			if delay > 0 {
-				timer := time.NewTimer(delay)
-				select {
-				case <-timer.C:
-					timer.Stop()
-				case <-ctx.Done():
-					timer.Stop()
-					return nil, ctx.Err()
-				}
-			}
-		}
-
-		req, err := http.NewRequestWithContext(ctx, method, reqURL, nil)
-		if err != nil {
-			return nil, fmt.Errorf("create request: %w", err)
-		}
-		req.Header.Set("Authorization", "Bearer "+c.token)
-		if accept != "" {
-			req.Header.Set("Accept", accept)
-		} else {
-			req.Header.Set("Accept", "application/vnd.github+json")
-		}
-
-		resp, err := c.httpClient.Do(req)
-		if err != nil {
-			return nil, fmt.Errorf("do request: %w", err)
-		}
-
-		if resp.StatusCode >= 200 && resp.StatusCode < 300 {
-			body, err := io.ReadAll(io.LimitReader(resp.Body, maxResponseBodyBytes))
-			resp.Body.Close()
-			if err != nil {
-				return nil, fmt.Errorf("read response body: %w", err)
-			}
-			return body, nil
-		}
-
-		errBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes))
-		resp.Body.Close()
-
-		lastErr = &APIError{StatusCode: resp.StatusCode, Body: string(errBody)}
-
-		// Retry on 429 rate limit
-		if resp.StatusCode == http.StatusTooManyRequests && attempt < maxRetryAttempts-1 {
-			// Check for Retry-After header and override backoff if present.
-			// Supports both integer seconds (common) and HTTP-date format (RFC 7231).
-			if ra := resp.Header.Get("Retry-After"); ra != "" {
-				if delay, ok := c.parseRetryAfter(ra); ok {
-					if delay > maxRetryAfter {
-						delay = maxRetryAfter
-					}
-					if attempt < len(backoff) {
-						backoff[attempt] = delay
-					}
-				}
-			}
-			continue
-		}
-
-		// Don't retry other errors
-		return nil, lastErr
-	}
-
-	return nil, lastErr
-}
-
-// doGet is a convenience wrapper for GET requests with the default Accept header.
-func (c *Client) doGet(ctx context.Context, url string) ([]byte, error) {
-	return c.doRequest(ctx, http.MethodGet, url, "")
-}

github/client_test.go

@@ -1,409 +0,0 @@
-package github
-
-import (
-	"context"
-	"errors"
-	"net/http"
-	"net/http/httptest"
-	"testing"
-	"time"
-)
-
-func TestNewClient_DefaultBaseURL(t *testing.T) {
-	c := NewClient("tok", "")
-	if c.baseURL != defaultBaseURL {
-		t.Errorf("baseURL = %q, want %q", c.baseURL, defaultBaseURL)
-	}
-}
-
-func TestNewClient_CustomBaseURL(t *testing.T) {
-	c := NewClient("tok", "https://github.concur.com/api/v3/")
-	if c.baseURL != "https://github.concur.com/api/v3" {
-		t.Errorf("baseURL = %q, want trailing slash stripped", c.baseURL)
-	}
-}
-
-func TestDoRequest_Success(t *testing.T) {
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if got := r.Header.Get("Authorization"); got != "Bearer test-token" {
-			t.Errorf("Authorization = %q, want Bearer test-token", got)
-		}
-		w.WriteHeader(http.StatusOK)
-		w.Write([]byte(`{"ok":true}`))
-	}))
-	defer srv.Close()
-
-	c := NewClient("test-token", srv.URL)
-	body, err := c.doGet(context.Background(), srv.URL+"/test")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if string(body) != `{"ok":true}` {
-		t.Errorf("body = %q, want %q", body, `{"ok":true}`)
-	}
-}
-
-func TestDoRequest_429_RetryAfter_IntegerSeconds(t *testing.T) {
-	attempts := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		if attempts == 1 {
-			w.Header().Set("Retry-After", "0")
-			w.WriteHeader(http.StatusTooManyRequests)
-			w.Write([]byte("rate limited"))
-			return
-		}
-		w.WriteHeader(http.StatusOK)
-		w.Write([]byte("success"))
-	}))
-	defer srv.Close()
-
-	c := NewClient("tok", srv.URL)
-	c.SetRetryBackoff([]time.Duration{0, 0})
-
-	body, err := c.doGet(context.Background(), srv.URL+"/test")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if string(body) != "success" {
-		t.Errorf("body = %q, want %q", body, "success")
-	}
-	if attempts != 2 {
-		t.Errorf("attempts = %d, want 2", attempts)
-	}
-}
-
-func TestDoRequest_429_RetryAfter_HTTPDate(t *testing.T) {
-	// Fix "now" to a known time for deterministic testing.
-	fixedNow := time.Date(2025, 12, 1, 15, 59, 59, 0, time.UTC)
-	retryAt := "Mon, 01 Dec 2025 16:00:00 GMT" // 1 second in the future
-
-	attempts := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		if attempts == 1 {
-			w.Header().Set("Retry-After", retryAt)
-			w.WriteHeader(http.StatusTooManyRequests)
-			w.Write([]byte("rate limited"))
-			return
-		}
-		w.WriteHeader(http.StatusOK)
-		w.Write([]byte("success"))
-	}))
-	defer srv.Close()
-
-	c := NewClient("tok", srv.URL)
-	c.now = func() time.Time { return fixedNow }
-	// Initial backoff is 0; the HTTP-date parser will compute 1s and override.
-	c.SetRetryBackoff([]time.Duration{0, 0})
-
-	body, err := c.doGet(context.Background(), srv.URL+"/test")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if string(body) != "success" {
-		t.Errorf("body = %q, want %q", body, "success")
-	}
-	if attempts != 2 {
-		t.Errorf("attempts = %d, want 2", attempts)
-	}
-}
-
-func TestDoRequest_429_RetryAfter_HTTPDate_InPast(t *testing.T) {
-	// If the HTTP-date is in the past, delay should be 0 (retry immediately).
-	fixedNow := time.Date(2025, 12, 1, 17, 0, 0, 0, time.UTC)
-	retryAt := "Mon, 01 Dec 2025 16:00:00 GMT" // 1 hour in the past
-
-	attempts := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		if attempts == 1 {
-			w.Header().Set("Retry-After", retryAt)
-			w.WriteHeader(http.StatusTooManyRequests)
-			w.Write([]byte("rate limited"))
-			return
-		}
-		w.WriteHeader(http.StatusOK)
-		w.Write([]byte("success"))
-	}))
-	defer srv.Close()
-
-	c := NewClient("tok", srv.URL)
-	c.now = func() time.Time { return fixedNow }
-	c.SetRetryBackoff([]time.Duration{0, 0})
-
-	body, err := c.doGet(context.Background(), srv.URL+"/test")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if string(body) != "success" {
-		t.Errorf("body = %q, want %q", body, "success")
-	}
-}
-
-func TestDoRequest_429_NoRetryAfter_UsesDefaultBackoff(t *testing.T) {
-	attempts := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		if attempts == 1 {
-			w.WriteHeader(http.StatusTooManyRequests)
-			w.Write([]byte("rate limited"))
-			return
-		}
-		w.WriteHeader(http.StatusOK)
-		w.Write([]byte("success"))
-	}))
-	defer srv.Close()
-
-	c := NewClient("tok", srv.URL)
-	c.SetRetryBackoff([]time.Duration{0, 0})
-
-	body, err := c.doGet(context.Background(), srv.URL+"/test")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if string(body) != "success" {
-		t.Errorf("body = %q, want %q", body, "success")
-	}
-	if attempts != 2 {
-		t.Errorf("attempts = %d, want 2", attempts)
-	}
-}
-
-func TestDoRequest_429_InvalidRetryAfter_UsesDefaultBackoff(t *testing.T) {
-	attempts := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		if attempts == 1 {
-			w.Header().Set("Retry-After", "not-a-number-or-date")
-			w.WriteHeader(http.StatusTooManyRequests)
-			w.Write([]byte("rate limited"))
-			return
-		}
-		w.WriteHeader(http.StatusOK)
-		w.Write([]byte("success"))
-	}))
-	defer srv.Close()
-
-	c := NewClient("tok", srv.URL)
-	c.SetRetryBackoff([]time.Duration{0, 0})
-
-	body, err := c.doGet(context.Background(), srv.URL+"/test")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if string(body) != "success" {
-		t.Errorf("body = %q, want %q", body, "success")
-	}
-}
-
-func TestDoRequest_404_NoRetry(t *testing.T) {
-	attempts := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		w.WriteHeader(http.StatusNotFound)
-		w.Write([]byte("not found"))
-	}))
-	defer srv.Close()
-
-	c := NewClient("tok", srv.URL)
-	_, err := c.doGet(context.Background(), srv.URL+"/test")
-	if err == nil {
-		t.Fatal("expected error, got nil")
-	}
-	if !IsNotFound(err) {
-		t.Errorf("expected IsNotFound, got %v", err)
-	}
-	if attempts != 1 {
-		t.Errorf("attempts = %d, want 1 (no retry on 404)", attempts)
-	}
-}
-
-func TestDoRequest_401_NoRetry(t *testing.T) {
-	attempts := 0
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		w.WriteHeader(http.StatusUnauthorized)
-		w.Write([]byte("unauthorized"))
-	}))
-	defer srv.Close()
-
-	c := NewClient("tok", srv.URL)
-	_, err := c.doGet(context.Background(), srv.URL+"/test")
-	if err == nil {
-		t.Fatal("expected error, got nil")
-	}
-	if !IsUnauthorized(err) {
-		t.Errorf("expected IsUnauthorized, got %v", err)
-	}
-	if attempts != 1 {
-		t.Errorf("attempts = %d, want 1 (no retry on 401)", attempts)
-	}
-}
-
-func TestDoRequest_ContextCanceled(t *testing.T) {
-	// This test exercises the timer-cancel path in the retry select:
-	//   select { case <-timer.C; case <-ctx.Done() }
-	// The server returns 429 with a long Retry-After, and we cancel the
-	// context shortly after the first response so that cancellation races
-	// against the timer rather than preventing the initial HTTP round-trip.
-	requestReceived := make(chan struct{}, 1)
-	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		select {
-		case requestReceived <- struct{}{}:
-		default:
-		}
-		w.Header().Set("Retry-After", "10")
-		w.WriteHeader(http.StatusTooManyRequests)
-	}))
-	defer srv.Close()
-
-	c := NewClient("tok", srv.URL)
-	c.SetRetryBackoff([]time.Duration{10 * time.Second, 10 * time.Second})
-
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
-	// Cancel the context after the first request completes, while the
-	// client is blocked in the retry timer select.
-	go func() {
-		<-requestReceived
-		// Small delay to ensure we're inside the timer select.
-		time.Sleep(50 * time.Millisecond)
-		cancel()
-	}()
-
-	_, err := c.doGet(ctx, srv.URL+"/test")
-	if err == nil {
-		t.Fatal("expected error, got nil")
-	}
-	if !errors.Is(err, context.Canceled) {
-		t.Errorf("err = %v, want context.Canceled", err)
-	}
-}
-
-func TestParseRetryAfter_IntegerSeconds(t *testing.T) {
-	c := NewClient("tok", "")
-	delay, ok := c.parseRetryAfter("42")
-	if !ok {
-		t.Fatal("expected ok=true")
-	}
-	if delay != 42*time.Second {
-		t.Errorf("delay = %v, want 42s", delay)
-	}
-}
-
-func TestParseRetryAfter_ZeroSeconds(t *testing.T) {
-	c := NewClient("tok", "")
-	delay, ok := c.parseRetryAfter("0")
-	if !ok {
-		t.Fatal("expected ok=true for zero seconds (RFC 7231 allows immediate retry)")
-	}
-	if delay != 0 {
-		t.Errorf("delay = %v, want 0", delay)
-	}
-}
-
-func TestParseRetryAfter_NegativeSeconds(t *testing.T) {
-	c := NewClient("tok", "")
-	_, ok := c.parseRetryAfter("-5")
-	if ok {
-		t.Error("expected ok=false for negative seconds")
-	}
-}
-
-func TestParseRetryAfter_HTTPDate_Future(t *testing.T) {
-	fixedNow := time.Date(2025, 12, 1, 15, 59, 50, 0, time.UTC)
-	c := NewClient("tok", "")
-	c.now = func() time.Time { return fixedNow }
-
-	delay, ok := c.parseRetryAfter("Mon, 01 Dec 2025 16:00:00 GMT")
-	if !ok {
-		t.Fatal("expected ok=true")
-	}
-	// Should be 10 seconds in the future.
-	if delay != 10*time.Second {
-		t.Errorf("delay = %v, want 10s", delay)
-	}
-}
-
-func TestParseRetryAfter_HTTPDate_Past(t *testing.T) {
-	fixedNow := time.Date(2025, 12, 1, 17, 0, 0, 0, time.UTC)
-	c := NewClient("tok", "")
-	c.now = func() time.Time { return fixedNow }
-
-	delay, ok := c.parseRetryAfter("Mon, 01 Dec 2025 16:00:00 GMT")
-	if !ok {
-		t.Fatal("expected ok=true")
-	}
-	if delay != 0 {
-		t.Errorf("delay = %v, want 0 (past date)", delay)
-	}
-}
-
-func TestParseRetryAfter_RFC850_Format(t *testing.T) {
-	fixedNow := time.Date(2025, 12, 1, 15, 59, 50, 0, time.UTC)
-	c := NewClient("tok", "")
-	c.now = func() time.Time { return fixedNow }
-
-	// RFC 850 format
-	delay, ok := c.parseRetryAfter("Monday, 01-Dec-25 16:00:00 GMT")
-	if !ok {
-		t.Fatal("expected ok=true for RFC 850 format")
-	}
-	if delay != 10*time.Second {
-		t.Errorf("delay = %v, want 10s", delay)
-	}
-}
-
-func TestParseRetryAfter_Invalid(t *testing.T) {
-	c := NewClient("tok", "")
-	_, ok := c.parseRetryAfter("not-valid")
-	if ok {
-		t.Error("expected ok=false for invalid value")
-	}
-}
-
-func TestParseRetryAfter_EmptyString(t *testing.T) {
-	c := NewClient("tok", "")
-	_, ok := c.parseRetryAfter("")
-	if ok {
-		t.Error("expected ok=false for empty string")
-	}
-}
-
-func TestParseRetryAfter_MaxCap(t *testing.T) {
-	// Verify that parseRetryAfter returns the raw value (capping is done by caller).
-	c := NewClient("tok", "")
-	delay, ok := c.parseRetryAfter("3600")
-	if !ok {
-		t.Fatal("expected ok=true")
-	}
-	if delay != 3600*time.Second {
-		t.Errorf("delay = %v, want 3600s (caller is responsible for capping)", delay)
-	}
-}
-
-func TestAPIError_Error_Truncation(t *testing.T) {
-	longBody := make([]byte, 300)
-	for i := range longBody {
-		longBody[i] = 'x'
-	}
-	apiErr := &APIError{StatusCode: 500, Body: string(longBody)}
-	msg := apiErr.Error()
-	if len(msg) > 250 {
-		// "HTTP 500: " (10) + 200 + "...(truncated)" (14) = 224
-		t.Errorf("error message too long: %d chars", len(msg))
-	}
-}
-
-func TestAPIError_Error_NewlineSanitized(t *testing.T) {
-	apiErr := &APIError{StatusCode: 400, Body: "line1\nline2\rline3"}
-	msg := apiErr.Error()
-	for _, c := range msg {
-		if c == '\n' || c == '\r' {
-			t.Errorf("error message contains unsanitized newline: %q", msg)
-			break
-		}
-	}
-}

go.mod

@@ -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

go.sum

@@ -1,2 +1,4 @@
-github.com/goccy/go-yaml v1.19.2 h1:PmFC1S6h8ljIz6gMRBopkjP1TVT7xuwrButHID66PoM=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
-github.com/goccy/go-yaml v1.19.2/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7LkFRi1kA=
+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=

review/persona.go

@@ -5,15 +5,12 @@ import (
 	"embed"
 	"encoding/json"
 	"fmt"
-	"io"
 	"os"
 	"sort"
 	"strings"
 	"unicode/utf8"
 
-	"github.com/goccy/go-yaml"
+	"gopkg.in/yaml.v3"
-	"github.com/goccy/go-yaml/ast"
-	"github.com/goccy/go-yaml/parser"
 )
 
 //go:embed personas/*.yaml
@@ -121,8 +118,10 @@ func ListBuiltinPersonas() []string {
 		default:
 			continue
 		}
+		if !seen[personaName] {
 			seen[personaName] = true
 		}
+	}
 	names := make([]string, 0, len(seen))
 	for name := range seen {
 		names = append(names, name)
@@ -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,176 +156,78 @@ func parsePersona(data []byte, source string) (*Persona, error) {
 	return &p, nil
 }
 
-// unmarshalYAMLWithDepthLimit unmarshals YAML data with three safety checks:
+// unmarshalYAMLWithDepthLimit unmarshals YAML data with explicit depth limiting
-//   - Depth limiting: rejects AST trees exceeding maxDepth to prevent stack exhaustion.
+// and strict field checking. This protects against stack exhaustion from deeply
-//   - Multi-document rejection: prevents silent data loss from ignored extra documents.
+// nested structures and catches typos in field names.
-//   - Strict field checking: rejects unknown YAML keys to catch typos early.
+// 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
+	// First pass: decode into a yaml.Node to check depth limits and node counts.
-	// multi-document rejection. This prevents stack exhaustion before we
+	// This prevents stack exhaustion before we attempt to decode into structs.
-	// attempt to decode into structs.
+	var node yaml.Node
-	file, err := parser.ParseBytes(data, 0)
+	dec := yaml.NewDecoder(bytes.NewReader(data))
-	if err != nil {
+	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".
+	// 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
-	// Safety note: goccy/go-yaml's decoder does not expand YAML aliases
+	// support the KnownFields option.
-	// recursively — it resolves them via the pre-built AST, which our first
+	strictDec := yaml.NewDecoder(bytes.NewReader(data))
-	// pass already depth-checked. Alias chains that would exceed depth limits
+	strictDec.KnownFields(true)
-	// are caught above; the decoder merely reads the resolved scalar values.
+	return strictDec.Decode(out)
-	dec := yaml.NewDecoder(bytes.NewReader(data), yaml.Strict())
-	return dec.Decode(out)
 }
 
-// checkYAMLDepth recursively checks that YAML AST nodes don't exceed the depth
+// checkYAMLDepth recursively checks that YAML nodes don't exceed the depth limit
-// limit or the total node count limit. It uses two tracking maps:
+// or the total node count limit. It also detects alias cycles to prevent infinite
-//   - validated: maps each node to the maximum depth at which it was previously
+// recursion from crafted YAML with self-referential aliases.
-//     checked. If a node is revisited at a deeper depth (e.g., via an alias),
+func checkYAMLDepth(node *yaml.Node, depth, maxDepth, maxNodes int, seen map[*yaml.Node]struct{}, nodeCount *int) error {
-//     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
-	}
-
 	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
+	// Cycle detection: if we've seen this node before, we're in a cycle.
-	// depth is the same or shallower than the depth at which this node was
+	if _, ok := seen[node]; ok {
-	// previously validated. A shallower (or equal) current depth means the
+		return nil // Already validated this subtree, skip to avoid infinite recursion.
-	// 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
 	}
-	validated[node] = depth
+	seen[node] = struct{}{}
 
-	// Mark as visiting (on the current recursion path) for cycle detection.
+	// Handle alias nodes: follow the alias to its anchor target.
-	visiting[node] = true
+	// Increment depth when following aliases since they expand the effective structure.
-	defer func() { visiting[node] = false }()
+	if node.Kind == yaml.AliasNode && node.Alias != nil {
+		return checkYAMLDepth(node.Alias, depth+1, maxDepth, maxNodes, seen, nodeCount)
+	}
 
-	// Walk children based on node type.
+	for _, child := range node.Content {
-	switch n := node.(type) {
+		if err := checkYAMLDepth(child, depth+1, maxDepth, maxNodes, seen, nodeCount); err != nil {
-	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 {
-			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
 }
 
 // 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)
 }
 

review/persona_test.go

@@ -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: ...":
+	// Each level adds 2 to the depth count (key + value mapping).
-	//   - 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.
 	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.
+	// simulating what could happen with malicious input that bypasses
-	parent := &ast.MappingNode{
+	// go-yaml's cycle detection.
-		Values: []*ast.MappingValueNode{
+	parent := &yaml.Node{
-			{
+		Kind: yaml.MappingNode,
-				Key:   &ast.StringNode{Value: "name"},
+		Content: []*yaml.Node{
-				Value: &ast.StringNode{Value: "test"},
+			{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{
+	aliasToParent := &yaml.Node{
-		Value: parent,
+		Kind:  yaml.AliasNode,
+		Alias: parent,
 	}
-	parent.Values = append(parent.Values, &ast.MappingValueNode{
+	parent.Content = append(parent.Content, aliasToParent)
-		Key:   &ast.StringNode{Value: "nested"},
-		Value: aliasToParent,
-	})
 
 	nodeCount := 0
-	validated := make(map[ast.Node]int)
+	seen := make(map[*yaml.Node]struct{})
-	visiting := make(map[ast.Node]bool)
 
-	// This should NOT hang or stack overflow - cycle detection prevents infinite recursion
+	// This should NOT hang or stack overflow - the seen map prevents infinite recursion
-	err := checkYAMLDepth(parent, 0, MaxYAMLDepth, MaxYAMLNodes, validated, visiting, &nodeCount)
+	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
+	// Verify we tracked the parent in the seen map
-	if _, ok := validated[parent]; !ok {
+	if _, ok := seen[parent]; !ok {
-		t.Error("parent node not tracked in validated map")
+		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{
+	// This tests the seen map logic independent of go-yaml's parsing.
-		Values: []*ast.MappingValueNode{
+	node := &yaml.Node{
-			{
+		Kind: yaml.MappingNode,
-				Key:   &ast.StringNode{Value: "key"},
+		Content: []*yaml.Node{
-				Value: &ast.StringNode{Value: "value"},
+			{Kind: yaml.ScalarNode, Value: "key"},
-			},
+			{Kind: yaml.ScalarNode, Value: "value"},
 		},
 	}
 
 	// Create a cycle by making a child reference the parent
-	cycleChild := &ast.AliasNode{
+	cycleChild := &yaml.Node{
-		Value: node, // Points back to the parent
+		Kind:  yaml.AliasNode,
+		Alias: node, // Points back to the parent
 	}
-	node.Values = append(node.Values, &ast.MappingValueNode{
+	node.Content = append(node.Content,
-		Key:   &ast.StringNode{Value: "cyclic"},
+		&yaml.Node{Kind: yaml.ScalarNode, Value: "cyclic"},
-		Value: cycleChild,
+		cycleChild,
-	})
+	)
 
 	nodeCount := 0
-	validated := make(map[ast.Node]int)
+	seen := make(map[*yaml.Node]struct{})
-	visiting := make(map[ast.Node]bool)
+	err := checkYAMLDepth(node, 0, MaxYAMLDepth, MaxYAMLNodes, seen, &nodeCount)
-	err := checkYAMLDepth(node, 0, MaxYAMLDepth, MaxYAMLNodes, validated, visiting, &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
+	// The seen map should contain multiple entries
-	if len(validated) < 2 {
+	if len(seen) < 2 {
-		t.Errorf("validated map has %d entries, expected at least 2", len(validated))
+		t.Errorf("seen map has %d entries, expected at least 2", len(seen))
-	}
-}
-
-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())
 	}
 }
 
@@ -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())
-	}
-}

vcs/check_test.go

@@ -0,0 +1,27 @@
+//go:build phase2
+
+package vcs_test
+
+import (
+	"gitea.weiker.me/rodin/review-bot/gitea"
+	"gitea.weiker.me/rodin/review-bot/vcs"
+)
+
+// Compile-time assertion: documents the gap between gitea.Client and vcs.Client.
+// Guarded by the "phase2" build tag — enable once the Gitea adapter bridges these gaps:
+//
+//  1. PostReview signature mismatch:
+//     gitea.Client:  PostReview(ctx, owner, repo, number, event, body string, comments []gitea.ReviewComment)
+//     vcs.Reviewer:  PostReview(ctx, owner, repo, number, req vcs.ReviewRequest)
+//
+//  2. GetFileContent signature mismatch:
+//     gitea.Client:  GetFileContent(ctx, owner, repo, filepath string)  [no ref; uses default branch]
+//     vcs.FileReader: GetFileContent(ctx, owner, repo, path, ref string)
+//     (gitea.Client has GetFileContentRef for the ref variant)
+//
+//  3. ReviewComment type mismatch:
+//     gitea.ReviewComment uses NewPosition int64 (Gitea line-number convention)
+//     vcs.ReviewComment uses Position int (GitHub diff-position convention)
+//
+// The Gitea adapter (Phase 2) will wrap gitea.Client to bridge these gaps.
+var _ vcs.Client = (*gitea.Client)(nil)

vcs/interfaces.go

@@ -0,0 +1,40 @@
+// Package vcs defines the shared VCS client interface and supporting types.
+// Platform adapters (gitea, github) implement these interfaces so the core
+// review logic can work with any VCS platform without platform-specific code.
+package vcs
+
+import "context"
+
+// PRReader can fetch pull request metadata, diffs, and changed files.
+type PRReader interface {
+	GetPullRequest(ctx context.Context, owner, repo string, number int) (*PullRequest, error)
+	GetPullRequestDiff(ctx context.Context, owner, repo string, number int) (string, error)
+	GetPullRequestFiles(ctx context.Context, owner, repo string, number int) ([]ChangedFile, error)
+}
+
+// FileReader can fetch file contents and list directory entries.
+type FileReader interface {
+	GetFileContent(ctx context.Context, owner, repo, path, ref string) (string, error)
+	ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error)
+}
+
+// Reviewer can post, list, and delete pull request reviews.
+type Reviewer interface {
+	PostReview(ctx context.Context, owner, repo string, number int, req ReviewRequest) (*Review, error)
+	ListReviews(ctx context.Context, owner, repo string, number int) ([]Review, error)
+	DeleteReview(ctx context.Context, owner, repo string, number int, reviewID int64) error
+}
+
+// Identity can report who the authenticated user is.
+type Identity interface {
+	GetAuthenticatedUser(ctx context.Context) (string, error)
+}
+
+// Client is the full VCS interface: PR reads, file reads, review management, and identity.
+// Platform adapters (gitea, github) implement this interface.
+type Client interface {
+	PRReader
+	FileReader
+	Reviewer
+	Identity
+}

vcs/types.go

@@ -0,0 +1,82 @@
+package vcs
+
+// ReviewEvent is the event type for a pull request review action.
+// Adapters must translate these action constants to/from platform-native values.
+// For example, Gitea uses "APPROVED" as both action and state, while GitHub
+// uses "APPROVE" for the action and returns "approved" as the state.
+type ReviewEvent string
+
+const (
+	// ReviewEventApprove approves the pull request.
+	ReviewEventApprove ReviewEvent = "APPROVE"
+	// ReviewEventRequestChanges requests changes to the pull request.
+	ReviewEventRequestChanges ReviewEvent = "REQUEST_CHANGES"
+	// ReviewEventComment posts a review comment without approval or rejection.
+	ReviewEventComment ReviewEvent = "COMMENT"
+)
+
+// HeadRef identifies the source branch and latest commit of a pull request.
+type HeadRef struct {
+	SHA string `json:"sha"`
+	Ref string `json:"ref"`
+}
+
+// UserInfo identifies a user by login name.
+type UserInfo struct {
+	Login string `json:"login"`
+}
+
+// PullRequest holds relevant PR metadata.
+type PullRequest struct {
+	Title string  `json:"title"`
+	Body  string  `json:"body"`
+	Head  HeadRef `json:"head"`
+}
+
+// ChangedFile represents a file modified in a PR.
+type ChangedFile struct {
+	Filename string `json:"filename"`
+	Status   string `json:"status"`
+}
+
+// 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"
+}
+
+// Review represents a pull request review.
+type Review struct {
+	ID       int64    `json:"id"`
+	Body     string   `json:"body"`
+	User     UserInfo `json:"user"`
+	State    string   `json:"state"`
+	Stale    bool     `json:"stale"`
+	CommitID string   `json:"commit_id"`
+}
+
+// ReviewComment represents an inline comment in a review.
+// All adapters use GitHub diff-position convention:
+//   - Position is a 1-indexed offset from the @@ hunk line in the unified diff.
+//   - CommitID identifies the commit the comment is anchored to.
+//     It is optional; omit (empty string) for review-level comments that are
+//     not attached to a specific commit.
+//
+// Adapters are responsible for translating to/from platform-native formats
+// (e.g. Gitea uses line numbers; GitHub uses diff positions natively).
+type ReviewComment struct {
+	Path     string `json:"path"`
+	Position int    `json:"position"` // diff-position: 1-indexed offset from @@ hunk line
+	CommitID string `json:"commit_id"`
+	Body     string `json:"body"`
+}
+
+// ReviewRequest is the payload for posting a review.
+type ReviewRequest struct {
+	// Body is the top-level review comment.
+	Body string `json:"body"`
+	// Event is the review action (approve, request changes, or comment).
+	Event    ReviewEvent     `json:"event"`
+	Comments []ReviewComment `json:"comments,omitempty"`
+}