Merge pull request 'feat(github): add safeguards against accidental AllowInsecureHTTP use (#96)' (#113) from review-bot-issue-96 into main

Reviewed-on: #113
Reviewed-by: security-review-bot <10+security-review-bot@noreply.gitea.weiker.me>

.gitea/actions/review/action.yml

@@ -26,18 +26,40 @@ inputs:
     required: false
     default: ''
   llm-base-url:
-    description: 'OpenAI-compatible LLM API base URL'
-    required: true
+    description: 'OpenAI-compatible LLM API base URL (not required for aicore provider)'
+    required: false
+    default: ''
   llm-api-key:
-    description: 'LLM API key'
-    required: true
+    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 or anthropic (default openai)'
+    description: 'LLM API provider: openai, anthropic, or aicore (default openai)'
     required: false
-    default: 'openai' 
+    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
@@ -74,6 +96,14 @@ inputs:
     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'
@@ -155,6 +185,13 @@ runs:
         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

.gitea/workflows/ci.yml

@@ -18,7 +18,10 @@ jobs:
       - run: go vet ./...
       - run: go build -o review-bot ./cmd/review-bot
 
-  # Self-review: builds from source since we're pre-release
+  # 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'
@@ -28,34 +31,15 @@ jobs:
         include:
           - name: sonnet
             token_secret: SONNET_REVIEW_TOKEN
-            provider: anthropic
-            llm_path: /anthropic/v1
-            model: claude-sonnet-4-6
+            model: anthropic--claude-4.6-sonnet
           - name: gpt
             token_secret: GPT_REVIEW_TOKEN
-            provider: openai
-            llm_path: /openai/v1
             model: gpt-5
-          - name: gpt41
-            token_secret: GPT_REVIEW_TOKEN
-            provider: openai
-            llm_path: /openai/v1
-            model: gpt-4.1
-          - name: gpt5-mini
-            token_secret: GPT_REVIEW_TOKEN
-            provider: openai
-            llm_path: /openai/v1
-            model: gpt-5-mini
-          - name: gpt41-mini
-            token_secret: GPT_REVIEW_TOKEN
-            provider: openai
-            llm_path: /openai/v1
-            model: gpt-4.1-mini
           - name: security
             token_secret: SECURITY_REVIEW_TOKEN
-            provider: openai
-            llm_path: /openai/v1
             model: gpt-5
+            patterns_repo: rodin/security-patterns
+            patterns_files: "."
             system_prompt_file: SECURITY_REVIEW.md
     steps:
       - uses: actions/checkout@v4
@@ -70,13 +54,16 @@ jobs:
           PR_NUMBER: ${{ github.event.pull_request.number }}
           REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }}
           REVIEWER_NAME: ${{ matrix.name }}
-          LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }}${{ matrix.llm_path }}
-          LLM_API_KEY: ${{ secrets.LLM_API_KEY }}
+          LLM_PROVIDER: aicore
           LLM_MODEL: ${{ matrix.model }}
-          LLM_PROVIDER: ${{ matrix.provider }}
+          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: "rodin/go-patterns"
-          PATTERNS_FILES: "README.md,patterns/"
+          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

.gitea/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}"

CONVENTIONS.md

@@ -2,8 +2,26 @@
 
 ## Language & Dependencies
 
-- Go standard library only — no external dependencies.
 - Target the latest stable Go release.
+- **STRICT ALLOWLIST:** Only packages listed below may be imported. No exceptions.
+
+### Approved Third-Party Packages
+
+| Package | Use Case | Scope |
+|---------|----------|-------|
+| `github.com/goccy/go-yaml` | YAML parsing and AST inspection (subpkgs: `ast`, `parser`) | 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.**
+
+Transitive dependencies of approved packages are automatically allowed.
+
+To request a new dependency:
+1. Open a PR that ONLY updates this table
+2. Requires explicit approval from Aaron
+3. After merge, a separate PR may use the package
+
+*Enforcement: `scripts/check-deps.sh` parses this table — update only here.*
 
 ## Error Handling
 

Makefile

@@ -1,4 +1,4 @@
-.PHONY: build test test-integration lint clean coverage
+.PHONY: build test test-integration lint clean coverage check-deps precommit
 
 build:
 	go build -o review-bot ./cmd/review-bot/
@@ -12,9 +12,15 @@ test-integration:
 lint:
 	go vet ./...
 
+check-deps:
+	@./scripts/check-deps.sh
+
 clean:
 	rm -f review-bot
 
 coverage:
 	go test -coverprofile=coverage.out ./...
 	go tool cover -func=coverage.out
+
+# Precommit runs all checks required before pushing
+precommit: check-deps lint test

README.md

@@ -4,12 +4,12 @@ AI-powered code review bot for Gitea pull requests. Fetches diff + context, send
 
 ## Features
 
-- **Multi-provider**: OpenAI-compatible and Anthropic Messages API
+- **Multi-provider**: OpenAI-compatible, Anthropic Messages API, and SAP AI Core
 - **Context-aware**: Fetches full file content, conventions, language patterns, CI status
 - **Smart budget**: Automatically trims context to fit model token limits
 - **Idempotent reviews**: Posts new review, then cleans up stale ones (one review per bot)
 - **Custom prompts**: Load additional instructions from a file (e.g. security-focused review)
-- **Zero dependencies**: Go stdlib only
+- **Minimal dependencies**: Go stdlib + `gopkg.in/yaml.v3` only
 
 ## Quick Start: Composite Action
 
@@ -168,26 +168,56 @@ Prints the review to CI logs without posting to the PR. Useful for testing promp
     llm-provider: anthropic
 ```
 
+### Using SAP AI Core
+
+For SAP environments with AI Core deployments, use the `aicore` provider for native authentication:
+
+```yaml
+- uses: https://gitea.weiker.me/rodin/review-bot/.gitea/actions/review@v0.1.0
+  with:
+    reviewer-token: ${{ secrets.REVIEW_TOKEN }}
+    reviewer-name: aicore-review
+    llm-model: anthropic--claude-4.6-sonnet  # or gpt-5
+    llm-provider: aicore
+    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: default
+```
+
+AI Core handles OAuth token management and deployment discovery automatically. Model names must match the deployment name in AI Core (e.g. `anthropic--claude-4.6-sonnet`, `gpt-5`).
+
 ## Action Inputs
 
 | Input | Required | Default | Description |
 |-------|----------|---------|-------------|
 | `reviewer-token` | Yes | — | Gitea token for posting reviews (needs `write:issue`, `write:repository`) |
 | `reviewer-name` | No | `""` | Logical identity for this reviewer. Used as sentinel for idempotent cleanup. Set this when running multiple review bots on the same PR. |
-| `llm-base-url` | Yes | — | LLM API base URL |
-| `llm-api-key` | Yes | — | LLM API key |
+| `llm-base-url` | No* | `""` | LLM API base URL (required unless using aicore provider) |
+| `llm-api-key` | No* | `""` | LLM API key (required unless using aicore provider) |
 | `llm-model` | Yes | — | Model name |
-| `llm-provider` | No | `openai` | API provider: `openai` or `anthropic` |
+| `llm-provider` | No | `openai` | API provider: `openai`, `anthropic`, or `aicore` |
+| `aicore-client-id` | No** | `""` | SAP AI Core client ID |
+| `aicore-client-secret` | No** | `""` | SAP AI Core client secret |
+| `aicore-auth-url` | No** | `""` | SAP AI Core authentication URL |
+| `aicore-api-url` | No** | `""` | SAP AI Core API URL |
+| `aicore-resource-group` | No | `default` | SAP AI Core resource group |
 | `conventions-file` | No | `""` | Path to coding conventions file in the repo |
 | `patterns-repo` | No | `""` | Comma-separated repos with language patterns (e.g. `rodin/go-patterns`) |
 | `patterns-files` | No | `README.md` | Files/directories to fetch from pattern repos |
 | `system-prompt-file` | No | `""` | Local file with additional system prompt instructions |
+| `persona` | No | `""` | Built-in persona name (security, architect, docs) |
+| `persona-file` | No | `""` | Path to persona file (YAML or JSON) with custom review focus |
 | `temperature` | No | `0` | LLM temperature (0 = server default) |
 | `timeout` | No | `300` | LLM request timeout in seconds |
 | `dry-run` | No | `false` | Print review to stdout instead of posting |
 | `update-existing` | No | `true` | Delete previous review from same bot before posting. Accepts: true/1/yes or false/0/no |
 | `version` | No | `latest` | review-bot version to install |
 
+*Required for `openai` and `anthropic` providers, not for `aicore`.
+**Required only for `aicore` provider.
+
 ## Runner Requirements
 
 The composite action requires these tools on the runner:
@@ -299,11 +329,12 @@ All flags have environment variable equivalents:
 ### Token Scopes Required
 
 | Scope | Purpose |
-|-------|---------|
+|-------|--------|
 | `write:issue` | Post and delete reviews |
 | `write:repository` | Read PR diffs, file content, commit statuses |
+| `read:user` | Self-request as reviewer (optional but recommended) |
 
-No `read:user` scope needed — the bot identifies itself from the review response.
+Without `read:user`, the bot still works but cannot add itself to the PR's reviewer list.
 
 ## Development
 
@@ -329,3 +360,116 @@ budget/             Token estimation + context trimming
 ## License
 
 MIT
+
+## Review Personas
+
+Personas provide role-based review specialization. Instead of generic code review, each persona focuses on a specific domain (security, architecture, documentation) with tailored prompts and severity calibration.
+
+### Built-in Personas
+
+| Persona | Focus |
+|---------|-------|
+| `security` | Vulnerabilities, auth bypass, secrets exposure, injection attacks |
+| `architect` | Design patterns, code organization, API contracts, testability |
+| `docs` | Documentation quality, API clarity, error messages |
+
+### Using Built-in Personas
+
+```yaml
+- uses: rodin/review-bot/.gitea/actions/review@v1
+  with:
+    reviewer-name: security
+    persona: security
+    llm-model: claude-opus-4-20250514  # Security benefits from strong reasoning
+    ...
+```
+
+### Multiple Personas in Parallel
+
+```yaml
+jobs:
+  review:
+    strategy:
+      matrix:
+        include:
+          - name: security
+            persona: security
+          - name: architect
+            persona: architect
+    steps:
+      - uses: rodin/review-bot/.gitea/actions/review@v1
+        with:
+          reviewer-name: ${{ matrix.name }}
+          persona: ${{ matrix.persona }}
+          ...
+```
+
+Each persona posts independently with its own sentinel, so reviews don't interfere.
+
+
+### Custom Personas
+
+Create a YAML file with your domain-specific review focus:
+
+```yaml
+# .review/personas/trading.yaml
+name: trading
+display_name: Trading Domain Expert
+
+identity: |
+  You are a trading systems expert reviewing code for correctness.
+
+  Your expertise:
+  - Order lifecycle and state machines
+  - Fill handling and partial fills
+  - Position tracking and P&L calculations
+  - Event sourcing invariants
+
+focus:
+  - Order state machine correctness
+  - Fill handling edge cases (partial, overfill)
+  - Position and P&L calculation accuracy
+  - Event replay determinism
+  - Decimal precision for money
+
+ignore:
+  - Code style
+  - General performance
+  - Documentation formatting
+
+severity:
+  major: "Bugs that cause incorrect positions, fills, or money calculations"
+  minor: "Edge cases that could cause issues under unusual conditions"
+  nit: "Clarity improvements for domain logic"
+```
+
+Use it in CI:
+
+```yaml
+- uses: rodin/review-bot/.gitea/actions/review@v1
+  with:
+    reviewer-name: trading
+    persona-file: .review/personas/trading.yaml
+    ...
+```
+
+YAML is the recommended format for personas because it supports:
+- Multi-line strings with `|` blocks (cleaner identity definitions)
+- Comments for documentation
+- More readable arrays and nested structures
+
+JSON is also supported for backwards compatibility—just use `.json` extension.
+
+
+### Persona vs system-prompt-file
+
+| Feature | `persona` / `persona-file` | `system-prompt-file` |
+|---------|---------------------------|----------------------|
+| Replaces base prompt | Yes | No (appends) |
+| Structured format | Yes (YAML/JSON) | No (freeform) |
+| Focus/ignore lists | Yes | Manual |
+| Severity calibration | Yes | Manual |
+| Header display name | Yes | No |
+| Built-in options | Yes | No |
+
+Use personas for domain-specialized reviews. Use `system-prompt-file` for minor tweaks to the generic review.

cmd/review-bot/integration_test.go

@@ -130,7 +130,7 @@ func TestIntegration_PostAndCleanup(t *testing.T) {
 	// Post a test review
 	sentinel := "<!-- review-bot:integration-test -->"
 	testBody := "# Integration Test Review\n\nThis is a test review.\n\n" + sentinel
-	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "COMMENT", testBody, nil)
+	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, "COMMENT", testBody, "", nil)
 	if err != nil {
 		t.Fatalf("PostReview: %v", err)
 	}

cmd/review-bot/main.go

@@ -65,11 +65,19 @@ 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", "README.md"), "Comma-separated file paths to fetch from patterns repo")
+	patternsFiles := flag.String("patterns-files", envOrDefault("PATTERNS_FILES", ""), "Comma-separated file paths to fetch from patterns repo (empty = all files)")
 	dryRun := flag.Bool("dry-run", false, "Print review to stdout instead of posting")
 	llmTemp := flag.Float64("llm-temperature", envOrDefaultFloat("LLM_TEMPERATURE", 0), "LLM temperature (0 = server default)")
 	llmTimeout := flag.Int("llm-timeout", envOrDefaultInt("LLM_TIMEOUT", 300), "LLM request timeout in seconds (default 300)")
-	llmProvider := flag.String("llm-provider", envOrDefault("LLM_PROVIDER", "openai"), "LLM API provider: openai or anthropic")
+	llmProvider := flag.String("llm-provider", envOrDefault("LLM_PROVIDER", "openai"), "LLM API provider: openai, anthropic, or aicore")
+	personaName := flag.String("persona", envOrDefault("PERSONA", ""), "Built-in persona name (security, architect, docs)")
+	personaFile := flag.String("persona-file", envOrDefault("PERSONA_FILE", ""), "Path to persona JSON file")
+	// AI Core specific flags (only used when provider=aicore)
+	aicoreClientID := flag.String("aicore-client-id", envOrDefault("AICORE_CLIENT_ID", ""), "SAP AI Core client ID (for provider=aicore)")
+	aicoreClientSecret := flag.String("aicore-client-secret", envOrDefault("AICORE_CLIENT_SECRET", ""), "SAP AI Core client secret (for provider=aicore)")
+	aicoreAuthURL := flag.String("aicore-auth-url", envOrDefault("AICORE_AUTH_URL", ""), "SAP AI Core auth URL (for provider=aicore)")
+	aicoreAPIURL := flag.String("aicore-api-url", envOrDefault("AICORE_API_URL", ""), "SAP AI Core API URL (for provider=aicore)")
+	aicoreResourceGroup := flag.String("aicore-resource-group", envOrDefault("AICORE_RESOURCE_GROUP", "default"), "SAP AI Core resource group (for provider=aicore)")
 
 	flag.Parse()
 
@@ -84,12 +92,30 @@ func main() {
 	slog.Info("review-bot starting", "version", version)
 
 	// Validate required fields
-	if *giteaURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" ||
-		*llmBaseURL == "" || *llmAPIKey == "" || *llmModel == "" {
+	// For aicore provider, llm-base-url and llm-api-key are not required
+	isAICore := llm.Provider(*llmProvider) == llm.ProviderAICore
+	if *giteaURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" || *llmModel == "" {
 		fmt.Fprintf(os.Stderr, "Error: missing required flags or environment variables\n\n")
-		fmt.Fprintf(os.Stderr, "Required: --gitea-url, --repo, --pr, --reviewer-token, --llm-base-url, --llm-api-key, --llm-model\n")
+		fmt.Fprintf(os.Stderr, "Required: --gitea-url, --repo, --pr, --reviewer-token, --llm-model\n")
 		os.Exit(1)
 	}
+	if !isAICore && (*llmBaseURL == "" || *llmAPIKey == "") {
+		fmt.Fprintf(os.Stderr, "Error: --llm-base-url and --llm-api-key are required for provider=%s\n", *llmProvider)
+		os.Exit(1)
+	}
+	if isAICore && (*aicoreClientID == "" || *aicoreClientSecret == "" || *aicoreAuthURL == "" || *aicoreAPIURL == "") {
+		fmt.Fprintf(os.Stderr, "Error: AI Core credentials required for provider=aicore\n\n")
+		fmt.Fprintf(os.Stderr, "Required: --aicore-client-id, --aicore-client-secret, --aicore-auth-url, --aicore-api-url\n")
+		os.Exit(1)
+	}
+
+	// Validate persona flags are mutually exclusive
+	if *personaName != "" && *personaFile != "" {
+		slog.Error("--persona and --persona-file are mutually exclusive")
+		os.Exit(1)
+	}
+
+	// NOTE: Persona loading deferred until after Gitea client init to support repo personas
 
 	// Validate reviewer-name: only safe characters allowed in sentinel
 	if err := validateReviewerName(*reviewerName); err != nil {
@@ -125,8 +151,17 @@ func main() {
 	switch llm.Provider(*llmProvider) {
 	case llm.ProviderOpenAI, llm.ProviderAnthropic:
 		llmClient.WithProvider(llm.Provider(*llmProvider))
+	case llm.ProviderAICore:
+		llmClient.WithAICore(llm.AICoreConfig{
+			ClientID:      *aicoreClientID,
+			ClientSecret:  *aicoreClientSecret,
+			AuthURL:       *aicoreAuthURL,
+			APIURL:        *aicoreAPIURL,
+			ResourceGroup: *aicoreResourceGroup,
+		})
+		slog.Info("using SAP AI Core provider", "resource_group", *aicoreResourceGroup)
 	default:
-		slog.Error("invalid LLM provider", "provider", *llmProvider, "valid", "openai, anthropic")
+		slog.Error("invalid LLM provider", "provider", *llmProvider, "valid", "openai, anthropic, aicore")
 		os.Exit(1)
 	}
 	if *llmTimeout > 0 {
@@ -138,6 +173,43 @@ func main() {
 	ctx, cancel := context.WithTimeout(context.Background(), overallTimeout)
 	defer cancel()
 
+	// Load persona if specified (after Gitea client init to support repo personas)
+	var persona *review.Persona
+	if *personaName != "" {
+		// Try loading from repo first, then fall back to built-in
+		repoPersonas, err := review.LoadRepoPersonas(ctx, newGiteaClientAdapter(giteaClient), owner, repoName)
+		if err != nil {
+			slog.Warn("could not load repo personas", "repo", owner+"/"+repoName, "error", err)
+			// Continue with built-in personas only.
+			// NOTE: repoPersonas is nil here, but map indexing on a nil map is safe in Go
+			// (returns the zero value), so the fallback to built-in below works correctly.
+		}
+		if p, ok := repoPersonas[*personaName]; ok {
+			persona = p
+			slog.Info("loaded repo persona", "persona", persona.Name, "display", persona.DisplayName, "repo", owner+"/"+repoName)
+		} else {
+			// Fall back to built-in
+			persona, err = review.LoadBuiltinPersona(*personaName)
+			if err != nil {
+				slog.Error("failed to load persona", "persona", *personaName, "error", err)
+				os.Exit(1)
+			}
+			slog.Info("loaded built-in persona", "persona", persona.Name, "display", persona.DisplayName)
+		}
+	} else if *personaFile != "" {
+		resolvedPath, err := validateWorkspacePath(*personaFile, "persona-file")
+		if err != nil {
+			slog.Error("invalid persona-file path", "error", err)
+			os.Exit(1)
+		}
+		persona, err = review.LoadPersona(resolvedPath)
+		if err != nil {
+			slog.Error("failed to load persona file", "file", *personaFile, "error", err)
+			os.Exit(1)
+		}
+		slog.Info("loaded persona from file", "file", *personaFile, "persona", persona.Name)
+	}
+
 	slog.Info("reviewing pull request", "pr", prNumber, "repo", fmt.Sprintf("%s/%s", owner, repoName))
 
 	// Step 1: Fetch PR metadata
@@ -201,34 +273,14 @@ func main() {
 	// Step 6b: Load additional system prompt if specified
 	additionalPrompt := ""
 	if *systemPromptFile != "" {
-		workspace := os.Getenv("GITHUB_WORKSPACE")
-		if workspace == "" {
-			workspace, _ = os.Getwd()
-		}
-		absWorkspace, err := filepath.Abs(workspace)
+		resolvedPath, err := validateWorkspacePath(*systemPromptFile, "system-prompt-file")
 		if err != nil {
-			slog.Error("failed to resolve workspace path", "error", err)
-			os.Exit(1)
-		}
-		promptPath := filepath.Join(absWorkspace, *systemPromptFile)
-		promptPath = filepath.Clean(promptPath)
-		if !strings.HasPrefix(promptPath, absWorkspace+string(filepath.Separator)) && promptPath != absWorkspace {
-			slog.Error("system-prompt-file resolves outside workspace", "path", promptPath, "workspace", absWorkspace)
-			os.Exit(1)
-		}
-		// Resolve symlinks and re-validate to prevent symlink traversal
-		resolvedPath, err := filepath.EvalSymlinks(promptPath)
-		if err != nil {
-			slog.Error("failed to resolve system prompt file", "path", promptPath, "error", err)
-			os.Exit(1)
-		}
-		if !strings.HasPrefix(resolvedPath, absWorkspace+string(filepath.Separator)) && resolvedPath != absWorkspace {
-			slog.Error("system-prompt-file symlink resolves outside workspace", "resolved", resolvedPath, "workspace", absWorkspace)
+			slog.Error("invalid system-prompt-file path", "error", err)
 			os.Exit(1)
 		}
 		data, err := os.ReadFile(resolvedPath)
 		if err != nil {
-			slog.Error("failed to read system prompt file", "path", promptPath, "error", err)
+			slog.Error("failed to read system prompt file", "path", *systemPromptFile, "error", err)
 			os.Exit(1)
 		}
 		additionalPrompt = string(data)
@@ -236,7 +288,13 @@ func main() {
 	}
 
 	// Step 7: Budget-aware prompt assembly
-	systemBase := review.BuildSystemBase()
+	var systemBase string
+	if persona != nil {
+		systemBase = review.BuildPersonaSystemPrompt(persona)
+		slog.Debug("using persona system prompt", "persona", persona.Name)
+	} else {
+		systemBase = review.BuildSystemBase()
+	}
 	if additionalPrompt != "" {
 		systemBase += "\n\n## Additional Review Instructions\n\n" + additionalPrompt
 	}
@@ -293,7 +351,12 @@ func main() {
 	slog.Info("review parsed", "verdict", result.Verdict, "findings", len(result.Findings))
 
 	// Step 10: Format and post review
-	reviewBody := review.FormatMarkdown(result, *reviewerName)
+	var reviewBody string
+	if persona != nil && persona.DisplayName != "" {
+		reviewBody = review.FormatMarkdownWithDisplay(result, persona.DisplayName, *reviewerName)
+	} else {
+		reviewBody = review.FormatMarkdown(result, *reviewerName)
+	}
 
 	// Add commit footer so readers know which commit was evaluated
 	if pr.Head.Sha != "" {
@@ -315,6 +378,24 @@ func main() {
 
 	sentinel := fmt.Sprintf("<!-- review-bot:%s -->", *reviewerName)
 
+	// Stale check: verify HEAD hasn't moved since we started
+	evaluatedSHA := pr.Head.Sha
+	var currentSHA string
+	currentPR, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber)
+	if err != nil {
+		slog.Warn("could not re-fetch PR for stale check", "pr", prNumber, "error", err)
+		// currentSHA stays empty — shouldSkipStaleReview will return false
+	} else {
+		currentSHA = currentPR.Head.Sha
+	}
+	if shouldSkipStaleReview(evaluatedSHA, currentSHA) {
+		slog.Warn("HEAD moved during review — skipping stale review",
+			"evaluated", evaluatedSHA,
+			"current", currentSHA,
+			"pr", prNumber)
+		return
+	}
+
 	// Map findings to inline comments for lines present in the diff
 	diffRanges := gitea.ParseDiffNewLines(diff)
 	var inlineComments []gitea.ReviewComment
@@ -363,7 +444,7 @@ func main() {
 
 	// POST new review
 	slog.Info("posting review", "event", event, "pr", prNumber)
-	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, inlineComments)
+	posted, err := giteaClient.PostReview(ctx, owner, repoName, prNumber, event, reviewBody, evaluatedSHA, inlineComments)
 	if err != nil {
 		slog.Error("failed to post review", "pr", prNumber, "event", event, "error", err)
 		os.Exit(1)
@@ -442,11 +523,25 @@ 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 {
@@ -463,12 +558,10 @@ func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patt
 		}
 		owner, repo := parts[0], parts[1]
 
-		for _, path := range paths {
-			path = strings.TrimSpace(path)
-			if path == "" {
-				continue
-			}
+		var repoLoadedFiles []string
+		var repoSkippedFiles []string
 
+		for _, path := range paths {
 			files, err := client.GetAllFilesInPath(ctx, owner, repo, path)
 			if err != nil {
 				slog.Warn("could not fetch patterns", "path", path, "repo", repoRef, "error", err)
@@ -478,11 +571,22 @@ func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patt
 			for filePath, content := range files {
 				// Only include markdown and text files as patterns
 				if !isPatternFile(filePath) {
+					repoSkippedFiles = append(repoSkippedFiles, filePath)
 					continue
 				}
+				repoLoadedFiles = append(repoLoadedFiles, filePath)
 				sb.WriteString(fmt.Sprintf("### %s/%s\n\n%s\n\n", repoRef, filePath, content))
 			}
 		}
+
+		if len(repoLoadedFiles) > 0 {
+			slog.Info("loaded pattern files", "repo", repoRef, "count", len(repoLoadedFiles), "files", repoLoadedFiles)
+		} else {
+			slog.Warn("no pattern files loaded", "repo", repoRef, "paths", paths)
+		}
+		if len(repoSkippedFiles) > 0 {
+			slog.Debug("skipped non-pattern files", "repo", repoRef, "count", len(repoSkippedFiles), "files", repoSkippedFiles)
+		}
 	}
 	return sb.String()
 }
@@ -569,6 +673,43 @@ func validateReviewerName(name string) error {
 	return nil
 }
 
+// validateWorkspacePath ensures a file path is within the workspace and resolves
+// symlinks to prevent traversal attacks. Returns the resolved absolute path or
+// an error if the path is outside the workspace.
+func validateWorkspacePath(path, pathName string) (string, error) {
+	workspace := os.Getenv("GITHUB_WORKSPACE")
+	if workspace == "" {
+		workspace, _ = os.Getwd()
+	}
+	absWorkspace, err := filepath.Abs(workspace)
+	if err != nil {
+		return "", fmt.Errorf("failed to resolve workspace path: %w", err)
+	}
+
+	// Join and clean the path
+	fullPath := filepath.Join(absWorkspace, path)
+	fullPath = filepath.Clean(fullPath)
+
+	// Check path is within workspace using filepath.Rel (more robust than HasPrefix)
+	rel, err := filepath.Rel(absWorkspace, fullPath)
+	if err != nil || strings.HasPrefix(rel, "..") {
+		return "", fmt.Errorf("%s resolves outside workspace: path=%s workspace=%s", pathName, fullPath, absWorkspace)
+	}
+
+	// Resolve symlinks and re-validate to prevent symlink traversal
+	resolvedPath, err := filepath.EvalSymlinks(fullPath)
+	if err != nil {
+		return "", fmt.Errorf("failed to resolve %s: %w", pathName, err)
+	}
+
+	relResolved, err := filepath.Rel(absWorkspace, resolvedPath)
+	if err != nil || strings.HasPrefix(relResolved, "..") {
+		return "", fmt.Errorf("%s symlink resolves outside workspace: resolved=%s workspace=%s", pathName, resolvedPath, absWorkspace)
+	}
+
+	return resolvedPath, nil
+}
+
 // buildSupersededBody creates the body for a superseded review: struck-through banner
 // with collapsed original content and the commit it was evaluated against.
 func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string) string {
@@ -666,3 +807,45 @@ func findAllOwnReviews(reviews []gitea.Review, sentinel string) []gitea.Review {
 	}
 	return result
 }
+
+// shouldSkipStaleReview reports whether to skip posting because HEAD moved.
+// Returns true (skip) if evaluatedSHA differs from currentSHA.
+// Returns false (don't skip) if:
+//   - SHAs match (no movement)
+//   - currentSHA is empty (re-fetch failed; prefer posting stale over failing)
+func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool {
+	if currentSHA == "" {
+		// Re-fetch failed; better to post potentially stale than fail
+		return false
+	}
+	return evaluatedSHA != currentSHA
+}
+
+// giteaClientAdapter adapts gitea.Client to review.GiteaClient interface.
+type giteaClientAdapter struct {
+	client *gitea.Client
+}
+
+func newGiteaClientAdapter(c *gitea.Client) *giteaClientAdapter {
+	return &giteaClientAdapter{client: c}
+}
+
+func (a *giteaClientAdapter) ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error) {
+	entries, err := a.client.ListContents(ctx, owner, repo, path)
+	if err != nil {
+		return nil, err
+	}
+	result := make([]review.ContentEntry, len(entries))
+	for i, e := range entries {
+		result[i] = review.ContentEntry{
+			Name: e.Name,
+			Path: e.Path,
+			Type: e.Type,
+		}
+	}
+	return result, nil
+}
+
+func (a *giteaClientAdapter) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
+	return a.client.GetFileContent(ctx, owner, repo, filepath)
+}

cmd/review-bot/main_test.go

@@ -6,6 +6,7 @@ import (
 	"log/slog"
 	"os"
 	"os/exec"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -45,6 +46,114 @@ func TestValidateReviewerName(t *testing.T) {
 	}
 }
 
+func TestValidateWorkspacePath(t *testing.T) {
+	// Create a temp directory as our workspace
+	tmpDir := t.TempDir()
+
+	// Create a valid file inside the workspace
+	validFile := filepath.Join(tmpDir, "valid.json")
+	if err := os.WriteFile(validFile, []byte("{}"), 0644); err != nil {
+		t.Fatalf("failed to create test file: %v", err)
+	}
+
+	// Create a subdirectory with a file
+	subDir := filepath.Join(tmpDir, "subdir")
+	if err := os.MkdirAll(subDir, 0755); err != nil {
+		t.Fatalf("failed to create subdir: %v", err)
+	}
+	nestedFile := filepath.Join(subDir, "nested.json")
+	if err := os.WriteFile(nestedFile, []byte("{}"), 0644); err != nil {
+		t.Fatalf("failed to create nested file: %v", err)
+	}
+
+	// Create a symlink pointing outside the workspace
+	symlinkPath := filepath.Join(tmpDir, "evil-symlink.json")
+	if err := os.Symlink("/etc/passwd", symlinkPath); err != nil {
+		t.Fatalf("failed to create symlink: %v", err)
+	}
+
+	// Save and restore GITHUB_WORKSPACE
+	origWorkspace := os.Getenv("GITHUB_WORKSPACE")
+	defer os.Setenv("GITHUB_WORKSPACE", origWorkspace)
+
+	tests := []struct {
+		name      string
+		workspace string
+		path      string
+		wantErr   bool
+		errMatch  string
+	}{
+		{
+			name:      "valid relative path",
+			workspace: tmpDir,
+			path:      "valid.json",
+			wantErr:   false,
+		},
+		{
+			name:      "valid nested path",
+			workspace: tmpDir,
+			path:      "subdir/nested.json",
+			wantErr:   false,
+		},
+		{
+			name:      "path traversal attempt",
+			workspace: tmpDir,
+			path:      "../../../etc/passwd",
+			wantErr:   true,
+			errMatch:  "resolves outside workspace",
+		},
+		{
+			name:      "absolute path normalized to workspace-relative",
+			workspace: tmpDir,
+			path:      "/etc/passwd",
+			wantErr:   true,
+			// Go 1.21+ filepath.Join normalizes absolute paths: Join("/tmp/x", "/etc/passwd")
+			// becomes "/tmp/x/etc/passwd", which is within workspace but doesn't exist.
+			errMatch: "failed to resolve",
+		},
+		{
+			name:      "nonexistent file",
+			workspace: tmpDir,
+			path:      "nonexistent.json",
+			wantErr:   true,
+			errMatch:  "failed to resolve",
+		},
+		{
+			name:      "symlink escaping workspace",
+			workspace: tmpDir,
+			path:      "evil-symlink.json",
+			wantErr:   true,
+			errMatch:  "symlink resolves outside workspace",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			os.Setenv("GITHUB_WORKSPACE", tc.workspace)
+			resolved, err := validateWorkspacePath(tc.path, "test-file")
+
+			if tc.wantErr {
+				if err == nil {
+					t.Errorf("expected error for %q, got nil", tc.path)
+				} else if tc.errMatch != "" && !strings.Contains(err.Error(), tc.errMatch) {
+					t.Errorf("error %q should contain %q", err.Error(), tc.errMatch)
+				}
+			} else {
+				if err != nil {
+					t.Errorf("expected no error for %q, got %v", tc.path, err)
+				}
+				if resolved == "" {
+					t.Error("expected non-empty resolved path")
+				}
+				// Verify resolved path is within workspace
+				if !strings.HasPrefix(resolved, tc.workspace) {
+					t.Errorf("resolved path %q not within workspace %q", resolved, tc.workspace)
+				}
+			}
+		})
+	}
+}
+
 func makeReview(id int64, login, state string, stale bool, body string) gitea.Review {
 	r := gitea.Review{
 		ID:    id,
@@ -56,7 +165,6 @@ func makeReview(id int64, login, state string, stale bool, body string) gitea.Re
 	return r
 }
 
-
 func TestBuildSupersededBody(t *testing.T) {
 	original := "# Review\n\nLooks good.\n\n<!-- review-bot:sonnet -->"
 	sentinel := "<!-- review-bot:sonnet -->"
@@ -396,6 +504,52 @@ 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
@@ -626,8 +780,8 @@ func TestExtractSentinelName_EdgeCases(t *testing.T) {
 		{"<!-- review-bot:sonnet --> rest", "sonnet"},
 		{"<!-- review-bot:gpt-review --> rest", "gpt-review"},
 		{"no sentinel here", "unknown"},
-		{"<!-- review-bot:", "unknown"},       // prefix but no suffix
-		{"prefix <!-- review-bot:abc --> end", "abc"}, // embedded in text
+		{"<!-- review-bot:", "unknown"},                   // prefix but no suffix
+		{"prefix <!-- review-bot:abc --> end", "abc"},     // embedded in text
 	}
 
 	for _, tc := range tests {
@@ -862,3 +1016,53 @@ func TestFindAllOwnReviews(t *testing.T) {
 		}
 	}
 }
+
+func TestShouldSkipStaleReview(t *testing.T) {
+	tests := []struct {
+		name         string
+		evaluatedSHA string
+		currentSHA   string
+		wantSkip     bool
+	}{
+		{
+			name:         "matching SHAs",
+			evaluatedSHA: "abc123def456",
+			currentSHA:   "abc123def456",
+			wantSkip:     false,
+		},
+		{
+			name:         "different SHAs",
+			evaluatedSHA: "abc123def456",
+			currentSHA:   "xyz789abc123",
+			wantSkip:     true,
+		},
+		{
+			name:         "empty current SHA (re-fetch failed)",
+			evaluatedSHA: "abc123def456",
+			currentSHA:   "",
+			wantSkip:     false,
+		},
+		{
+			name:         "both empty (edge case)",
+			evaluatedSHA: "",
+			currentSHA:   "",
+			wantSkip:     false,
+		},
+		{
+			name:         "only current empty",
+			evaluatedSHA: "abc123",
+			currentSHA:   "",
+			wantSkip:     false,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := shouldSkipStaleReview(tc.evaluatedSHA, tc.currentSHA)
+			if got != tc.wantSkip {
+				t.Errorf("shouldSkipStaleReview(%q, %q) = %v, want %v",
+					tc.evaluatedSHA, tc.currentSHA, got, tc.wantSkip)
+			}
+		})
+	}
+}

docs/DESIGN-51-personas.md

@@ -0,0 +1,334 @@
+# Design: Role-based Review Personas (Issue #51)
+
+> **Note:** This design was revised during implementation to use JSON instead of YAML
+> to maintain the repository's zero-external-dependencies convention. All persona
+> files use JSON format. See "Design Revision" section at the end for details.
+
+## Problem
+
+Current review-bot performs generic code review. Every reviewer (regardless of `reviewer-name`) uses the same base prompt and evaluates the same concerns. This leads to:
+
+1. **Redundancy** — Two reviewers (e.g., GPT + Claude twins) often flag identical issues
+2. **Gaps** — Generic reviewers miss specialized concerns (security, domain logic, architecture)
+3. **Noise** — NITs about style mixed with critical security findings
+4. **No ownership** — Findings lack clear domain attribution
+
+## Constraints
+
+- Must work with existing CLI flags and CI workflow patterns
+- Must not break backwards compatibility (existing configs still work)
+- Must integrate cleanly with the budget system (personas add to context)
+- Multiple personas running in parallel must not interfere with each other
+- Each persona must have clear scope boundaries (no duplication)
+
+## Proposed Approach
+
+### 1. Persona Definition
+
+A persona is a named review role with:
+- **Identity** — Who am I? What's my expertise?
+- **Focus** — What do I look for?
+- **Scope boundaries** — What do I explicitly NOT comment on?
+- **Severity calibration** — What counts as MAJOR/MINOR/NIT for MY domain?
+
+Personas are defined in JSON files that can live:
+1. In the pattern repos (shared across projects)
+2. In the target repo (project-specific personas)
+3. Inline via a new `--persona-file` flag (JSON format)
+
+### 2. Persona File Format
+
+```json
+# .review/personas/security.yaml
+name: security
+display_name: Security Specialist
+model_preference: opus  # optional hint for expensive analysis
+
+identity: |
+  You are a security specialist reviewing code for vulnerabilities.
+  Your expertise: OWASP Top 10, injection attacks, auth/authz, secrets management,
+  event sourcing security (replay attacks, event injection).
+
+focus:
+  - Injection attacks (SQL, command, path traversal, template)
+  - Authentication and authorization gaps
+  - Secrets exposure (hardcoded credentials, tokens in logs)
+  - Input validation (unsanitized input, unsafe deserialization)
+  - Race conditions with security implications
+  - Event sourcing attack vectors
+
+ignore:
+  - Code style and naming conventions
+  - Performance (unless security-related)
+  - Documentation
+  - General code quality
+  - Test coverage
+
+severity:
+  critical: "Remote code execution, auth bypass, data exfiltration"
+  major: "Privilege escalation, information disclosure, DoS"
+  minor: "Missing rate limiting, verbose errors"
+  nit: "Theoretical risk with low exploitability"
+
+output_format: |
+  For each finding:
+  - Severity: [CRITICAL|MAJOR|MINOR|NIT]
+  - Attack vector: How could this be exploited?
+  - Evidence: Code snippet showing the vulnerability
+  - Recommendation: Specific fix
+```
+
+### 3. New CLI Flags
+
+```
+--persona-file PATH      Path to persona JSON file (local or in repo)
+--persona NAME           Built-in persona name (security, architect, domain)
+```
+
+Either flag sets the persona. If neither is provided, behavior is unchanged (generic review).
+
+### 4. Prompt Assembly
+
+Current flow:
+```
+SystemBase → Patterns → Conventions → [LLM]
+```
+
+New flow with persona:
+```
+PersonaPrompt (from YAML) → Patterns (filtered?) → Conventions → [LLM]
+```
+
+The persona's identity/focus/ignore/severity sections become the system prompt, replacing the generic "You are an expert code reviewer" base.
+
+### 5. Built-in Personas
+
+Ship with these built-in personas (loadable via `--persona NAME`):
+
+| Name | Focus |
+|------|-------|
+| `security` | Vulnerabilities, auth, secrets |
+| `architect` | Patterns, consistency, design |
+| `domain` | Business logic (requires repo-specific config) |
+| `docs` | Documentation, API clarity |
+
+Built-in personas live in `review/personas/` as embedded Go assets or YAML shipped with the binary.
+
+### 6. CI Workflow Integration
+
+Single persona:
+```yaml
+- uses: rodin/review-bot/.gitea/actions/review@v1
+  with:
+    reviewer-name: security
+    persona: security
+    ...
+```
+
+Multiple personas (parallel jobs):
+```yaml
+jobs:
+  review:
+    strategy:
+      matrix:
+        include:
+          - name: security
+            persona: security
+          - name: architect
+            persona: architect
+    steps:
+      - uses: rodin/review-bot/.gitea/actions/review@v1
+        with:
+          reviewer-name: ${{ matrix.name }}
+          persona: ${{ matrix.persona }}
+```
+
+Custom persona from repo:
+```yaml
+- uses: rodin/review-bot/.gitea/actions/review@v1
+  with:
+    reviewer-name: trading
+    persona-file: .review/personas/trading.yaml
+```
+
+### 7. Persona + Patterns Interaction
+
+Some personas benefit from filtered patterns:
+- Security → only security-related patterns
+- Architect → all patterns (structural focus)
+- Domain → domain docs, not language patterns
+
+For v1, keep it simple: all patterns are included regardless of persona. Future enhancement could add `patterns_filter` to persona YAML.
+
+### 8. Output Format Changes
+
+Persona name appears in the review header:
+```markdown
+# Security Review
+
+## Summary
+No critical vulnerabilities found in this change.
+
+## Findings
+| # | Severity | File | Line | Finding |
+...
+
+## Recommendation
+**APPROVE** — No security-relevant issues detected.
+
+---
+*Review by security*
+<!-- review-bot:security -->
+```
+
+## State/Data Model
+
+### Persona struct
+
+```go
+// review/persona.go
+type Persona struct {
+    Name          string   `yaml:"name"`
+    DisplayName   string   `yaml:"display_name"`
+    ModelPref     string   `yaml:"model_preference,omitempty"`
+    Identity      string   `yaml:"identity"`
+    Focus         []string `yaml:"focus"`
+    Ignore        []string `yaml:"ignore"`
+    Severity      Severity `yaml:"severity"`
+    OutputFormat  string   `yaml:"output_format,omitempty"`
+}
+
+type Severity struct {
+    Critical string `yaml:"critical"`
+    Major    string `yaml:"major"`
+    Minor    string `yaml:"minor"`
+    Nit      string `yaml:"nit"`
+}
+```
+
+### Loading precedence
+
+1. `--persona-file PATH` → load from local file system
+2. `--persona NAME` → load from embedded built-ins
+3. Neither → use generic system prompt (current behavior)
+
+## Error Cases
+
+| Error | Handling |
+|-------|----------|
+| Persona file not found | Fatal exit with clear message |
+| Invalid YAML in persona file | Fatal exit with parse error |
+| Both `--persona` and `--persona-file` specified | Fatal exit: mutually exclusive |
+| Unknown built-in persona name | Fatal exit with list of valid names |
+| Empty identity in persona | Warning, fall back to generic prompt |
+
+## Edge Cases
+
+- **Empty focus list**: Valid — persona relies on identity alone
+- **Empty ignore list**: Valid — no explicit scope exclusions
+- **No severity section**: Use default MAJOR/MINOR/NIT definitions
+- **Model preference set but budget insufficient**: Ignore preference, log warning
+- **Persona file in pattern repo**: Fetch like other pattern files
+
+## Testing Strategy
+
+### Unit tests
+- `persona_test.go`: Parse valid/invalid YAML, validate required fields
+- `prompt_test.go`: Verify persona prompt assembly
+- Integration with budget: persona prompts count toward token limit
+
+### Integration tests
+- End-to-end with `--persona security` (built-in)
+- End-to-end with `--persona-file custom.yaml`
+- Backwards compatibility: no flags = generic behavior
+
+### Manual verification
+- Run security persona on a PR with obvious vulnerability
+- Verify security persona ignores style issues
+- Verify non-security persona doesn't flag security issues
+
+## Implementation Phases
+
+### Phase 1: Persona types and loading
+- [ ] `review/persona.go`: Persona struct + YAML parsing
+- [ ] `review/persona_test.go`: Unit tests
+- [ ] Embed built-in personas in binary
+- [ ] Compiles clean, tests pass
+
+### Phase 2: Prompt generation
+- [ ] `review/prompt.go`: `BuildPersonaPrompt(p Persona) string`
+- [ ] Modify `BuildSystemBase()` to accept optional persona
+- [ ] Integrate persona prompt with budget system
+- [ ] Tests for prompt assembly
+
+### Phase 3: CLI integration
+- [ ] Add `--persona` and `--persona-file` flags
+- [ ] Flag validation (mutually exclusive, valid names)
+- [ ] Load persona based on flags
+- [ ] Pass persona to prompt builder
+
+### Phase 4: Action integration
+- [ ] Add `persona` and `persona-file` inputs to action.yml
+- [ ] Update README with persona examples
+- [ ] End-to-end CI test
+
+### Phase 5: Built-in personas
+- [ ] `security.yaml` built-in
+- [ ] `architect.yaml` built-in
+- [ ] `docs.yaml` built-in
+- [ ] Document each persona's focus
+
+## Open Questions
+
+1. **Persona file location in repo**: Should we support `--persona-file .review/security.yaml` where the file is fetched from the PR's repo (like conventions)? This adds complexity but enables project-specific personas without action changes.
+
+2. **Model preference enforcement**: If persona specifies `model_preference: opus` but the action uses a different model, should we warn? Override? Ignore? Current thinking: log warning, use the specified model (user controls model via action input).
+
+3. **Severity override output**: If persona defines custom severity levels (CRITICAL), should the JSON output include them, or map back to standard MAJOR/MINOR/NIT? Current thinking: keep standard output format, use severity calibration only for prompt guidance.
+
+## Completion Checklist
+
+1. Persona struct matches YAML schema exactly?
+2. Built-in personas embedded in binary (not external files)?
+3. `--persona` and `--persona-file` are mutually exclusive?
+4. Unknown persona name produces clear error with valid options?
+5. Empty persona file fields have sensible defaults?
+6. Persona prompt integrates with budget system (token counting)?
+7. Backwards compatibility: no flags = current behavior?
+8. Review header shows persona display name?
+9. Sentinel still uses reviewer-name (not persona name)?
+10. Unit tests cover parse errors, missing fields, valid YAML?
+
+## Design Review Findings (Self-Review)
+
+### Finding 1: Severity Mapping
+The persona YAML allows `critical` severity, but the LLM output parser (`review/parser.go`) only accepts MAJOR/MINOR/NIT. 
+
+**Resolution:** Keep standard output format. Persona severity section is ONLY for calibrating the LLM's judgment (prompt guidance). Output must still use MAJOR/MINOR/NIT. Document this clearly in persona format docs.
+
+### Finding 2: Embedding Built-in Personas
+Go doesn't natively embed YAML. Must use `//go:embed` directive (Go 1.16+).
+
+**Resolution:** Create `review/personas/` directory with YAML files and use:
+```go
+//go:embed personas/*.yaml
+var embeddedPersonas embed.FS
+```
+
+### Finding 3: display_name vs reviewer-name
+Design says header shows "persona display name" but sentinel uses "reviewer-name". This is correct - they serve different purposes:
+- `display_name` → human-readable header ("Security Specialist Review")
+- `reviewer-name` → machine sentinel for cleanup (`<!-- review-bot:security -->`)
+
+When persona is used, `display_name` takes precedence for the header title, but `reviewer-name` (CLI flag) is still used for the sentinel.
+
+## Design Revision: YAML with gopkg.in/yaml.v3
+
+**Decision:** Add `gopkg.in/yaml.v3` as a dependency.
+
+YAML is preferred over JSON for persona files because:
+- Multi-line strings are cleaner (no escaping quotes in identity/focus text)
+- Comments are supported for documentation
+- More human-readable for complex persona definitions
+
+The implementation supports both YAML (`.yaml`, `.yml`) and JSON (`.json`) for backwards compatibility, with YAML as the default for built-in personas.

docs/DESIGN-57-yaml-persona.md

@@ -0,0 +1,87 @@
+# Design: YAML Support for Persona Files (#57)
+
+## Problem
+
+JSON is awkward for persona files that contain multi-line text (identity, severity descriptions). YAML supports cleaner multi-line strings and comments, improving readability and maintainability.
+
+## Constraints
+
+- 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
+
+## Proposed Approach
+
+1. **Update `parsePersona`** to detect format from file extension
+2. **Add YAML parsing** with explicit depth limit (defense in depth)
+3. **Keep JSON as fallback** for files without `.yaml`/`.yml` extension
+4. **Convert built-in personas** to YAML format
+5. **Update embed directive** to include both formats
+
+### File Extension Detection
+
+```go
+func parsePersona(data []byte, source string) (*Persona, error) {
+    isYAML := strings.HasSuffix(source, ".yaml") || strings.HasSuffix(source, ".yml")
+    if isYAML {
+        return parseYAML(data, source)
+    }
+    return parseJSON(data, source)
+}
+```
+
+### YAML Parsing with Depth Protection
+
+We implement a custom AST-based depth/node-count walk (`checkYAMLDepth` in
+`review/persona.go`) rather than relying on library decoder options. Key design
+decisions:
+
+- **Library:** `github.com/goccy/go-yaml` with `ast.Node`-based traversal
+- **Dual-map tracking:** `validated` (depth-aware short-circuit) + `visiting` (cycle detection)
+- **Node-count limit:** Conservative overcounting bounds total validation work
+- **Alias-aware depth:** Aliases increment depth and are re-checked when encountered at greater depths
+
+See `review/persona.go:checkYAMLDepth` for the authoritative implementation.
+
+## State/Data Model
+
+No new state. Same `Persona` struct, just different parsing.
+
+## Error Cases
+
+| Error | Handling |
+|-------|----------|
+| Invalid YAML syntax | Return parse error with source file |
+| Deeply nested YAML | Custom AST walk (`checkYAMLDepth`) rejects before decode |
+| Unknown extension | Fall back to JSON parsing |
+| Missing required fields | Validation rejects after parse |
+
+## Edge Cases
+
+- File with `.json` extension but YAML content → JSON parse fails, user sees error
+- File with no extension → defaults to JSON
+- Embedded persona reference like `builtin:security` → detect by embed path (`personas/X.yaml`)
+
+## Testing Strategy
+
+1. Unit tests for YAML parsing (valid, invalid, deeply nested)
+2. Unit tests for extension detection
+3. Integration test for built-in personas (now YAML)
+4. Backwards compat test: verify JSON still works for external files
+
+## Completion Checklist
+
+1. [ ] `go-yaml` dependency added at v1.16.0+
+2. [ ] Extension detection uses case-insensitive comparison
+3. [ ] YAML parse errors include source file name
+4. [ ] JSON parsing still works for `.json` files
+5. [ ] Built-in personas converted to YAML with readable multi-line strings
+6. [ ] Embed directive updated to include `*.yaml`
+7. [ ] Test for deeply nested YAML rejection
+8. [ ] All existing tests pass
+
+## Open Questions
+
+- Should we support both `.yaml` AND `.yml`? Issue says `.yaml` only for consistency, but some users expect `.yml`. **Decision:** Support both for reading, recommend `.yaml` in docs.
+- Should we add a "format" field to detect mismatched extension/content? **Decision:** No, keep it simple. Extension determines format.

gitea/client.go

@@ -11,9 +11,12 @@ import (
 	"fmt"
 	"io"
 	"log/slog"
+	"math"
+	"net"
 	"net/http"
 	"net/url"
 	"strings"
+	"syscall"
 	"time"
 )
 
@@ -39,12 +42,68 @@ func IsNotFound(err error) bool {
 	return errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound
 }
 
+// IsServerError reports whether an error is an API 5xx response.
+func IsServerError(err error) bool {
+	var apiErr *APIError
+	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 {
 	baseURL string
 	token   string
 	http    *http.Client
+
+	// RetryBackoff defines the delays between retry attempts.
+	// RetryBackoff[i] is the delay before attempt i+1 (after attempt i fails).
+	// If nil, defaults to {1s, 2s}. Set to shorter durations in tests.
+	//
+	// 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
+}
+
+// defaultCheckRedirect is the redirect policy used by NewClient.
+// NOTE: This function is intentionally duplicated in github/client.go (and vice versa)
+// because the packages are separate. Changes here must be mirrored there.
+// It rejects HTTPS->HTTP protocol downgrades (to prevent plaintext leakage)
+// and cross-host redirects (to prevent following responses from untrusted
+// endpoints). Same-host, same-or-upgraded-scheme redirects are allowed.
+func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
+	if len(via) >= 10 {
+		return fmt.Errorf("stopped after 10 redirects")
+	}
+	// Guard for direct invocation in tests and any future callers;
+	// net/http guarantees len(via) >= 1 during actual redirects.
+	if len(via) == 0 {
+		return nil
+	}
+	prev := via[len(via)-1]
+	// Reject protocol downgrade: HTTPS->HTTP leaks request metadata over plaintext.
+	if prev.URL.Scheme == "https" && req.URL.Scheme == "http" {
+		return fmt.Errorf("refusing redirect: HTTPS to HTTP downgrade (%s -> %s)", prev.URL.Host, req.URL.Host)
+	}
+	// Reject cross-host redirect entirely to avoid consuming responses
+	// from untrusted endpoints.
+	if req.URL.Host != prev.URL.Host {
+		return fmt.Errorf("refusing redirect: cross-host (%s -> %s)", prev.URL.Host, req.URL.Host)
+	}
+	return nil
 }
 
 // NewClient creates a new Gitea API client.
@@ -52,10 +111,33 @@ func NewClient(baseURL, token string) *Client {
 	return &Client{
 		baseURL: strings.TrimRight(baseURL, "/"),
 		token:   token,
-		http:    &http.Client{Timeout: 30 * time.Second},
+		http: &http.Client{
+			Timeout:       30 * time.Second,
+			CheckRedirect: defaultCheckRedirect,
+		},
 	}
 }
 
+// SetHTTPClient sets the underlying HTTP client used for requests.
+// This is intended for test setup only to inject mock transports; it must be
+// called before any goroutines issue requests.
+//
+// Passing nil restores the default client (30s timeout + redirect-rejecting
+// CheckRedirect policy matching NewClient).
+//
+// Callers providing a non-nil client are responsible for configuring a safe
+// CheckRedirect policy. Without one, the default net/http behavior will follow
+// redirects and may forward the Authorization header to untrusted hosts.
+func (c *Client) SetHTTPClient(hc *http.Client) {
+	if hc == nil {
+		hc = &http.Client{
+			Timeout:       30 * time.Second,
+			CheckRedirect: defaultCheckRedirect,
+		}
+	}
+	c.http = hc
+}
+
 // PullRequest holds relevant PR metadata.
 type PullRequest struct {
 	Title string `json:"title"`
@@ -103,9 +185,28 @@ 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)
-	body, err := c.doGet(ctx, reqURL)
+
+	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)
 	}
@@ -161,18 +262,22 @@ func (c *Client) GetFileContentRef(ctx context.Context, owner, repo, filepath, r
 }
 
 // PostReview submits a review to a PR and returns the created review.
-// event should be "APPROVED" or "REQUEST_CHANGES".
+// event should be one of "APPROVED", "REQUEST_CHANGES", or "COMMENT".
+// commitID anchors the review to a specific commit SHA. If empty, Gitea
+// defaults to the current PR head.
 // comments are optional inline comments attached to specific lines.
-func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body string, comments []ReviewComment) (*Review, error) {
+func (c *Client) PostReview(ctx context.Context, owner, repo string, number int, event, body, commitID string, comments []ReviewComment) (*Review, error) {
 	reqURL := fmt.Sprintf("%s/api/v1/repos/%s/%s/pulls/%d/reviews", c.baseURL, url.PathEscape(owner), url.PathEscape(repo), number)
 
 	payload := struct {
 		Body     string          `json:"body"`
 		Event    string          `json:"event"`
+		CommitID string          `json:"commit_id,omitempty"`
 		Comments []ReviewComment `json:"comments,omitempty"`
 	}{
 		Body:     body,
 		Event:    event,
+		CommitID: commitID,
 		Comments: comments,
 	}
 
@@ -210,24 +315,218 @@ func (c *Client) PostReview(ctx context.Context, owner, repo string, number int,
 	return &review, nil
 }
 
+// isTemporaryNetError reports whether err is a temporary network error worth retrying.
+// This includes connection refused, network unreachable, connection reset, and DNS
+// timeouts. It explicitly excludes permanent errors like permission denied or
+// "no such host" DNS failures.
+func isTemporaryNetError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	// Check for OpError and inspect the underlying syscall error.
+	// Not all OpErrors are transient — permission denied, for example, is permanent.
+	var opErr *net.OpError
+	if errors.As(err, &opErr) {
+		return isRetriableSyscallError(opErr.Err)
+	}
+
+	// DNS errors: only retry on timeout, not on "no such host" which is permanent.
+	var dnsErr *net.DNSError
+	if errors.As(err, &dnsErr) {
+		return dnsErr.IsTimeout
+	}
+
+	// Check for net.Error with Timeout() (Temporary is deprecated)
+	var netErr net.Error
+	if errors.As(err, &netErr) {
+		return netErr.Timeout()
+	}
+
+	return false
+}
+
+// isRetriableSyscallError reports whether the underlying error from a net.OpError
+// is a transient syscall error worth retrying.
+func isRetriableSyscallError(err error) bool {
+	if err == nil {
+		return false
+	}
+
+	// Check for syscall.Errno directly or wrapped
+	var errno syscall.Errno
+	if errors.As(err, &errno) {
+		switch errno {
+		case syscall.ECONNREFUSED, // connection refused — server not listening
+			syscall.ECONNRESET,   // connection reset by peer
+			syscall.ENETUNREACH,  // network unreachable
+			syscall.EHOSTUNREACH, // host unreachable
+			syscall.ETIMEDOUT:    // connection timed out
+			return true
+		default:
+			// EACCES, EPERM, etc. are permanent — don't retry
+			return false
+		}
+	}
+
+	// If we can't identify the specific syscall error, be conservative and retry.
+	// This handles wrapped errors or platform-specific error types.
+	// The retry count is limited, so erring on the side of retrying is safe.
+	return true
+}
+
+// redactURL strips query parameters and userinfo credentials from a URL for
+// safe logging. This prevents accidental exposure of sensitive data (tokens in
+// query strings, or user:pass in the authority) in log output.
+func redactURL(rawURL string) string {
+	parsed, err := url.Parse(rawURL)
+	if err != nil {
+		// If we cannot parse it, return a safe placeholder rather than
+		// potentially logging something sensitive.
+		return "[invalid URL]"
+	}
+	if parsed.User != nil {
+		parsed.User = url.User("REDACTED")
+	}
+	if parsed.RawQuery != "" {
+		parsed.RawQuery = "[redacted]"
+	}
+	return parsed.String()
+}
+
+// sanitizeErrorForLog returns a loggable version of an error that omits
+// potentially sensitive content like response bodies. For APIError, only
+// the status code is included; for other errors, the type is preserved.
+func sanitizeErrorForLog(err error) string {
+	if err == nil {
+		return "<nil>"
+	}
+	var apiErr *APIError
+	if errors.As(err, &apiErr) {
+		return fmt.Sprintf("HTTP %d", apiErr.StatusCode)
+	}
+	return err.Error()
+}
+
+// doGetWithReader performs an HTTP GET request with retry on 5xx errors and
+// temporary network errors. Retries up to 3 times with exponential backoff
+// (1s, 2s delays by default; configurable via Client.RetryBackoff for testing).
+// The readBody function is called with the response body on success (2xx) and
+// 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.
+	backoff := c.RetryBackoff
+	if backoff == nil {
+		backoff = []time.Duration{1 * time.Second, 2 * time.Second}
+	}
+
+	// maxErrorBodyBytes limits how much of an error response body we read
+	// to protect against malicious servers sending unbounded data.
+	const maxErrorBodyBytes = 64 * 1024 // 64 KB
+
+	var lastErr error
+	for attempt := 0; attempt < maxAttempts; attempt++ {
+		if attempt > 0 {
+			// Determine delay: use backoff slice if available, otherwise retry immediately.
+			// An empty RetryBackoff slice means "retry without delay" — this is intentional
+			// as the caller explicitly configured no delays.
+			var delay time.Duration
+			if attempt-1 < len(backoff) {
+				delay = backoff[attempt-1]
+			}
+
+			if delay > 0 {
+				slog.Warn("retrying request after error",
+					"attempt", attempt+1,
+					"url", redactURL(reqURL),
+					"delay", delay.String(),
+					"lastError", sanitizeErrorForLog(lastErr))
+
+				timer := time.NewTimer(delay)
+				select {
+				case <-timer.C:
+				case <-ctx.Done():
+					timer.Stop()
+					return nil, ctx.Err()
+				}
+			}
+		}
+
+		req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, nil)
+		if err != nil {
+			return nil, err
+		}
+		req.Header.Set("Authorization", "token "+c.token)
+
+		resp, err := c.http.Do(req)
+		if err != nil {
+			// Always capture the error for consistent return at loop end.
+			// This ensures both network errors and HTTP 5xx return lastErr.
+			lastErr = err
+
+			// Only retry temporary network errors when attempts remain.
+			if attempt < maxAttempts-1 && isTemporaryNetError(err) {
+				slog.Warn("temporary network error, will retry",
+					"attempt", attempt+1,
+					"url", redactURL(reqURL),
+					"error", err)
+				continue
+			}
+			// Non-retryable network error or final attempt exhausted.
+			return nil, lastErr
+		}
+		if resp.StatusCode >= 200 && resp.StatusCode < 300 {
+			return readBody(resp.Body)
+		}
+
+		// Error path: limit how much we read from potentially malicious server
+		errBody, _ := io.ReadAll(io.LimitReader(resp.Body, maxErrorBodyBytes))
+		resp.Body.Close()
+
+		lastErr = &APIError{StatusCode: resp.StatusCode, Body: string(errBody)}
+
+		// Only retry on 5xx server errors
+		if resp.StatusCode < 500 || resp.StatusCode >= 600 {
+			return nil, lastErr
+		}
+	}
+
+	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) {
-	req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, nil)
-	if err != nil {
-		return nil, err
-	}
-	req.Header.Set("Authorization", "token "+c.token)
+	return c.doGetWithReader(ctx, reqURL, func(body io.ReadCloser) ([]byte, error) {
+		defer body.Close()
+		return io.ReadAll(body)
+	})
+}
 
-	resp, err := c.http.Do(req)
-	if err != nil {
-		return nil, err
-	}
-	defer resp.Body.Close()
-
-	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		body, _ := io.ReadAll(resp.Body)
-		return nil, &APIError{StatusCode: resp.StatusCode, Body: string(body)}
-	}
-	return io.ReadAll(resp.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.
@@ -251,7 +550,13 @@ type ContentEntry struct {
 
 // ListContents lists files and directories at a given path in a repo.
 // Pass an empty path to list the repository root.
+// If the path points to a file (not a directory), Gitea returns a single
+// object instead of an array; this method normalizes both cases to a slice.
 func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) {
+	// Normalize "." to empty string — Gitea API rejects "." with 500
+	if path == "." {
+		path = ""
+	}
 	var reqURL string
 	if path == "" {
 		reqURL = fmt.Sprintf("%s/api/v1/repos/%s/%s/contents", c.baseURL, url.PathEscape(owner), url.PathEscape(repo))
@@ -264,7 +569,16 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]
 	}
 	var entries []ContentEntry
 	if err := json.Unmarshal(body, &entries); err != nil {
-		return nil, fmt.Errorf("parse contents JSON: %w", err)
+		// Gitea returns a single object (not an array) when path is a file
+		var single ContentEntry
+		if err2 := json.Unmarshal(body, &single); err2 != nil {
+			return nil, fmt.Errorf("parse contents JSON: %w", err)
+		}
+		// Guard against empty/malformed responses
+		if single.Name == "" && single.Path == "" {
+			return nil, fmt.Errorf("parse contents JSON: empty response for path %q", path)
+		}
+		entries = []ContentEntry{single}
 	}
 	return entries, nil
 }
@@ -317,9 +631,9 @@ func (c *Client) GetAllFilesInPath(ctx context.Context, owner, repo, path string
 
 // Review represents a pull request review from the Gitea API.
 type Review struct {
-	ID       int64  `json:"id"`
-	Body     string `json:"body"`
-	User     struct {
+	ID   int64  `json:"id"`
+	Body string `json:"body"`
+	User struct {
 		Login string `json:"login"`
 	} `json:"user"`
 	State    string `json:"state"`

gitea/client_test.go

@@ -6,10 +6,15 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"net"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"strings"
+	"sync/atomic"
+	"syscall"
 	"testing"
+	"time"
 )
 
 func TestGetPullRequest(t *testing.T) {
@@ -112,8 +117,9 @@ func TestPostReview(t *testing.T) {
 		}
 
 		var payload struct {
-			Body  string `json:"body"`
-			Event string `json:"event"`
+			Body     string `json:"body"`
+			Event    string `json:"event"`
+			CommitID string `json:"commit_id"`
 		}
 		if err := json.NewDecoder(r.Body).Decode(&payload); err != nil {
 			t.Fatalf("failed to decode payload: %v", err)
@@ -124,14 +130,16 @@ func TestPostReview(t *testing.T) {
 		if payload.Event != "APPROVED" {
 			t.Errorf("expected event %q, got %q", "APPROVED", payload.Event)
 		}
-
+		if payload.CommitID != "abc123def" {
+			t.Errorf("expected commit_id %q, got %q", "abc123def", payload.CommitID)
+		}
 		w.WriteHeader(http.StatusOK)
 		w.Write([]byte(`{"id":100,"user":{"login":"review-bot"},"state":"APPROVED","stale":false}`))
 	}))
 	defer server.Close()
 
 	client := NewClient(server.URL, "test-token")
-	review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", nil)
+	review, err := client.PostReview(context.Background(), "owner", "repo", 3, "APPROVED", "LGTM", "abc123def", nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -178,12 +186,35 @@ func TestPostReview_Non200(t *testing.T) {
 	defer server.Close()
 
 	client := NewClient(server.URL, "test-token")
-	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", nil)
+	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "test", "", nil)
 	if err == nil {
 		t.Fatal("expected error for 403, got nil")
 	}
 }
 
+func TestPostReview_EmptyCommitID_OmittedFromPayload(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		body, _ := io.ReadAll(r.Body)
+		var raw map[string]interface{}
+		if err := json.Unmarshal(body, &raw); err != nil {
+			t.Fatalf("failed to decode payload: %v", err)
+		}
+		if _, exists := raw["commit_id"]; exists {
+			t.Errorf("expected commit_id to be omitted from payload when empty, but it was present")
+		}
+
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte(`{"id":200,"user":{"login":"bot"},"state":"APPROVED","stale":false}`))
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "ok", "", nil)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
 func TestGetFileContent(t *testing.T) {
 	expected := "# Conventions\n- Be nice\n"
 
@@ -276,11 +307,64 @@ func TestListContents(t *testing.T) {
 	}
 }
 
+func TestListContents_DotPath(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		// "." should be normalized to empty path, which hits the root contents endpoint
+		if r.URL.Path != "/api/v1/repos/owner/repo/contents" {
+			t.Errorf("expected root contents path, got: %s", r.URL.Path)
+		}
+		w.Header().Set("Content-Type", "application/json")
+		fmt.Fprintf(w, `[{"name":"README.md","path":"README.md","type":"file"}]`)
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	entries, err := client.ListContents(context.Background(), "owner", "repo", ".")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(entries) != 1 {
+		t.Fatalf("expected 1 entry, got %d", len(entries))
+	}
+	if entries[0].Name != "README.md" {
+		t.Errorf("expected README.md, got %s", entries[0].Name)
+	}
+}
+
+func TestListContents_FilePath(t *testing.T) {
+	// Gitea returns a single object (not an array) when path is a file
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path != "/api/v1/repos/owner/repo/contents/README.md" {
+			t.Errorf("unexpected path: %s", r.URL.Path)
+		}
+		w.Header().Set("Content-Type", "application/json")
+		// Single object, not an array
+		fmt.Fprintf(w, `{"name":"README.md","path":"README.md","type":"file"}`)
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	entries, err := client.ListContents(context.Background(), "owner", "repo", "README.md")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(entries) != 1 {
+		t.Fatalf("expected 1 entry, got %d", len(entries))
+	}
+	if entries[0].Name != "README.md" {
+		t.Errorf("expected README.md, got %s", entries[0].Name)
+	}
+	if entries[0].Type != "file" {
+		t.Errorf("expected type file, got %s", entries[0].Type)
+	}
+}
+
 func TestGetAllFilesInPath_File(t *testing.T) {
 	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		if r.URL.Path == "/api/v1/repos/owner/repo/contents/README.md" {
-			// Gitea returns 404 for contents API on files (it's not a dir)
-			http.NotFound(w, r)
+			// Gitea returns a single object (not array) when path is a file
+			w.Header().Set("Content-Type", "application/json")
+			fmt.Fprintf(w, `{"name":"README.md","path":"README.md","type":"file"}`)
 			return
 		}
 		if r.URL.Path == "/api/v1/repos/owner/repo/raw/README.md" {
@@ -584,9 +668,9 @@ func TestGetAllFilesInPath_403Propagates(t *testing.T) {
 
 func TestIsNotFound(t *testing.T) {
 	tests := []struct {
-		name   string
-		err    error
-		want   bool
+		name string
+		err  error
+		want bool
 	}{
 		{"nil error", nil, false},
 		{"non-API error", fmt.Errorf("network timeout"), false},
@@ -743,3 +827,461 @@ func TestResolveComment_Error(t *testing.T) {
 		t.Fatal("expected error for 404 response")
 	}
 }
+
+func TestIsServerError(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{"nil error", nil, false},
+		{"non-API error", fmt.Errorf("network timeout"), false},
+		{"404 APIError", &APIError{StatusCode: 404, Body: "not found"}, false},
+		{"500 APIError", &APIError{StatusCode: 500, Body: "server error"}, true},
+		{"502 APIError", &APIError{StatusCode: 502, Body: "bad gateway"}, true},
+		{"503 APIError", &APIError{StatusCode: 503, Body: "unavailable"}, true},
+		{"599 APIError", &APIError{StatusCode: 599, Body: "edge case"}, true},
+		{"600 not server error", &APIError{StatusCode: 600, Body: "edge"}, false},
+		{"400 not server error", &APIError{StatusCode: 400, Body: "bad request"}, false},
+		{"wrapped 500", fmt.Errorf("fetch: %w", &APIError{StatusCode: 500, Body: "err"}), true},
+		{"wrapped 404", fmt.Errorf("fetch: %w", &APIError{StatusCode: 404, Body: "err"}), false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := IsServerError(tt.err)
+			if got != tt.want {
+				t.Errorf("IsServerError(%v) = %v, want %v", tt.err, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestDoGet_RetriesOn500(t *testing.T) {
+	attempts := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts++
+		if attempts < 3 {
+			w.WriteHeader(http.StatusInternalServerError)
+			w.Write([]byte(`{"message":"transient error"}`))
+			return
+		}
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte(`{"data":"success"}`))
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	// Use short backoff for fast tests
+	client.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond}
+
+	body, err := client.doGet(context.Background(), server.URL+"/test")
+	if err != nil {
+		t.Fatalf("expected success after retry, got error: %v", err)
+	}
+	if string(body) != `{"data":"success"}` {
+		t.Errorf("body = %q, want %q", string(body), `{"data":"success"}`)
+	}
+	if attempts != 3 {
+		t.Errorf("attempts = %d, want 3", attempts)
+	}
+}
+
+func TestDoGet_FailsAfterMaxRetries(t *testing.T) {
+	attempts := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts++
+		w.WriteHeader(http.StatusInternalServerError)
+		w.Write([]byte(`{"message":"persistent error"}`))
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	// Use short backoff for fast tests
+	client.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond}
+
+	_, err := client.doGet(context.Background(), server.URL+"/test")
+	if err == nil {
+		t.Fatal("expected error after max retries")
+	}
+	var apiErr *APIError
+	if !errors.As(err, &apiErr) {
+		t.Fatalf("expected APIError, got: %v", err)
+	}
+	if apiErr.StatusCode != http.StatusInternalServerError {
+		t.Errorf("status = %d, want 500", apiErr.StatusCode)
+	}
+	if attempts != 3 {
+		t.Errorf("attempts = %d, want 3 (max retries)", attempts)
+	}
+}
+
+func TestDoGet_NoRetryOn4xx(t *testing.T) {
+	attempts := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts++
+		w.WriteHeader(http.StatusForbidden)
+		w.Write([]byte(`{"message":"forbidden"}`))
+	}))
+	defer server.Close()
+
+	client := NewClient(server.URL, "test-token")
+	_, err := client.doGet(context.Background(), server.URL+"/test")
+	if err == nil {
+		t.Fatal("expected error for 403")
+	}
+	var apiErr *APIError
+	if !errors.As(err, &apiErr) {
+		t.Fatalf("expected APIError, got: %v", err)
+	}
+	if apiErr.StatusCode != http.StatusForbidden {
+		t.Errorf("status = %d, want 403", apiErr.StatusCode)
+	}
+	if attempts != 1 {
+		t.Errorf("attempts = %d, want 1 (no retry on 4xx)", attempts)
+	}
+}
+
+func TestDoGet_RespectsContextCancellation(t *testing.T) {
+	attempts := 0
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		attempts++
+		w.WriteHeader(http.StatusInternalServerError)
+		w.Write([]byte(`{"message":"error"}`))
+	}))
+	defer server.Close()
+
+	ctx, cancel := context.WithCancel(context.Background())
+
+	client := NewClient(server.URL, "test-token")
+	// Use longer backoff to give us time to cancel during the wait
+	client.RetryBackoff = []time.Duration{100 * time.Millisecond, 100 * time.Millisecond}
+
+	// Cancel after first attempt returns and retry begins
+	go func() {
+		time.Sleep(20 * time.Millisecond)
+		cancel()
+	}()
+
+	_, err := client.doGet(ctx, server.URL+"/test")
+	if err == nil {
+		t.Fatal("expected error on context cancellation")
+	}
+	// Should have made 1 attempt, then context cancelled during backoff
+	if attempts != 1 {
+		t.Errorf("attempts = %d, expected 1 before context cancel during backoff", attempts)
+	}
+}
+// mockTransport is a test helper that returns errors for the first N calls,
+// then delegates to a real server.
+type mockTransport struct {
+	failCount    int32 // number of failures remaining (atomic)
+	failErr      error // error to return on failure
+	realServer   *httptest.Server
+	attemptsMade atomic.Int32 // tracks total attempts
+}
+
+func (m *mockTransport) RoundTrip(req *http.Request) (*http.Response, error) {
+	m.attemptsMade.Add(1)
+	remaining := atomic.AddInt32(&m.failCount, -1)
+	if remaining >= 0 {
+		// Still have failures to return
+		return nil, m.failErr
+	}
+	// Redirect to real server
+	req.URL.Host = m.realServer.Listener.Addr().String()
+	req.URL.Scheme = "http"
+	return http.DefaultTransport.RoundTrip(req)
+}
+
+func TestDoGet_RetriesOnTemporaryNetError(t *testing.T) {
+	// Real server that will handle successful requests
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte(`{"status":"ok"}`))
+	}))
+	defer server.Close()
+
+	// Mock transport: fail twice with ECONNREFUSED, then succeed
+	mt := &mockTransport{
+		failCount:  2,
+		failErr:    &net.OpError{Op: "dial", Net: "tcp", Err: syscall.ECONNREFUSED},
+		realServer: server,
+	}
+
+	client := NewClient("http://fake-host/", "test-token")
+	client.SetHTTPClient(&http.Client{Transport: mt})
+	client.RetryBackoff = []time.Duration{1 * time.Millisecond, 1 * time.Millisecond}
+
+	body, err := client.doGet(context.Background(), "http://fake-host/test")
+	if err != nil {
+		t.Fatalf("expected success after retries, got error: %v", err)
+	}
+	if string(body) != `{"status":"ok"}` {
+		t.Errorf("body = %q, want %q", string(body), `{"status":"ok"}`)
+	}
+
+	// Should have made exactly 3 attempts: 2 failures + 1 success
+	if got := mt.attemptsMade.Load(); got != 3 {
+		t.Errorf("attempts = %d, want 3 (2 failures + 1 success)", got)
+	}
+}
+
+func TestIsTemporaryNetError(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{"nil error", nil, false},
+		{"plain error", fmt.Errorf("some error"), false},
+		// OpError with retriable syscall errors
+		{"OpError ECONNREFUSED", &net.OpError{Op: "dial", Err: syscall.ECONNREFUSED}, true},
+		{"OpError ECONNRESET", &net.OpError{Op: "read", Err: syscall.ECONNRESET}, true},
+		{"OpError ENETUNREACH", &net.OpError{Op: "dial", Err: syscall.ENETUNREACH}, true},
+		{"OpError EHOSTUNREACH", &net.OpError{Op: "dial", Err: syscall.EHOSTUNREACH}, true},
+		{"OpError ETIMEDOUT", &net.OpError{Op: "dial", Err: syscall.ETIMEDOUT}, true},
+		// OpError with permanent syscall errors — should NOT retry
+		{"OpError EACCES", &net.OpError{Op: "dial", Err: syscall.EACCES}, false},
+		{"OpError EPERM", &net.OpError{Op: "dial", Err: syscall.EPERM}, false},
+		// OpError with unknown inner error — conservative retry
+		{"OpError unknown inner", &net.OpError{Op: "dial", Err: fmt.Errorf("unknown")}, true},
+		// DNS errors
+		{"DNS timeout", &net.DNSError{IsTimeout: true}, true},
+		{"DNS no such host", &net.DNSError{IsTimeout: false, Name: "bad.host"}, false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := isTemporaryNetError(tt.err)
+			if got != tt.want {
+				t.Errorf("isTemporaryNetError(%v) = %v, want %v", tt.err, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestIsRetriableSyscallError(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want bool
+	}{
+		{"nil", nil, false},
+		{"ECONNREFUSED", syscall.ECONNREFUSED, true},
+		{"ECONNRESET", syscall.ECONNRESET, true},
+		{"ENETUNREACH", syscall.ENETUNREACH, true},
+		{"EHOSTUNREACH", syscall.EHOSTUNREACH, true},
+		{"ETIMEDOUT", syscall.ETIMEDOUT, true},
+		{"EACCES (permanent)", syscall.EACCES, false},
+		{"EPERM (permanent)", syscall.EPERM, false},
+		{"ENOENT (permanent)", syscall.ENOENT, false},
+		{"unknown error", fmt.Errorf("something"), true}, // conservative retry
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := isRetriableSyscallError(tt.err)
+			if got != tt.want {
+				t.Errorf("isRetriableSyscallError(%v) = %v, want %v", tt.err, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestRedactURL(t *testing.T) {
+	tests := []struct {
+		name  string
+		input string
+		want  string
+	}{
+		{
+			name:  "no query params",
+			input: "https://gitea.example.com/api/v1/repos/owner/repo/pulls/1",
+			want:  "https://gitea.example.com/api/v1/repos/owner/repo/pulls/1",
+		},
+		{
+			name:  "with query params - redacts",
+			input: "https://gitea.example.com/api/v1/repos/owner/repo/raw/file?ref=main",
+			want:  "https://gitea.example.com/api/v1/repos/owner/repo/raw/file?[redacted]",
+		},
+		{
+			name:  "multiple query params",
+			input: "https://example.com/path?token=secret&page=1",
+			want:  "https://example.com/path?[redacted]",
+		},
+		{
+			name:  "invalid URL",
+			input: "://invalid",
+			want:  "[invalid URL]",
+		},
+		{
+			name:  "empty string",
+			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) {
+			got := redactURL(tt.input)
+			if got != tt.want {
+				t.Errorf("redactURL(%q) = %q, want %q", tt.input, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestSanitizeErrorForLog(t *testing.T) {
+	tests := []struct {
+		name string
+		err  error
+		want string
+	}{
+		{
+			name: "nil error",
+			err:  nil,
+			want: "<nil>",
+		},
+		{
+			name: "APIError omits body",
+			err:  &APIError{StatusCode: 500, Body: "internal error: database connection failed"},
+			want: "HTTP 500",
+		},
+		{
+			name: "APIError with large body still only shows status",
+			err:  &APIError{StatusCode: 502, Body: strings.Repeat("x", 1000)},
+			want: "HTTP 502",
+		},
+		{
+			name: "non-API error preserved",
+			err:  fmt.Errorf("connection refused"),
+			want: "connection refused",
+		},
+		{
+			name: "wrapped APIError",
+			err:  fmt.Errorf("request failed: %w", &APIError{StatusCode: 503, Body: "service unavailable"}),
+			want: "HTTP 503",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := sanitizeErrorForLog(tt.err)
+			if got != tt.want {
+				t.Errorf("sanitizeErrorForLog() = %q, want %q", got, tt.want)
+			}
+		})
+	}
+}
+
+func TestNewClient_HasCheckRedirect(t *testing.T) {
+	c := NewClient("https://gitea.example.com", "token")
+	if c.http.CheckRedirect == nil {
+		t.Fatal("expected CheckRedirect to be set")
+	}
+}
+
+func TestDefaultCheckRedirect_RejectsHTTPSToHTTP(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "http", Host: "gitea.example.com", Path: "/foo"},
+		Header: http.Header{"Authorization": []string{"token abc"}},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err == nil {
+		t.Fatal("expected error on HTTPS->HTTP redirect")
+	}
+	if !strings.Contains(err.Error(), "HTTPS to HTTP downgrade") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_RejectsCrossHost(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "https", Host: "cdn.example.com", Path: "/bar"},
+		Header: http.Header{"Authorization": []string{"token abc"}},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err == nil {
+		t.Fatal("expected error on cross-host redirect")
+	}
+	if !strings.Contains(err.Error(), "cross-host") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_AllowsSameHost(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/bar"},
+		Header: http.Header{"Authorization": []string{"token abc"}},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if auth := req.Header.Get("Authorization"); auth != "token abc" {
+		t.Errorf("expected Authorization to be preserved, got %q", auth)
+	}
+}
+
+func TestDefaultCheckRedirect_AllowsSameHostHTTPToHTTP(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "http", Host: "localhost:3000", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "http", Host: "localhost:3000", Path: "/bar"},
+		Header: http.Header{},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_RejectsTooManyRedirects(t *testing.T) {
+	via := make([]*http.Request, 10)
+	for i := range via {
+		via[i] = &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/"}}
+	}
+	req := &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/final"}}
+	err := defaultCheckRedirect(req, via)
+	if err == nil {
+		t.Fatal("expected error after 10 redirects")
+	}
+	if !strings.Contains(err.Error(), "10 redirects") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_EmptyViaAllowed(t *testing.T) {
+	req := &http.Request{URL: &url.URL{Scheme: "https", Host: "gitea.example.com", Path: "/foo"}}
+	err := defaultCheckRedirect(req, nil)
+	if err != nil {
+		t.Fatalf("unexpected error with empty via: %v", err)
+	}
+}
+
+func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
+	c := NewClient("https://gitea.example.com", "token")
+	c.SetHTTPClient(nil)
+	if c.http == nil {
+		t.Fatal("expected non-nil http client after SetHTTPClient(nil)")
+	}
+	if c.http.Timeout != 30*time.Second {
+		t.Errorf("expected 30s timeout, got %v", c.http.Timeout)
+	}
+	if c.http.CheckRedirect == nil {
+		t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)")
+	}
+}

gitea/diff_size_test.go

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

gitea/post_review_comments_test.go

@@ -37,7 +37,7 @@ func TestPostReview_WithComments(t *testing.T) {
 		{Path: "util.go", NewPosition: 10, Body: "[MINOR] Style issue"},
 	}
 
-	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "REQUEST_CHANGES", "summary", comments)
+	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "REQUEST_CHANGES", "summary", "", comments)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -72,7 +72,7 @@ func TestPostReview_NilComments(t *testing.T) {
 	defer server.Close()
 
 	client := NewClient(server.URL, "test-token")
-	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "all good", nil)
+	_, err := client.PostReview(context.Background(), "owner", "repo", 1, "APPROVED", "all good", "", nil)
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}

github/client.go

@@ -0,0 +1,378 @@
+// 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"
+	"log/slog"
+	"net/http"
+	"net/url"
+	"os"
+	"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
+
+	// allowInsecureHTTP permits requests to HTTP (non-TLS) endpoints.
+	// When false, doRequest rejects URLs with an http:// scheme.
+	allowInsecureHTTP bool
+
+	// retryBackoff defines the delays between retry attempts for 429 responses.
+	// retryBackoff[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
+}
+
+// defaultCheckRedirect is the redirect policy used by NewClient.
+// NOTE: This function is intentionally duplicated in gitea/client.go (and vice versa)
+// because the packages are separate. Changes here must be mirrored there.
+// It rejects HTTPS->HTTP protocol downgrades (to prevent plaintext leakage)
+// and cross-host redirects (to prevent following responses from untrusted
+// endpoints). Same-host, same-or-upgraded-scheme redirects are allowed.
+func defaultCheckRedirect(req *http.Request, via []*http.Request) error {
+	if len(via) >= 10 {
+		return fmt.Errorf("stopped after 10 redirects")
+	}
+	// Guard for direct invocation in tests and any future callers;
+	// net/http guarantees len(via) >= 1 during actual redirects.
+	if len(via) == 0 {
+		return nil
+	}
+	prev := via[len(via)-1]
+	// Reject protocol downgrade: HTTPS->HTTP leaks request metadata over plaintext.
+	if prev.URL.Scheme == "https" && req.URL.Scheme == "http" {
+		return fmt.Errorf("refusing redirect: HTTPS to HTTP downgrade (%s -> %s)", prev.URL.Host, req.URL.Host)
+	}
+	// Reject cross-host redirect entirely to avoid consuming responses
+	// from untrusted endpoints.
+	if req.URL.Host != prev.URL.Host {
+		return fmt.Errorf("refusing redirect: cross-host (%s -> %s)", prev.URL.Host, req.URL.Host)
+	}
+	return nil
+}
+
+// ClientOption configures optional behavior of a Client.
+type ClientOption func(*clientConfig)
+
+type clientConfig struct {
+	allowInsecureHTTP    bool
+	insecureIsTestBypass bool
+}
+
+// AllowInsecureHTTP permits sending credentials over plaintext HTTP connections.
+// In production, this option is gated by the REVIEW_BOT_ALLOW_INSECURE=1
+// environment variable. Without the env var set, the option is ignored
+// and a warning is logged.
+//
+// For tests, use AllowInsecureHTTPForTest (defined in a _test.go file in the same package) which bypasses the env gate.
+func AllowInsecureHTTP() ClientOption {
+	return func(cfg *clientConfig) {
+		cfg.allowInsecureHTTP = true
+	}
+}
+
+// NewClient creates a new GitHub API client.
+// 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, opts ...ClientOption) *Client {
+	if baseURL == "" {
+		baseURL = defaultBaseURL
+	}
+
+	var cfg clientConfig
+	for _, opt := range opts {
+		opt(&cfg)
+	}
+
+	if cfg.allowInsecureHTTP && !cfg.insecureIsTestBypass {
+		if os.Getenv("REVIEW_BOT_ALLOW_INSECURE") != "1" {
+			slog.Warn("AllowInsecureHTTP ignored: set REVIEW_BOT_ALLOW_INSECURE=1 to enable")
+			cfg.allowInsecureHTTP = false
+		} else {
+			slog.Warn("AllowInsecureHTTP enabled — credentials may be sent over plaintext",
+				"env", "REVIEW_BOT_ALLOW_INSECURE=1")
+		}
+	}
+
+	return &Client{
+		baseURL:           strings.TrimRight(baseURL, "/"),
+		token:             token,
+		allowInsecureHTTP: cfg.allowInsecureHTTP,
+		httpClient: &http.Client{
+			Timeout:       30 * time.Second,
+			CheckRedirect: defaultCheckRedirect,
+		},
+		now: time.Now,
+	}
+}
+
+// SetHTTPClient sets the underlying HTTP client used for requests.
+// This is intended for test setup only to inject mock transports; it must be
+// called before any goroutines issue requests.
+//
+// Passing nil restores the default client (30s timeout + redirect-rejecting
+// CheckRedirect policy matching NewClient).
+//
+// Callers providing a non-nil client are responsible for configuring a safe
+// CheckRedirect policy. Without one, the default net/http behavior will follow
+// redirects and may forward the Authorization header to untrusted hosts.
+func (c *Client) SetHTTPClient(hc *http.Client) {
+	if hc == nil {
+		hc = &http.Client{
+			Timeout:       30 * time.Second,
+			CheckRedirect: defaultCheckRedirect,
+		}
+	}
+	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
+}
+
+// redactURL redacts sensitive components from a URL for safe inclusion in error
+// messages and log output. It removes userinfo (e.g., user:pass@) and replaces
+// query parameters with a placeholder.
+func redactURL(rawURL string) string {
+	u, err := url.Parse(rawURL)
+	if err != nil {
+		return "<unparseable URL>"
+	}
+	u.User = nil
+
+	if u.RawQuery != "" {
+		u.RawQuery = "<redacted>"
+	}
+	return u.String()
+}
+
+// doRequest performs an HTTP request with retry on 429 rate limit responses.
+// 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) {
+	// NOTE: This parses reqURL a second time (http.NewRequestWithContext parses it
+	// again internally). Acceptable cost: URL parsing is cheap and threading the
+	// parsed *url.URL through would complicate the interface for negligible gain.
+	if !c.allowInsecureHTTP {
+		parsed, err := url.Parse(reqURL)
+		if err != nil {
+			return nil, fmt.Errorf("parse request URL: %w", err)
+		}
+		if strings.EqualFold(parsed.Scheme, "http") {
+			return nil, fmt.Errorf("refusing HTTP request to %s: use HTTPS or set AllowInsecureHTTP option", redactURL(reqURL))
+		}
+	}
+
+	var backoff []time.Duration
+	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() // no-op after fire; kept for symmetry with the ctx.Done case
+				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

@@ -0,0 +1,658 @@
+package github
+
+import (
+	"context"
+	"errors"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strings"
+	"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, AllowInsecureHTTPForTest())
+	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, AllowInsecureHTTPForTest())
+	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, AllowInsecureHTTPForTest())
+	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, AllowInsecureHTTPForTest())
+	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, AllowInsecureHTTPForTest())
+	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, AllowInsecureHTTPForTest())
+	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, AllowInsecureHTTPForTest())
+	_, 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, AllowInsecureHTTPForTest())
+	_, 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, AllowInsecureHTTPForTest())
+	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
+		}
+	}
+}
+
+func TestNewClient_HasCheckRedirect(t *testing.T) {
+	c := NewClient("secret-token", "https://api.github.com")
+	if c.httpClient.CheckRedirect == nil {
+		t.Fatal("expected CheckRedirect to be set")
+	}
+}
+
+func TestDefaultCheckRedirect_RejectsHTTPSToHTTP(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "http", Host: "api.github.com", Path: "/foo"},
+		Header: http.Header{"Authorization": []string{"Bearer token"}},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err == nil {
+		t.Fatal("expected error on HTTPS->HTTP redirect")
+	}
+	if !strings.Contains(err.Error(), "HTTPS to HTTP downgrade") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_RejectsCrossHost(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "https", Host: "objects.githubusercontent.com", Path: "/bar"},
+		Header: http.Header{"Authorization": []string{"Bearer token"}},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err == nil {
+		t.Fatal("expected error on cross-host redirect")
+	}
+	if !strings.Contains(err.Error(), "cross-host") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_AllowsSameHost(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "https", Host: "api.github.com", Path: "/bar"},
+		Header: http.Header{"Authorization": []string{"Bearer token"}},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	// Auth should be preserved on same-host redirect
+	if auth := req.Header.Get("Authorization"); auth != "Bearer token" {
+		t.Errorf("expected Authorization to be preserved, got %q", auth)
+	}
+}
+
+func TestDefaultCheckRedirect_AllowsSameHostHTTPToHTTP(t *testing.T) {
+	prev := &http.Request{URL: &url.URL{Scheme: "http", Host: "localhost:8080", Path: "/foo"}}
+	req := &http.Request{
+		URL:    &url.URL{Scheme: "http", Host: "localhost:8080", Path: "/bar"},
+		Header: http.Header{},
+	}
+	err := defaultCheckRedirect(req, []*http.Request{prev})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_RejectsTooManyRedirects(t *testing.T) {
+	via := make([]*http.Request, 10)
+	for i := range via {
+		via[i] = &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/"}}
+	}
+	req := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/final"}}
+	err := defaultCheckRedirect(req, via)
+	if err == nil {
+		t.Fatal("expected error after 10 redirects")
+	}
+	if !strings.Contains(err.Error(), "10 redirects") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestDefaultCheckRedirect_EmptyViaAllowed(t *testing.T) {
+	req := &http.Request{URL: &url.URL{Scheme: "https", Host: "api.github.com", Path: "/foo"}}
+	err := defaultCheckRedirect(req, nil)
+	if err != nil {
+		t.Fatalf("unexpected error with empty via: %v", err)
+	}
+}
+
+func TestSetHTTPClient_NilRestoresDefault(t *testing.T) {
+	c := NewClient("token", "https://api.github.com")
+	c.SetHTTPClient(nil)
+	if c.httpClient == nil {
+		t.Fatal("expected non-nil httpClient after SetHTTPClient(nil)")
+	}
+	if c.httpClient.Timeout != 30*time.Second {
+		t.Errorf("expected 30s timeout, got %v", c.httpClient.Timeout)
+	}
+	if c.httpClient.CheckRedirect == nil {
+		t.Fatal("expected CheckRedirect policy after SetHTTPClient(nil)")
+	}
+}
+
+func TestAllowInsecureHTTPForTest_PermitsHTTP(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte("ok"))
+	}))
+	defer srv.Close()
+
+	c := NewClient("tok", srv.URL, AllowInsecureHTTPForTest())
+	body, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if string(body) != "ok" {
+		t.Errorf("body = %q, want %q", body, "ok")
+	}
+}
+
+func TestNoInsecureOption_RejectsHTTP(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		t.Fatal("request should not have been sent")
+	}))
+	defer srv.Close()
+
+	c := NewClient("tok", srv.URL)
+	_, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err == nil {
+		t.Fatal("expected error for HTTP request without AllowInsecureHTTP")
+	}
+	if !strings.Contains(err.Error(), "refusing HTTP request") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestNoInsecureOption_RejectsUppercaseHTTP(t *testing.T) {
+	// Verify case-insensitive scheme check (RFC 3986).
+	c := NewClient("tok", "HTTP://127.0.0.1:1")
+	_, err := c.doGet(context.Background(), "HTTP://127.0.0.1:1/test")
+	if err == nil {
+		t.Fatal("expected error for uppercase HTTP scheme")
+	}
+	if !strings.Contains(err.Error(), "refusing HTTP request") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestNoInsecureOption_RejectsMixedCaseHTTP(t *testing.T) {
+	// Verify mixed case like "Http://" is also rejected.
+	c := NewClient("tok", "Http://127.0.0.1:1")
+	_, err := c.doGet(context.Background(), "Http://127.0.0.1:1/test")
+	if err == nil {
+		t.Fatal("expected error for mixed-case HTTP scheme")
+	}
+	if !strings.Contains(err.Error(), "refusing HTTP request") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestAllowInsecureHTTP_WithoutEnvVar_Rejected(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		t.Fatal("request should not have been sent")
+	}))
+	defer srv.Close()
+
+	t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "")
+
+	c := NewClient("tok", srv.URL, AllowInsecureHTTP())
+	_, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err == nil {
+		t.Fatal("expected error: AllowInsecureHTTP without env var should be rejected")
+	}
+	if !strings.Contains(err.Error(), "refusing HTTP request") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestAllowInsecureHTTP_WithEnvVar_Permitted(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+		w.Write([]byte("insecure-ok"))
+	}))
+	defer srv.Close()
+
+	t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "1")
+
+	c := NewClient("tok", srv.URL, AllowInsecureHTTP())
+	body, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if string(body) != "insecure-ok" {
+		t.Errorf("body = %q, want %q", body, "insecure-ok")
+	}
+}
+
+func TestAllowInsecureHTTP_EnvVarNotOne_Rejected(t *testing.T) {
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		t.Fatal("request should not have been sent")
+	}))
+	defer srv.Close()
+
+	// "true" is not "1" — strict check
+	t.Setenv("REVIEW_BOT_ALLOW_INSECURE", "true")
+
+	c := NewClient("tok", srv.URL, AllowInsecureHTTP())
+	_, err := c.doGet(context.Background(), srv.URL+"/test")
+	if err == nil {
+		t.Fatal("expected error: env var 'true' is not '1'")
+	}
+	if !strings.Contains(err.Error(), "refusing HTTP request") {
+		t.Errorf("unexpected error message: %v", err)
+	}
+}
+
+func TestRedactURL_WithQuery(t *testing.T) {
+	got := redactURL("http://localhost:1234/path?secret=token&foo=bar")
+	want := "http://localhost:1234/path?<redacted>"
+	if got != want {
+		t.Errorf("redactURL = %q, want %q", got, want)
+	}
+}
+
+func TestRedactURL_NoQuery(t *testing.T) {
+	got := redactURL("http://localhost:1234/path")
+	want := "http://localhost:1234/path"
+	if got != want {
+		t.Errorf("redactURL = %q, want %q", got, want)
+	}
+}
+
+func TestRedactURL_Userinfo(t *testing.T) {
+	got := redactURL("http://user:pass@localhost:1234/path")
+	want := "http://localhost:1234/path"
+	if got != want {
+		t.Errorf("redactURL = %q, want %q", got, want)
+	}
+}
+
+func TestRedactURL_UserinfoWithQuery(t *testing.T) {
+	got := redactURL("http://user:pass@localhost:1234/path?secret=token")
+	want := "http://localhost:1234/path?<redacted>"
+	if got != want {
+		t.Errorf("redactURL = %q, want %q", got, want)
+	}
+}

github/export_test.go

@@ -0,0 +1,13 @@
+package github
+
+// AllowInsecureHTTPForTest permits sending credentials over plaintext HTTP
+// without requiring the REVIEW_BOT_ALLOW_INSECURE environment variable.
+// This is intended exclusively for test code using httptest.Server.
+//
+// Defined in a _test.go file so it is only available to test binaries.
+func AllowInsecureHTTPForTest() ClientOption {
+	return func(cfg *clientConfig) {
+		cfg.allowInsecureHTTP = true
+		cfg.insecureIsTestBypass = true
+	}
+}

go.mod

@@ -1,3 +1,5 @@
 module gitea.weiker.me/rodin/review-bot
 
 go 1.26.2
+
+require github.com/goccy/go-yaml v1.19.2

go.sum

@@ -0,0 +1,2 @@
+github.com/goccy/go-yaml v1.19.2 h1:PmFC1S6h8ljIz6gMRBopkjP1TVT7xuwrButHID66PoM=
+github.com/goccy/go-yaml v1.19.2/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7LkFRi1kA=

llm/aicore.go

@@ -0,0 +1,391 @@
+package llm
+
+import (
+	"bytes"
+	"context"
+	"encoding/json"
+	"fmt"
+	"io"
+	"net/http"
+	"net/url"
+	"strings"
+	"sync"
+	"time"
+)
+
+// AICoreOpenAIAPIVersion is the API version used for OpenAI models through AI Core.
+// Update this when SAP AI Core releases a new stable version.
+const AICoreOpenAIAPIVersion = "2024-12-01-preview"
+
+// maxErrorBodyLen limits the length of response bodies included in error messages
+// to prevent leaking potentially sensitive upstream details in logs.
+const maxErrorBodyLen = 200
+
+// AICoreConfig holds SAP AI Core authentication and connection settings.
+type AICoreConfig struct {
+	ClientID      string
+	ClientSecret  string
+	AuthURL       string
+	APIURL        string
+	ResourceGroup string
+}
+
+// AICoreClient wraps AI Core authentication and deployment discovery.
+// Thread-safe for concurrent use after construction.
+//
+// Design: The deployment cache is populated once and never invalidated. This is
+// acceptable for short-lived CI runner processes, but longer-lived deployments
+// may want to add a TTL or re-fetch on errors.
+type AICoreClient struct {
+	config AICoreConfig
+	http   *http.Client
+
+	mu          sync.RWMutex
+	token       string
+	tokenExpiry time.Time
+	deployments map[string]string // model name -> deployment URL
+}
+
+// NewAICoreClient creates a new AI Core client with the given configuration.
+// The client uses a default 5-minute timeout; use WithTimeout to customize.
+func NewAICoreClient(cfg AICoreConfig) *AICoreClient {
+	return &AICoreClient{
+		config:      cfg,
+		http:        &http.Client{Timeout: 5 * time.Minute},
+		deployments: make(map[string]string),
+	}
+}
+
+// WithTimeout sets the HTTP request timeout for AI Core calls.
+// This should be called during construction, before concurrent use.
+func (c *AICoreClient) WithTimeout(d time.Duration) *AICoreClient {
+	c.http.Timeout = d
+	return c
+}
+
+// truncateBody truncates a response body for inclusion in error messages.
+// This prevents leaking potentially sensitive upstream response details in logs.
+func truncateBody(body []byte) string {
+	if len(body) <= maxErrorBodyLen {
+		return string(body)
+	}
+	return string(body[:maxErrorBodyLen]) + "..."
+}
+
+// getToken returns a valid OAuth token, refreshing if necessary.
+func (c *AICoreClient) getToken(ctx context.Context) (string, error) {
+	c.mu.RLock()
+	if c.token != "" && time.Now().Add(5*time.Minute).Before(c.tokenExpiry) {
+		token := c.token
+		c.mu.RUnlock()
+		return token, nil
+	}
+	c.mu.RUnlock()
+
+	c.mu.Lock()
+	defer c.mu.Unlock()
+
+	// Double-check after acquiring write lock
+	if c.token != "" && time.Now().Add(5*time.Minute).Before(c.tokenExpiry) {
+		return c.token, nil
+	}
+
+	token, expiry, err := c.fetchToken(ctx)
+	if err != nil {
+		return "", err
+	}
+	c.token = token
+	c.tokenExpiry = expiry
+	return token, nil
+}
+
+func (c *AICoreClient) fetchToken(ctx context.Context) (string, time.Time, error) {
+	tokenURL := strings.TrimRight(c.config.AuthURL, "/") + "/oauth/token"
+
+	data := url.Values{}
+	data.Set("grant_type", "client_credentials")
+	data.Set("client_id", c.config.ClientID)
+	data.Set("client_secret", c.config.ClientSecret)
+
+	req, err := http.NewRequestWithContext(ctx, http.MethodPost, tokenURL, strings.NewReader(data.Encode()))
+	if err != nil {
+		return "", time.Time{}, fmt.Errorf("create token request: %w", err)
+	}
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+
+	resp, err := c.http.Do(req)
+	if err != nil {
+		return "", time.Time{}, fmt.Errorf("token request: %w", err)
+	}
+	defer resp.Body.Close()
+
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return "", time.Time{}, fmt.Errorf("read token response: %w", err)
+	}
+
+	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
+		return "", time.Time{}, fmt.Errorf("token request failed (status %d): %s", resp.StatusCode, truncateBody(body))
+	}
+
+	var tokenResp struct {
+		AccessToken string `json:"access_token"`
+		ExpiresIn   int    `json:"expires_in"`
+	}
+	if err := json.Unmarshal(body, &tokenResp); err != nil {
+		return "", time.Time{}, fmt.Errorf("parse token response: %w", err)
+	}
+
+	if tokenResp.AccessToken == "" {
+		return "", time.Time{}, fmt.Errorf("empty access token in response")
+	}
+
+	expiry := time.Now().Add(time.Duration(tokenResp.ExpiresIn) * time.Second)
+	return tokenResp.AccessToken, expiry, nil
+}
+
+// getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.
+// getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.
+// Also returns a valid token for use by the caller, avoiding redundant getToken calls.
+//
+// Note: The token is fetched before acquiring the write lock to avoid holding the lock
+// during network I/O. In rare cases where multiple goroutines race and one waits a long
+// time for the write lock, the token could theoretically expire. The 5-minute refresh
+// buffer in getToken makes this extremely unlikely in practice.
+func (c *AICoreClient) getDeploymentURL(ctx context.Context, model string) (deployURL, token string, err error) {
+	c.mu.RLock()
+	if u, ok := c.deployments[model]; ok {
+		c.mu.RUnlock()
+		// Still need a token for the caller
+		token, err = c.getToken(ctx)
+		if err != nil {
+			return "", "", fmt.Errorf("get token: %w", err)
+		}
+		return u, token, nil
+	}
+	c.mu.RUnlock()
+
+	// Fetch token first (before acquiring write lock to avoid holding lock during I/O)
+	token, err = c.getToken(ctx)
+	if err != nil {
+		return "", "", fmt.Errorf("get token for deployments: %w", err)
+	}
+
+	c.mu.Lock()
+	defer c.mu.Unlock()
+
+	// Double-check after acquiring write lock
+	if u, ok := c.deployments[model]; ok {
+		return u, token, nil
+	}
+
+	if err := c.fetchDeployments(ctx, token); err != nil {
+		return "", "", err
+	}
+
+	if u, ok := c.deployments[model]; ok {
+		return u, token, nil
+	}
+	return "", "", fmt.Errorf("no deployment found for model %q", model)
+}
+
+func (c *AICoreClient) fetchDeployments(ctx context.Context, token string) error {
+	deployURL := strings.TrimRight(c.config.APIURL, "/") + "/v2/lm/deployments"
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, deployURL, nil)
+	if err != nil {
+		return fmt.Errorf("create deployments request: %w", err)
+	}
+	req.Header.Set("Authorization", "Bearer "+token)
+	req.Header.Set("AI-Resource-Group", c.config.ResourceGroup)
+
+	resp, err := c.http.Do(req)
+	if err != nil {
+		return fmt.Errorf("deployments request: %w", err)
+	}
+	defer resp.Body.Close()
+
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return fmt.Errorf("read deployments response: %w", err)
+	}
+
+	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
+		return fmt.Errorf("deployments request failed (status %d): %s", resp.StatusCode, truncateBody(body))
+	}
+
+	var deployResp struct {
+		Resources []struct {
+			DeploymentURL string `json:"deploymentUrl"`
+			Status        string `json:"status"`
+			Details       struct {
+				Resources struct {
+					BackendDetails struct {
+						Model struct {
+							Name string `json:"name"`
+						} `json:"model"`
+					} `json:"backend_details"`
+				} `json:"resources"`
+			} `json:"details"`
+		} `json:"resources"`
+	}
+	if err := json.Unmarshal(body, &deployResp); err != nil {
+		return fmt.Errorf("parse deployments response: %w", err)
+	}
+
+	for _, r := range deployResp.Resources {
+		if r.Status != "RUNNING" {
+			continue
+		}
+		modelName := r.Details.Resources.BackendDetails.Model.Name
+		if modelName == "" {
+			continue
+		}
+		c.deployments[modelName] = r.DeploymentURL
+	}
+
+	return nil
+}
+
+// CompleteAnthropic sends a request to an Anthropic model via AI Core.
+func (c *AICoreClient) CompleteAnthropic(ctx context.Context, model string, messages []Message, maxTokens int, temperature float64) (string, error) {
+	deployURL, token, err := c.getDeploymentURL(ctx, model)
+	if err != nil {
+		return "", err
+	}
+
+	// Extract system message
+	var system string
+	var userMessages []anthropicMsg
+	for _, m := range messages {
+		if m.Role == "system" {
+			system = m.Content
+		} else {
+			userMessages = append(userMessages, anthropicMsg{
+				Role:    m.Role,
+				Content: m.Content,
+			})
+		}
+	}
+
+	reqBody := anthropicRequest{
+		AnthropicVersion: "bedrock-2023-05-31", // SAP AI Core uses Bedrock format
+		// Model omitted - AI Core deployment already specifies model
+		MaxTokens: maxTokens,
+		System:    system,
+		Messages:  userMessages,
+	}
+	if temperature > 0 {
+		reqBody.Temperature = temperature
+	}
+
+	data, err := json.Marshal(reqBody)
+	if err != nil {
+		return "", fmt.Errorf("marshal request: %w", err)
+	}
+
+	// AI Core uses /invoke for Anthropic models
+	invokeURL := strings.TrimRight(deployURL, "/") + "/invoke"
+	req, err := http.NewRequestWithContext(ctx, http.MethodPost, invokeURL, bytes.NewReader(data))
+	if err != nil {
+		return "", fmt.Errorf("create request: %w", err)
+	}
+	req.Header.Set("Authorization", "Bearer "+token)
+	req.Header.Set("AI-Resource-Group", c.config.ResourceGroup)
+	req.Header.Set("Content-Type", "application/json")
+
+	resp, err := c.http.Do(req)
+	if err != nil {
+		return "", fmt.Errorf("AI Core request: %w", err)
+	}
+	defer resp.Body.Close()
+
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return "", fmt.Errorf("read response: %w", err)
+	}
+
+	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
+		return "", fmt.Errorf("AI Core API error (status %d): %s", resp.StatusCode, truncateBody(body))
+	}
+
+	var anthropicResp anthropicResponse
+	if err := json.Unmarshal(body, &anthropicResp); err != nil {
+		return "", fmt.Errorf("parse response: %w", err)
+	}
+
+	if len(anthropicResp.Content) == 0 {
+		return "", fmt.Errorf("no content in response")
+	}
+
+	var sb strings.Builder
+	for _, block := range anthropicResp.Content {
+		if block.Type == "text" {
+			sb.WriteString(block.Text)
+		}
+	}
+	result := sb.String()
+	if result == "" {
+		return "", fmt.Errorf("no text content in response")
+	}
+	return result, nil
+}
+
+// CompleteOpenAI sends a request to an OpenAI model via AI Core.
+func (c *AICoreClient) CompleteOpenAI(ctx context.Context, model string, messages []Message, temperature float64) (string, error) {
+	deployURL, token, err := c.getDeploymentURL(ctx, model)
+	if err != nil {
+		return "", err
+	}
+
+	reqBody := ChatRequest{
+		Model:       model,
+		Temperature: temperature,
+		Messages:    messages,
+	}
+
+	data, err := json.Marshal(reqBody)
+	if err != nil {
+		return "", fmt.Errorf("marshal request: %w", err)
+	}
+
+	// AI Core uses /chat/completions?api-version=<version> for OpenAI models
+	chatURL := strings.TrimRight(deployURL, "/") + "/chat/completions?api-version=" + AICoreOpenAIAPIVersion
+	req, err := http.NewRequestWithContext(ctx, http.MethodPost, chatURL, bytes.NewReader(data))
+	if err != nil {
+		return "", fmt.Errorf("create request: %w", err)
+	}
+	req.Header.Set("Authorization", "Bearer "+token)
+	req.Header.Set("AI-Resource-Group", c.config.ResourceGroup)
+	req.Header.Set("Content-Type", "application/json")
+
+	resp, err := c.http.Do(req)
+	if err != nil {
+		return "", fmt.Errorf("AI Core request: %w", err)
+	}
+	defer resp.Body.Close()
+
+	body, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return "", fmt.Errorf("read response: %w", err)
+	}
+
+	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
+		return "", fmt.Errorf("AI Core API error (status %d): %s", resp.StatusCode, truncateBody(body))
+	}
+
+	var openaiResp ChatResponse
+	if err := json.Unmarshal(body, &openaiResp); err != nil {
+		return "", fmt.Errorf("parse response: %w", err)
+	}
+
+	if len(openaiResp.Choices) == 0 {
+		return "", fmt.Errorf("no choices in response")
+	}
+	return openaiResp.Choices[0].Message.Content, nil
+}
+
+// IsAnthropicModel returns true if the model name indicates an Anthropic model.
+// SAP AI Core uses "anthropic--" prefix for Anthropic models (e.g., "anthropic--claude-3-5-sonnet").
+func IsAnthropicModel(model string) bool {
+	return strings.HasPrefix(model, "anthropic--")
+}

llm/aicore_test.go

@@ -0,0 +1,535 @@
+package llm
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"strings"
+	"sync/atomic"
+	"testing"
+	"time"
+)
+
+func TestAICoreClient_TokenFetch(t *testing.T) {
+	tokenCalls := int32(0)
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == "/oauth/token" {
+			atomic.AddInt32(&tokenCalls, 1)
+			if r.Method != http.MethodPost {
+				t.Errorf("expected POST for token, got %s", r.Method)
+			}
+			if r.Header.Get("Content-Type") != "application/x-www-form-urlencoded" {
+				t.Errorf("expected form content type")
+			}
+			w.Header().Set("Content-Type", "application/json")
+			json.NewEncoder(w).Encode(map[string]interface{}{
+				"access_token": "test-token-123",
+				"expires_in":   3600,
+			})
+			return
+		}
+		t.Errorf("unexpected path: %s", r.URL.Path)
+	}))
+	defer server.Close()
+
+	client := NewAICoreClient(AICoreConfig{
+		ClientID:      "test-id",
+		ClientSecret:  "test-secret",
+		AuthURL:       server.URL,
+		APIURL:        server.URL,
+		ResourceGroup: "default",
+	})
+
+	token, err := client.getToken(context.Background())
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if token != "test-token-123" {
+		t.Errorf("expected token 'test-token-123', got %q", token)
+	}
+
+	// Second call should use cached token
+	token2, err := client.getToken(context.Background())
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if token2 != "test-token-123" {
+		t.Errorf("expected cached token")
+	}
+	if atomic.LoadInt32(&tokenCalls) != 1 {
+		t.Errorf("expected 1 token call (cached), got %d", tokenCalls)
+	}
+}
+
+func TestAICoreClient_DeploymentFetch(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == "/oauth/token" {
+			w.Header().Set("Content-Type", "application/json")
+			json.NewEncoder(w).Encode(map[string]interface{}{
+				"access_token": "test-token",
+				"expires_in":   3600,
+			})
+			return
+		}
+		if r.URL.Path == "/v2/lm/deployments" {
+			if r.Header.Get("Authorization") != "Bearer test-token" {
+				t.Errorf("expected Bearer auth")
+			}
+			if r.Header.Get("AI-Resource-Group") != "default" {
+				t.Errorf("expected resource group header")
+			}
+			w.Header().Set("Content-Type", "application/json")
+			json.NewEncoder(w).Encode(map[string]interface{}{
+				"resources": []map[string]interface{}{
+					{
+						"id":            "deploy-123",
+						"deploymentUrl": "https://example.com/v2/inference/deployments/deploy-123",
+						"status":        "RUNNING",
+						"details": map[string]interface{}{
+							"resources": map[string]interface{}{
+								"backend_details": map[string]interface{}{
+									"model": map[string]interface{}{
+										"name": "anthropic--claude-4.6-sonnet",
+									},
+								},
+							},
+						},
+					},
+					{
+						"id":            "deploy-456",
+						"deploymentUrl": "https://example.com/v2/inference/deployments/deploy-456",
+						"status":        "STOPPED",
+						"details": map[string]interface{}{
+							"resources": map[string]interface{}{
+								"backend_details": map[string]interface{}{
+									"model": map[string]interface{}{
+										"name": "gpt-5",
+									},
+								},
+							},
+						},
+					},
+					{
+						"id":            "deploy-789",
+						"deploymentUrl": "https://example.com/v2/inference/deployments/deploy-789",
+						"status":        "RUNNING",
+						"details": map[string]interface{}{
+							"resources": map[string]interface{}{
+								"backend_details": map[string]interface{}{
+									"model": map[string]interface{}{
+										"name": "gpt-5",
+									},
+								},
+							},
+						},
+					},
+				},
+			})
+			return
+		}
+		t.Errorf("unexpected path: %s", r.URL.Path)
+	}))
+	defer server.Close()
+
+	client := NewAICoreClient(AICoreConfig{
+		ClientID:      "test-id",
+		ClientSecret:  "test-secret",
+		AuthURL:       server.URL,
+		APIURL:        server.URL,
+		ResourceGroup: "default",
+	})
+
+	// Should find running deployment
+	url, _, err := client.getDeploymentURL(context.Background(), "anthropic--claude-4.6-sonnet")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if url != "https://example.com/v2/inference/deployments/deploy-123" {
+		t.Errorf("unexpected URL: %s", url)
+	}
+
+	// Should find running gpt-5, not stopped one
+	url, _, err = client.getDeploymentURL(context.Background(), "gpt-5")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if url != "https://example.com/v2/inference/deployments/deploy-789" {
+		t.Errorf("unexpected URL: %s", url)
+	}
+
+	// Should error on unknown model
+	_, _, err = client.getDeploymentURL(context.Background(), "unknown-model")
+	if err == nil {
+		t.Error("expected error for unknown model")
+	}
+}
+
+func TestAICoreClient_CompleteAnthropic(t *testing.T) {
+	// baseURL is set after server creation; captured by closure in handlers
+	var baseURL string
+	mux := http.NewServeMux()
+	mux.HandleFunc("/oauth/token", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(map[string]interface{}{
+			"access_token": "test-token",
+			"expires_in":   3600,
+		})
+	})
+	mux.HandleFunc("/v2/lm/deployments", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(map[string]interface{}{
+			"resources": []map[string]interface{}{
+				{
+					"id":            "deploy-anthropic",
+					"deploymentUrl": baseURL + "/deployments/anthropic",
+					"status":        "RUNNING",
+					"details": map[string]interface{}{
+						"resources": map[string]interface{}{
+							"backend_details": map[string]interface{}{
+								"model": map[string]interface{}{
+									"name": "anthropic--claude-4.6-sonnet",
+								},
+							},
+						},
+					},
+				},
+			},
+		})
+	})
+	mux.HandleFunc("/deployments/anthropic/invoke", func(w http.ResponseWriter, r *http.Request) {
+		if r.Header.Get("Authorization") != "Bearer test-token" {
+			t.Errorf("expected Bearer auth on invoke")
+		}
+		var req anthropicRequest
+		if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+			t.Fatalf("decode request: %v", err)
+		}
+		if req.AnthropicVersion != "bedrock-2023-05-31" {
+			t.Errorf("expected bedrock anthropic_version in request")
+		}
+		if req.System != "You are helpful" {
+			t.Errorf("expected system prompt: %q", req.System)
+		}
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(map[string]interface{}{
+			"content": []map[string]interface{}{
+				{"type": "text", "text": "Hello from AI Core!"},
+			},
+		})
+	})
+
+	server := httptest.NewServer(mux)
+	baseURL = server.URL
+	defer server.Close()
+
+	client := NewAICoreClient(AICoreConfig{
+		ClientID:      "test-id",
+		ClientSecret:  "test-secret",
+		AuthURL:       server.URL,
+		APIURL:        server.URL,
+		ResourceGroup: "default",
+	})
+
+	result, err := client.CompleteAnthropic(context.Background(), "anthropic--claude-4.6-sonnet", []Message{
+		{Role: "system", Content: "You are helpful"},
+		{Role: "user", Content: "Hello"},
+	}, 8192, 0)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if result != "Hello from AI Core!" {
+		t.Errorf("expected 'Hello from AI Core!', got %q", result)
+	}
+}
+
+func TestAICoreClient_CompleteOpenAI(t *testing.T) {
+	var baseURL string
+	mux := http.NewServeMux()
+	mux.HandleFunc("/oauth/token", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(map[string]interface{}{
+			"access_token": "test-token",
+			"expires_in":   3600,
+		})
+	})
+	mux.HandleFunc("/v2/lm/deployments", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(map[string]interface{}{
+			"resources": []map[string]interface{}{
+				{
+					"id":            "deploy-openai",
+					"deploymentUrl": baseURL + "/deployments/openai",
+					"status":        "RUNNING",
+					"details": map[string]interface{}{
+						"resources": map[string]interface{}{
+							"backend_details": map[string]interface{}{
+								"model": map[string]interface{}{
+									"name": "gpt-5",
+								},
+							},
+						},
+					},
+				},
+			},
+		})
+	})
+	mux.HandleFunc("/deployments/openai/chat/completions", func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Query().Get("api-version") != AICoreOpenAIAPIVersion {
+			t.Errorf("expected api-version %s, got %s", AICoreOpenAIAPIVersion, r.URL.Query().Get("api-version"))
+		}
+		var req ChatRequest
+		if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+			t.Fatalf("decode request: %v", err)
+		}
+		if req.Model != "gpt-5" {
+			t.Errorf("expected model gpt-5, got %s", req.Model)
+		}
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(ChatResponse{
+			Choices: []struct {
+				Message struct {
+					Content string `json:"content"`
+				} `json:"message"`
+			}{
+				{Message: struct {
+					Content string `json:"content"`
+				}{Content: "Hello from GPT-5!"}},
+			},
+		})
+	})
+
+	server := httptest.NewServer(mux)
+	baseURL = server.URL
+	defer server.Close()
+
+	client := NewAICoreClient(AICoreConfig{
+		ClientID:      "test-id",
+		ClientSecret:  "test-secret",
+		AuthURL:       server.URL,
+		APIURL:        server.URL,
+		ResourceGroup: "default",
+	})
+
+	result, err := client.CompleteOpenAI(context.Background(), "gpt-5", []Message{
+		{Role: "user", Content: "Hello"},
+	}, 0)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if result != "Hello from GPT-5!" {
+		t.Errorf("expected 'Hello from GPT-5!', got %q", result)
+	}
+}
+
+func TestIsAnthropicModel(t *testing.T) {
+	tests := []struct {
+		model    string
+		expected bool
+	}{
+		// SAP AI Core uses "anthropic--" prefix for Anthropic models
+		{"anthropic--claude-4.6-sonnet", true},
+		{"anthropic--claude-4.6-opus", true},
+		{"anthropic--claude-3-5-sonnet", true},
+		// Non-prefixed model names are not detected as Anthropic
+		// (SAP AI Core always uses the prefix for Anthropic models)
+		{"claude-sonnet-4", false},
+		{"gpt-5", false},
+		{"gpt-4.1", false},
+		{"llama-3", false},
+		{"my-claude-model", false}, // Avoid false positives on "claude" substring
+	}
+
+	for _, tt := range tests {
+		got := IsAnthropicModel(tt.model)
+		if got != tt.expected {
+			t.Errorf("IsAnthropicModel(%q) = %v, want %v", tt.model, got, tt.expected)
+		}
+	}
+}
+
+func TestAICoreClient_TokenExpiry(t *testing.T) {
+	tokenCalls := int32(0)
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path == "/oauth/token" {
+			call := atomic.AddInt32(&tokenCalls, 1)
+			w.Header().Set("Content-Type", "application/json")
+			json.NewEncoder(w).Encode(map[string]interface{}{
+				"access_token": fmt.Sprintf("token-%d", call),
+				"expires_in":   1, // 1 second expiry
+			})
+			return
+		}
+	}))
+	defer server.Close()
+
+	client := NewAICoreClient(AICoreConfig{
+		ClientID:      "test-id",
+		ClientSecret:  "test-secret",
+		AuthURL:       server.URL,
+		APIURL:        server.URL,
+		ResourceGroup: "default",
+	})
+
+	// First call
+	token1, err := client.getToken(context.Background())
+	if err != nil {
+		t.Fatalf("first getToken: %v", err)
+	}
+
+	// Force token expiry by manipulating expiry time
+	client.mu.Lock()
+	client.tokenExpiry = time.Now().Add(-time.Hour)
+	client.mu.Unlock()
+
+	// Should fetch new token
+	token2, err := client.getToken(context.Background())
+	if err != nil {
+		t.Fatalf("second getToken: %v", err)
+	}
+
+	if token1 == token2 {
+		t.Error("expected different tokens after expiry")
+	}
+	if atomic.LoadInt32(&tokenCalls) != 2 {
+		t.Errorf("expected 2 token calls, got %d", tokenCalls)
+	}
+}
+
+func TestAICoreClient_WithTimeout(t *testing.T) {
+	client := NewAICoreClient(AICoreConfig{
+		ClientID:      "test-id",
+		ClientSecret:  "test-secret",
+		AuthURL:       "https://auth.example.com",
+		APIURL:        "https://api.example.com",
+		ResourceGroup: "default",
+	})
+
+	// Default timeout is 5 minutes
+	if client.http.Timeout != 5*time.Minute {
+		t.Errorf("expected default timeout 5m, got %v", client.http.Timeout)
+	}
+
+	// WithTimeout should update the timeout
+	client.WithTimeout(10 * time.Minute)
+	if client.http.Timeout != 10*time.Minute {
+		t.Errorf("expected timeout 10m, got %v", client.http.Timeout)
+	}
+}
+
+func TestClient_WithAICore(t *testing.T) {
+	client := NewClient("http://example.com", "key", "model")
+	if client.provider != ProviderOpenAI {
+		t.Errorf("expected default provider openai, got %s", client.provider)
+	}
+
+	client.WithAICore(AICoreConfig{
+		ClientID:      "id",
+		ClientSecret:  "secret",
+		AuthURL:       "https://auth.example.com",
+		APIURL:        "https://api.example.com",
+		ResourceGroup: "default",
+	})
+
+	if client.provider != ProviderAICore {
+		t.Errorf("expected provider aicore, got %s", client.provider)
+	}
+	if client.aicore == nil {
+		t.Error("expected aicore client to be set")
+	}
+}
+
+func TestClient_WithTimeout_PropagatestoAICore(t *testing.T) {
+	client := NewClient("http://example.com", "key", "model").
+		WithAICore(AICoreConfig{
+			ClientID:      "id",
+			ClientSecret:  "secret",
+			AuthURL:       "https://auth.example.com",
+			APIURL:        "https://api.example.com",
+			ResourceGroup: "default",
+		})
+
+	// Default should be 5 minutes (inherited from parent client)
+	if client.aicore.http.Timeout != 5*time.Minute {
+		t.Errorf("expected aicore default timeout 5m, got %v", client.aicore.http.Timeout)
+	}
+
+	// WithTimeout should propagate to AI Core client
+	client.WithTimeout(15 * time.Minute)
+	if client.http.Timeout != 15*time.Minute {
+		t.Errorf("expected parent timeout 15m, got %v", client.http.Timeout)
+	}
+	if client.aicore.http.Timeout != 15*time.Minute {
+		t.Errorf("expected aicore timeout 15m, got %v", client.aicore.http.Timeout)
+	}
+}
+
+func TestClient_CompleteAICore(t *testing.T) {
+	var baseURL string
+	mux := http.NewServeMux()
+	mux.HandleFunc("/oauth/token", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(map[string]interface{}{
+			"access_token": "test-token",
+			"expires_in":   3600,
+		})
+	})
+	mux.HandleFunc("/v2/lm/deployments", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(map[string]interface{}{
+			"resources": []map[string]interface{}{
+				{
+					"id":            "deploy-test",
+					"deploymentUrl": baseURL + "/deployments/test",
+					"status":        "RUNNING",
+					"details": map[string]interface{}{
+						"resources": map[string]interface{}{
+							"backend_details": map[string]interface{}{
+								"model": map[string]interface{}{
+									"name": "gpt-5",
+								},
+							},
+						},
+					},
+				},
+			},
+		})
+	})
+	mux.HandleFunc("/deployments/test/chat/completions", func(w http.ResponseWriter, r *http.Request) {
+		w.Header().Set("Content-Type", "application/json")
+		json.NewEncoder(w).Encode(ChatResponse{
+			Choices: []struct {
+				Message struct {
+					Content string `json:"content"`
+				} `json:"message"`
+			}{
+				{Message: struct {
+					Content string `json:"content"`
+				}{Content: "AI Core via Client works!"}},
+			},
+		})
+	})
+
+	server := httptest.NewServer(mux)
+	baseURL = server.URL
+	defer server.Close()
+
+	client := NewClient("", "", "gpt-5").WithAICore(AICoreConfig{
+		ClientID:      "test-id",
+		ClientSecret:  "test-secret",
+		AuthURL:       server.URL,
+		APIURL:        server.URL,
+		ResourceGroup: "default",
+	})
+
+	result, err := client.Complete(context.Background(), []Message{
+		{Role: "user", Content: "Hello"},
+	})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if !strings.Contains(result, "AI Core via Client works!") {
+		t.Errorf("unexpected result: %s", result)
+	}
+}

llm/client.go

@@ -1,6 +1,6 @@
 // Package llm provides clients for LLM chat completion APIs.
 //
-// Supports OpenAI-compatible (default) and Anthropic Messages API providers.
+// Supports OpenAI-compatible (default), Anthropic Messages API, and SAP AI Core providers.
 package llm
 
 import (
@@ -22,6 +22,8 @@ const (
 	ProviderOpenAI Provider = "openai"
 	// ProviderAnthropic uses the Anthropic Messages API endpoint.
 	ProviderAnthropic Provider = "anthropic"
+	// ProviderAICore uses SAP AI Core with OAuth authentication.
+	ProviderAICore Provider = "aicore"
 )
 
 // Client calls an LLM chat completion API.
@@ -35,6 +37,7 @@ type Client struct {
 	temperature float64
 	provider    Provider
 	http        *http.Client
+	aicore      *AICoreClient // Only set when provider is aicore
 }
 
 // NewClient creates a new LLM client. Default provider is OpenAI-compatible.
@@ -49,8 +52,12 @@ func NewClient(baseURL, apiKey, model string) *Client {
 }
 
 // WithTimeout sets the HTTP request timeout for LLM calls (default 5 minutes).
+// When using AI Core, this also sets the timeout on the AI Core client.
 func (c *Client) WithTimeout(d time.Duration) *Client {
 	c.http.Timeout = d
+	if c.aicore != nil {
+		c.aicore.WithTimeout(d)
+	}
 	return c
 }
 
@@ -60,12 +67,21 @@ func (c *Client) WithTemperature(t float64) *Client {
 	return c
 }
 
-// WithProvider sets the API provider format (openai or anthropic).
+// WithProvider sets the API provider format (openai, anthropic, or aicore).
 func (c *Client) WithProvider(p Provider) *Client {
 	c.provider = p
 	return c
 }
 
+// WithAICore configures the client to use SAP AI Core for authentication.
+// This sets the provider to aicore automatically.
+// The AI Core client inherits the current HTTP timeout from this client.
+func (c *Client) WithAICore(cfg AICoreConfig) *Client {
+	c.provider = ProviderAICore
+	c.aicore = NewAICoreClient(cfg).WithTimeout(c.http.Timeout)
+	return c
+}
+
 // Message represents a chat message.
 type Message struct {
 	Role    string `json:"role"`
@@ -82,6 +98,8 @@ func (c *Client) Complete(ctx context.Context, messages []Message) (string, erro
 		switch c.provider {
 		case ProviderAnthropic:
 			result, err = c.completeAnthropic(ctx, messages)
+		case ProviderAICore:
+			result, err = c.completeAICore(ctx, messages)
 		default:
 			result, err = c.completeOpenAI(ctx, messages)
 		}
@@ -106,6 +124,18 @@ func (c *Client) Complete(ctx context.Context, messages []Message) (string, erro
 	return "", err
 }
 
+// completeAICore routes to AI Core using the appropriate endpoint based on model type.
+func (c *Client) completeAICore(ctx context.Context, messages []Message) (string, error) {
+	if c.aicore == nil {
+		return "", fmt.Errorf("AI Core client not configured")
+	}
+
+	if IsAnthropicModel(c.model) {
+		return c.aicore.CompleteAnthropic(ctx, c.model, messages, 8192, c.temperature)
+	}
+	return c.aicore.CompleteOpenAI(ctx, c.model, messages, c.temperature)
+}
+
 // isRetryableError returns true for transient errors worth retrying.
 func isRetryableError(err error) bool {
 	if err == nil {
@@ -176,11 +206,12 @@ func (c *Client) completeOpenAI(ctx context.Context, messages []Message) (string
 // --- Anthropic Messages API implementation ---
 
 type anthropicRequest struct {
-	Model       string            `json:"model"`
-	MaxTokens   int               `json:"max_tokens"`
-	System      string            `json:"system,omitempty"`
-	Messages    []anthropicMsg    `json:"messages"`
-	Temperature float64           `json:"temperature,omitempty"`
+	AnthropicVersion string         `json:"anthropic_version,omitempty"`
+	Model       string         `json:"model,omitempty"`
+	MaxTokens   int            `json:"max_tokens"`
+	System      string         `json:"system,omitempty"`
+	Messages    []anthropicMsg `json:"messages"`
+	Temperature float64        `json:"temperature,omitempty"`
 }
 
 type anthropicMsg struct {

review/formatter.go

@@ -7,10 +7,37 @@ import (
 
 // FormatMarkdown formats a ReviewResult into the markdown body for a Gitea review.
 func FormatMarkdown(result *ReviewResult, reviewerName string) string {
+	return FormatMarkdownWithDisplay(result, reviewerName, reviewerName)
+}
+
+// GiteaEvent converts the verdict to the Gitea API event string.
+func GiteaEvent(verdict string) string {
+	switch verdict {
+	case "APPROVE":
+		return "APPROVED"
+	case "REQUEST_CHANGES":
+		return "REQUEST_CHANGES"
+	default:
+		return "COMMENT"
+	}
+}
+
+// FormatMarkdownWithDisplay formats a ReviewResult with separate display name and sentinel name.
+// Note: displayName is not HTML-escaped as Gitea sanitizes rendered Markdown.
+// Persona display names are controlled by repo owners (trusted input).
+// displayName is used for the header title, sentinelName is used for the cleanup sentinel.
+// If displayName is empty, sentinelName is used for both.
+func FormatMarkdownWithDisplay(result *ReviewResult, displayName, sentinelName string) string {
 	var sb strings.Builder
 
-	if reviewerName != "" {
-		title := strings.ToUpper(reviewerName[:1]) + reviewerName[1:]
+	// Use display name for header, or fall back to sentinel name
+	headerName := displayName
+	if headerName == "" {
+		headerName = sentinelName
+	}
+
+	if headerName != "" {
+		title := CapitalizeFirst(headerName)
 		sb.WriteString(fmt.Sprintf("# %s Review\n\n", title))
 	}
 
@@ -33,23 +60,11 @@ func FormatMarkdown(result *ReviewResult, reviewerName string) string {
 	sb.WriteString("## Recommendation\n\n")
 	sb.WriteString(fmt.Sprintf("**%s** — %s\n", result.Verdict, result.Recommendation))
 
-	if reviewerName != "" {
-		sb.WriteString(fmt.Sprintf("\n---\n*Review by %s*\n", reviewerName))
+	if sentinelName != "" {
+		sb.WriteString(fmt.Sprintf("\n---\n*Review by %s*\n", headerName))
 		// Hidden sentinel for identifying this bot's reviews during cleanup
-		sb.WriteString(fmt.Sprintf("\n<!-- review-bot:%s -->\n", reviewerName))
+		sb.WriteString(fmt.Sprintf("\n<!-- review-bot:%s -->\n", sentinelName))
 	}
 
 	return sb.String()
 }
-
-// GiteaEvent converts the verdict to the Gitea API event string.
-func GiteaEvent(verdict string) string {
-	switch verdict {
-	case "APPROVE":
-		return "APPROVED"
-	case "REQUEST_CHANGES":
-		return "REQUEST_CHANGES"
-	default:
-		return "COMMENT"
-	}
-}

review/formatter_test.go

@@ -159,3 +159,58 @@ func TestFormatMarkdown_RoleTitle(t *testing.T) {
 		t.Error("should not contain role title header when reviewer name is empty")
 	}
 }
+
+func TestFormatMarkdownWithDisplay(t *testing.T) {
+	result := &ReviewResult{
+		Verdict:        "APPROVE",
+		Summary:        "Test summary",
+		Findings:       nil,
+		Recommendation: "Test recommendation",
+	}
+
+	t.Run("with display name", func(t *testing.T) {
+		body := FormatMarkdownWithDisplay(result, "Security Specialist", "security")
+
+		// Header should use display name
+		if !strings.Contains(body, "# Security Specialist Review") {
+			t.Error("header should use display name")
+		}
+
+		// Sentinel should use sentinel name
+		if !strings.Contains(body, "<!-- review-bot:security -->") {
+			t.Error("sentinel should use sentinel name")
+		}
+
+		// Footer "Review by" should use display name
+		if !strings.Contains(body, "*Review by Security Specialist*") {
+			t.Error("footer should use display name")
+		}
+	})
+
+	t.Run("without display name", func(t *testing.T) {
+		body := FormatMarkdownWithDisplay(result, "", "reviewer")
+
+		// Should fall back to sentinel name for header
+		if !strings.Contains(body, "# Reviewer Review") {
+			t.Error("header should fall back to sentinel name")
+		}
+
+		if !strings.Contains(body, "<!-- review-bot:reviewer -->") {
+			t.Error("sentinel should use sentinel name")
+		}
+	})
+
+	t.Run("empty both names", func(t *testing.T) {
+		body := FormatMarkdownWithDisplay(result, "", "")
+
+		// Should not have header
+		if strings.Contains(body, "# ") && strings.Contains(body, " Review") {
+			t.Error("should not have header when both names empty")
+		}
+
+		// Should not have sentinel
+		if strings.Contains(body, "<!-- review-bot:") {
+			t.Error("should not have sentinel when sentinel name empty")
+		}
+	})
+}

review/persona.go

@@ -0,0 +1,367 @@
+package review
+
+import (
+	"bytes"
+	"embed"
+	"encoding/json"
+	"fmt"
+	"io"
+	"os"
+	"sort"
+	"strings"
+	"unicode/utf8"
+
+	"github.com/goccy/go-yaml"
+	"github.com/goccy/go-yaml/ast"
+	"github.com/goccy/go-yaml/parser"
+)
+
+//go:embed personas/*.yaml
+var embeddedPersonas embed.FS
+
+// MaxPersonaFileSize is the maximum size for persona files (64 KB).
+// This prevents denial-of-service via excessively large files.
+const MaxPersonaFileSize = 64 * 1024
+
+// MaxYAMLDepth is the maximum nesting depth allowed in YAML persona files.
+// This prevents stack exhaustion from deeply nested structures.
+const MaxYAMLDepth = 20
+
+// MaxYAMLNodes is the maximum number of YAML nodes allowed in persona files.
+// This prevents DoS via wide-but-shallow structures that bypass depth limits.
+const MaxYAMLNodes = 1000
+
+// Persona defines a specialized review role with focused expertise.
+type Persona struct {
+	Name         string   `json:"name" yaml:"name"`
+	DisplayName  string   `json:"display_name" yaml:"display_name"`
+	ModelPref    string   `json:"model_preference,omitempty" yaml:"model_preference,omitempty"`
+	Identity     string   `json:"identity" yaml:"identity"`
+	Focus        []string `json:"focus" yaml:"focus"`
+	Ignore       []string `json:"ignore" yaml:"ignore"`
+	Severity     Severity `json:"severity" yaml:"severity"`
+	OutputFormat string   `json:"output_format,omitempty" yaml:"output_format,omitempty"`
+}
+
+// Severity defines what constitutes each severity level for this persona.
+// These are prompt guidance for the LLM, not output format changes.
+type Severity struct {
+	Major string `json:"major" yaml:"major"`
+	Minor string `json:"minor" yaml:"minor"`
+	Nit   string `json:"nit" yaml:"nit"`
+}
+
+// LoadPersona loads a persona from a JSON or YAML file path.
+// Format is detected by file extension: .yaml/.yml for YAML, .json or other for JSON.
+// Files larger than MaxPersonaFileSize are rejected.
+//
+// Symlinks are supported: os.Stat follows symlinks, so a symlink pointing to
+// a regular file will pass the IsRegular() check. Symlinks to non-regular files
+// (directories, FIFOs, devices) are still rejected.
+func LoadPersona(path string) (*Persona, error) {
+	// os.Stat follows symlinks, so symlinks to regular files are supported.
+	// The IsRegular() check operates on the target, not the symlink itself.
+	info, err := os.Stat(path)
+	if err != nil {
+		return nil, fmt.Errorf("read persona file %s: %w", path, err)
+	}
+	if !info.Mode().IsRegular() {
+		return nil, fmt.Errorf("persona file %s is not a regular file", path)
+	}
+	if info.Size() > MaxPersonaFileSize {
+		return nil, fmt.Errorf("persona file %s exceeds maximum size (%d bytes)", path, MaxPersonaFileSize)
+	}
+	data, err := os.ReadFile(path)
+	if err != nil {
+		return nil, fmt.Errorf("read persona file %s: %w", path, err)
+	}
+	// Re-check size after read to defend against TOCTOU races where file
+	// grows between stat and read (e.g., appending process, replaced file).
+	if len(data) > MaxPersonaFileSize {
+		return nil, fmt.Errorf("persona file %s exceeds maximum size (%d bytes)", path, MaxPersonaFileSize)
+	}
+	return parsePersona(data, path)
+}
+
+// LoadBuiltinPersona loads a built-in persona by name.
+// Returns an error if the persona doesn't exist.
+// Built-in personas are stored in YAML format only (see embed directive).
+func LoadBuiltinPersona(name string) (*Persona, error) {
+	yamlFile := name + ".yaml"
+	data, err := embeddedPersonas.ReadFile("personas/" + yamlFile)
+	if err != nil {
+		available := ListBuiltinPersonas()
+		return nil, fmt.Errorf("unknown built-in persona %q (available: %s)", name, strings.Join(available, ", "))
+	}
+	return parsePersona(data, "builtin:"+yamlFile)
+}
+
+// ListBuiltinPersonas returns the names of all built-in personas in sorted order.
+// Returns an empty slice if the embedded directory cannot be read.
+func ListBuiltinPersonas() []string {
+	entries, err := embeddedPersonas.ReadDir("personas")
+	if err != nil {
+		return []string{}
+	}
+	seen := make(map[string]bool)
+	for _, e := range entries {
+		if e.IsDir() {
+			continue
+		}
+		name := e.Name()
+		// Strip extension to get persona name
+		var personaName string
+		switch {
+		case strings.HasSuffix(name, ".yaml"):
+			personaName = strings.TrimSuffix(name, ".yaml")
+		case strings.HasSuffix(name, ".yml"):
+			personaName = strings.TrimSuffix(name, ".yml")
+		case strings.HasSuffix(name, ".json"):
+			personaName = strings.TrimSuffix(name, ".json")
+		default:
+			continue
+		}
+		seen[personaName] = true
+	}
+	names := make([]string, 0, len(seen))
+	for name := range seen {
+		names = append(names, name)
+	}
+	sort.Strings(names)
+	return names
+}
+
+// parsePersona parses persona data from JSON or YAML format.
+// Format is detected by the source file extension.
+func parsePersona(data []byte, source string) (*Persona, error) {
+	lowerSource := strings.ToLower(source)
+	isYAML := strings.HasSuffix(lowerSource, ".yaml") || strings.HasSuffix(lowerSource, ".yml")
+
+	var p Persona
+	var err error
+	if isYAML {
+		err = unmarshalYAMLWithDepthLimit(data, &p, MaxYAMLDepth)
+	} else {
+		// Use json.Decoder with DisallowUnknownFields for consistency with
+		// YAML's Strict() - 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)
+	}
+	if err := validatePersona(&p, source); err != nil {
+		return nil, err
+	}
+	return &p, nil
+}
+
+// unmarshalYAMLWithDepthLimit unmarshals YAML data with three safety checks:
+//   - Depth limiting: rejects AST trees exceeding maxDepth to prevent stack exhaustion.
+//   - Multi-document rejection: prevents silent data loss from ignored extra documents.
+//   - Strict field checking: rejects unknown YAML keys to catch typos early.
+func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
+	// First pass: parse into AST to check depth limits, node counts, and
+	// multi-document rejection. This prevents stack exhaustion before we
+	// attempt to decode into structs.
+	file, err := parser.ParseBytes(data, 0)
+	if err != nil {
+		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 {
+		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 {
+		return err
+	}
+
+	// Second pass: decode with strict field checking enabled.
+	// Strict() rejects unknown keys, catching typos like "focuss" or "identiy".
+	//
+	// Safety note: goccy/go-yaml's decoder does not expand YAML aliases
+	// recursively — it resolves them via the pre-built AST, which our first
+	// pass already depth-checked. Alias chains that would exceed depth limits
+	// are caught above; the decoder merely reads the resolved scalar values.
+	dec := yaml.NewDecoder(bytes.NewReader(data), yaml.Strict())
+	return dec.Decode(out)
+}
+
+// checkYAMLDepth recursively checks that YAML AST nodes don't exceed the depth
+// limit or the total node count limit. It uses two tracking maps:
+//   - validated: maps each node to the maximum depth at which it was previously
+//     checked. If a node is revisited at a deeper depth (e.g., via an alias),
+//     we re-check it to ensure the combined effective depth doesn't exceed limits.
+//   - visiting: per-path recursion stack for true cycle detection. A node on the
+//     current path is a cycle (alias loop); we return nil to avoid infinite recursion.
+//
+// This design prevents the alias depth bypass where an anchored subtree validated
+// at a shallow depth could be referenced via alias at a greater depth, effectively
+// exceeding MaxYAMLDepth.
+func checkYAMLDepth(node ast.Node, depth, maxDepth, maxNodes int, validated map[ast.Node]int, visiting map[ast.Node]bool, nodeCount *int) error {
+	if node == nil {
+		return nil
+	}
+
+	if depth > maxDepth {
+		return fmt.Errorf("YAML nesting depth exceeds maximum (%d)", maxDepth)
+	}
+
+	// Cycle detection: if we're currently visiting this node on the current
+	// recursion path, it's a cycle (e.g., alias pointing to an ancestor).
+	// Return nil to break the cycle without error — cycles are a structural
+	// property, not a depth violation.
+	if visiting[node] {
+		return nil
+	}
+
+	// Track total nodes visited as defense-in-depth against wide-but-shallow attacks.
+	// Placed after cycle detection but before the depth-aware short-circuit. This means
+	// nodes revisited at shallower depths (via aliases) are counted each time they are
+	// encountered — intentional conservative overcounting. This bounds the total work
+	// performed during validation rather than tracking unique nodes, which is the safer
+	// security posture for untrusted YAML input.
+	*nodeCount++
+	if *nodeCount > maxNodes {
+		return fmt.Errorf("YAML node count exceeds maximum (%d)", maxNodes)
+	}
+
+	// Depth-aware short-circuit: skip re-validation only when the current visit
+	// depth is the same or shallower than the depth at which this node was
+	// previously validated. A shallower (or equal) current depth means the
+	// prior, deeper validation already covered any subtree depth violations.
+	// If the current depth exceeds the previous validation depth (e.g., an alias
+	// references this node deeper in the tree), we must re-traverse to ensure
+	// the combined effective depth doesn't exceed maxDepth.
+	//
+	// Note: using ast.Node (interface) as map key relies on pointer identity,
+	// which is correct because all goccy/go-yaml AST node types are pointer
+	// receivers (*MappingNode, *SequenceNode, etc.), never value types.
+	if prevDepth, ok := validated[node]; ok && depth <= prevDepth {
+		return nil
+	}
+	validated[node] = depth
+
+	// Mark as visiting (on the current recursion path) for cycle detection.
+	visiting[node] = true
+	defer func() { visiting[node] = false }()
+
+	// Walk children based on node type.
+	switch n := node.(type) {
+	case *ast.MappingNode:
+		for _, value := range n.Values {
+			if err := checkYAMLDepth(value, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
+				return err
+			}
+		}
+	case *ast.MappingValueNode:
+		// Both Key and Value are visited at depth+1 relative to this
+		// MappingValueNode. Since MappingNode visits its MappingValueNode
+		// children at depth+1 as well, keys and values end up at depth+2
+		// from the parent MappingNode. This is intentional: it mirrors the
+		// actual nesting structure (mapping → key-value pair → key/value).
+		if err := checkYAMLDepth(n.Key, depth+1, maxDepth, maxNodes, validated, visiting, nodeCount); err != nil {
+			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)
+}
+
+func validatePersona(p *Persona, source string) error {
+	if p.Name == "" {
+		return fmt.Errorf("persona %s: name is required", source)
+	}
+	if p.Identity == "" {
+		return fmt.Errorf("persona %s: identity is required", source)
+	}
+	// DisplayName defaults to Name if not set
+	if p.DisplayName == "" {
+		p.DisplayName = p.Name
+	}
+	return nil
+}
+
+// CapitalizeFirst capitalizes the first rune of a string in a Unicode-safe way.
+// Returns the original string if it's empty.
+func CapitalizeFirst(s string) string {
+	if s == "" {
+		return s
+	}
+	r, size := utf8.DecodeRuneInString(s)
+	if r == utf8.RuneError {
+		return s
+	}
+	return strings.ToUpper(string(r)) + s[size:]
+}

review/persona_prompt.go

@@ -0,0 +1,104 @@
+package review
+
+import (
+	"fmt"
+	"strings"
+)
+
+// BuildPersonaSystemPrompt constructs a system prompt from a persona definition.
+// This replaces BuildSystemBase when a persona is provided.
+func BuildPersonaSystemPrompt(p *Persona) string {
+	var sb strings.Builder
+
+	// Identity section
+	sb.WriteString(p.Identity)
+	sb.WriteString("\n\n")
+
+	// Focus section
+	if len(p.Focus) > 0 {
+		sb.WriteString("## Focus Areas\n\n")
+		sb.WriteString("Concentrate your review on:\n")
+		for _, f := range p.Focus {
+			sb.WriteString(fmt.Sprintf("- %s\n", f))
+		}
+		sb.WriteString("\n")
+	}
+
+	// Ignore section
+	if len(p.Ignore) > 0 {
+		sb.WriteString("## Explicitly Out of Scope\n\n")
+		sb.WriteString("Do NOT comment on:\n")
+		for _, i := range p.Ignore {
+			sb.WriteString(fmt.Sprintf("- %s\n", i))
+		}
+		sb.WriteString("\n")
+	}
+
+	// Severity calibration
+	if p.Severity.Major != "" || p.Severity.Minor != "" || p.Severity.Nit != "" {
+		sb.WriteString("## Severity Calibration\n\n")
+		sb.WriteString("Use these severity definitions for YOUR domain:\n")
+		if p.Severity.Major != "" {
+			sb.WriteString(fmt.Sprintf("- **MAJOR**: %s\n", p.Severity.Major))
+		}
+		if p.Severity.Minor != "" {
+			sb.WriteString(fmt.Sprintf("- **MINOR**: %s\n", p.Severity.Minor))
+		}
+		if p.Severity.Nit != "" {
+			sb.WriteString(fmt.Sprintf("- **NIT**: %s\n", p.Severity.Nit))
+		}
+		sb.WriteString("\n")
+	}
+
+	// Output format instructions (shared schema from prompt.go)
+	sb.WriteString("## Review Instructions\n\n")
+	sb.WriteString("CONTEXT:\n")
+	sb.WriteString("- You will receive the full content of modified files for reference, followed by the diff showing what changed.\n")
+	sb.WriteString("- The diff shows ONLY what was added/removed. The full file content provides complete context.\n")
+	sb.WriteString("- Focus your review on the CHANGES (the diff), using the full files for context.\n\n")
+	sb.WriteString("Your task:\n")
+	sb.WriteString("1. Review the diff for issues within YOUR focus areas only.\n")
+	sb.WriteString("2. Consider the CI status — if CI has failed, that is an automatic REQUEST_CHANGES regardless of code quality.\n")
+	sb.WriteString("3. Output your review as structured JSON (and ONLY JSON, no markdown fences or other text).\n\n")
+	sb.WriteString("Output format:\n")
+	sb.WriteString(outputSchemaJSON)
+	sb.WriteString("\n\n")
+	sb.WriteString(verdictRules)
+	sb.WriteString("\n- Only report findings within your focus areas. Ignore everything else.\n")
+	sb.WriteString("- Line numbers should reference the new file line numbers from the diff headers.\n")
+	sb.WriteString("- If the diff has no changes relevant to your focus areas, APPROVE with no findings.\n")
+
+	// Custom output format if provided
+	if p.OutputFormat != "" {
+		sb.WriteString("\n\n## Additional Output Guidelines\n\n")
+		sb.WriteString(p.OutputFormat)
+	}
+
+	return sb.String()
+}
+
+// BuildSystemPromptWithPersona constructs the full system prompt, using either
+// a persona or the default generic prompt. This is a convenience wrapper that
+// combines BuildPersonaSystemPrompt (or BuildSystemBase) with patterns and conventions.
+// It is exported for use by callers who want one-shot prompt assembly.
+func BuildSystemPromptWithPersona(persona *Persona, conventions, patterns string) string {
+	var base string
+	if persona != nil {
+		base = BuildPersonaSystemPrompt(persona)
+	} else {
+		base = BuildSystemBase()
+	}
+
+	var sb strings.Builder
+	sb.WriteString(base)
+
+	if patterns != "" {
+		sb.WriteString(fmt.Sprintf("\n\n## Language Patterns & Idioms\n\nUse the following patterns as review criteria. Code that violates these established patterns is a finding:\n\n%s\n", patterns))
+	}
+
+	if conventions != "" {
+		sb.WriteString(fmt.Sprintf("\n\n## Repository Conventions\n\nThe repository has the following coding conventions that must be respected:\n\n%s\n", conventions))
+	}
+
+	return sb.String()
+}

review/persona_prompt_test.go

@@ -0,0 +1,157 @@
+package review
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestBuildPersonaSystemPrompt(t *testing.T) {
+	p := &Persona{
+		Name:        "security",
+		DisplayName: "Security Specialist",
+		Identity:    "You are a security specialist.",
+		Focus:       []string{"injection attacks", "auth bypass"},
+		Ignore:      []string{"code style", "performance"},
+		Severity: Severity{
+			Major: "exploitable vulnerabilities",
+			Minor: "defense in depth",
+			Nit:   "theoretical risks",
+		},
+	}
+
+	prompt := BuildPersonaSystemPrompt(p)
+
+	// Check identity is included
+	if !strings.Contains(prompt, "You are a security specialist.") {
+		t.Error("prompt should contain identity")
+	}
+
+	// Check focus areas
+	if !strings.Contains(prompt, "Focus Areas") {
+		t.Error("prompt should contain Focus Areas section")
+	}
+	if !strings.Contains(prompt, "injection attacks") {
+		t.Error("prompt should contain focus item")
+	}
+
+	// Check ignore section
+	if !strings.Contains(prompt, "Out of Scope") {
+		t.Error("prompt should contain Out of Scope section")
+	}
+	if !strings.Contains(prompt, "code style") {
+		t.Error("prompt should contain ignore item")
+	}
+
+	// Check severity calibration
+	if !strings.Contains(prompt, "Severity Calibration") {
+		t.Error("prompt should contain Severity Calibration section")
+	}
+	if !strings.Contains(prompt, "exploitable vulnerabilities") {
+		t.Error("prompt should contain major severity definition")
+	}
+
+	// Check JSON output format is included
+	if !strings.Contains(prompt, `"verdict"`) {
+		t.Error("prompt should contain JSON output format")
+	}
+	if !strings.Contains(prompt, "APPROVE") {
+		t.Error("prompt should mention APPROVE verdict")
+	}
+}
+
+func TestBuildPersonaSystemPromptMinimal(t *testing.T) {
+	// Minimal persona with only required fields
+	p := &Persona{
+		Name:     "minimal",
+		Identity: "You are a minimal reviewer.",
+	}
+
+	prompt := BuildPersonaSystemPrompt(p)
+
+	// Should still work without optional fields
+	if !strings.Contains(prompt, "You are a minimal reviewer.") {
+		t.Error("prompt should contain identity")
+	}
+
+	// Should not have empty sections
+	if strings.Contains(prompt, "Focus Areas") && !strings.Contains(prompt, "Concentrate your review on:") {
+		t.Error("should not have Focus Areas header without content")
+	}
+}
+
+func TestBuildSystemPromptWithPersona(t *testing.T) {
+	t.Run("with persona", func(t *testing.T) {
+		p := &Persona{
+			Name:     "test",
+			Identity: "Test persona identity.",
+			Focus:    []string{"testing"},
+		}
+
+		prompt := BuildSystemPromptWithPersona(p, "test conventions", "test patterns")
+
+		if !strings.Contains(prompt, "Test persona identity.") {
+			t.Error("should contain persona identity")
+		}
+		if !strings.Contains(prompt, "test conventions") {
+			t.Error("should contain conventions")
+		}
+		if !strings.Contains(prompt, "test patterns") {
+			t.Error("should contain patterns")
+		}
+	})
+
+	t.Run("without persona", func(t *testing.T) {
+		prompt := BuildSystemPromptWithPersona(nil, "test conventions", "test patterns")
+
+		// Should use default system base
+		if !strings.Contains(prompt, "expert code reviewer") {
+			t.Error("should contain default system base when no persona")
+		}
+		if !strings.Contains(prompt, "test conventions") {
+			t.Error("should contain conventions")
+		}
+	})
+
+	t.Run("empty conventions and patterns", func(t *testing.T) {
+		p := &Persona{
+			Name:     "test",
+			Identity: "Test identity.",
+		}
+
+		prompt := BuildSystemPromptWithPersona(p, "", "")
+
+		if strings.Contains(prompt, "Language Patterns") {
+			t.Error("should not contain patterns section when empty")
+		}
+		if strings.Contains(prompt, "Repository Conventions") {
+			t.Error("should not contain conventions section when empty")
+		}
+	})
+}
+
+func TestPersonaPromptContainsOutputRules(t *testing.T) {
+	p := &Persona{
+		Name:     "test",
+		Identity: "Test.",
+	}
+
+	prompt := BuildPersonaSystemPrompt(p)
+
+	// Must contain the critical output rules
+	requiredStrings := []string{
+		"APPROVE",
+		"REQUEST_CHANGES",
+		"MAJOR",
+		"MINOR",
+		"NIT",
+		"verdict",
+		"findings",
+		"CI",
+	}
+
+	for _, s := range requiredStrings {
+		if !strings.Contains(prompt, s) {
+			t.Errorf("prompt should contain %q", s)
+		}
+	}
+}

review/persona_test.go

@@ -0,0 +1,959 @@
+package review
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/goccy/go-yaml/ast"
+)
+
+func TestLoadBuiltinPersona(t *testing.T) {
+	tests := []struct {
+		name        string
+		personaName string
+		wantErr     bool
+		wantDisplay string
+	}{
+		{
+			name:        "security persona",
+			personaName: "security",
+			wantErr:     false,
+			wantDisplay: "Security Specialist",
+		},
+		{
+			name:        "architect persona",
+			personaName: "architect",
+			wantErr:     false,
+			wantDisplay: "Software Architect",
+		},
+		{
+			name:        "docs persona",
+			personaName: "docs",
+			wantErr:     false,
+			wantDisplay: "Documentation Reviewer",
+		},
+		{
+			name:        "unknown persona",
+			personaName: "nonexistent",
+			wantErr:     true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			p, err := LoadBuiltinPersona(tt.personaName)
+			if tt.wantErr {
+				if err == nil {
+					t.Error("expected error, got nil")
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if p.Name != tt.personaName {
+				t.Errorf("Name = %q, want %q", p.Name, tt.personaName)
+			}
+			if p.DisplayName != tt.wantDisplay {
+				t.Errorf("DisplayName = %q, want %q", p.DisplayName, tt.wantDisplay)
+			}
+			if p.Identity == "" {
+				t.Error("Identity should not be empty")
+			}
+			if len(p.Focus) == 0 {
+				t.Error("Focus should not be empty")
+			}
+		})
+	}
+}
+
+func TestListBuiltinPersonas(t *testing.T) {
+	names := ListBuiltinPersonas()
+	if len(names) == 0 {
+		t.Fatal("expected at least one built-in persona")
+	}
+
+	// Check for expected personas
+	expected := map[string]bool{"security": false, "architect": false, "docs": false}
+	for _, name := range names {
+		if _, ok := expected[name]; ok {
+			expected[name] = true
+		}
+	}
+	for name, found := range expected {
+		if !found {
+			t.Errorf("expected built-in persona %q not found", name)
+		}
+	}
+}
+
+func TestLoadPersonaFromYAMLFile(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "test.yaml")
+
+	content := `# Test persona
+name: test
+display_name: Test Persona
+identity: |
+  You are a test persona.
+  Multi-line identity works.
+focus:
+  - testing
+  - validation
+ignore:
+  - nothing
+severity:
+  major: Big problems
+  minor: Small problems
+  nit: Tiny problems
+`
+
+	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
+		t.Fatalf("failed to write test file: %v", err)
+	}
+
+	p, err := LoadPersona(path)
+	if err != nil {
+		t.Fatalf("LoadPersona failed: %v", err)
+	}
+
+	if p.Name != "test" {
+		t.Errorf("Name = %q, want %q", p.Name, "test")
+	}
+	if p.DisplayName != "Test Persona" {
+		t.Errorf("DisplayName = %q, want %q", p.DisplayName, "Test Persona")
+	}
+	if len(p.Focus) != 2 {
+		t.Errorf("Focus len = %d, want 2", len(p.Focus))
+	}
+	if !strings.Contains(p.Identity, "Multi-line") {
+		t.Error("Identity should contain multi-line content")
+	}
+}
+
+func TestLoadPersonaFromYMLFile(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "test.yml")
+
+	content := `name: test
+display_name: Test YML
+identity: Test identity
+focus:
+  - testing
+ignore: []
+severity:
+  major: Big
+  minor: Small
+  nit: Tiny
+`
+
+	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
+		t.Fatalf("failed to write test file: %v", err)
+	}
+
+	p, err := LoadPersona(path)
+	if err != nil {
+		t.Fatalf("LoadPersona failed: %v", err)
+	}
+
+	if p.Name != "test" {
+		t.Errorf("Name = %q, want %q", p.Name, "test")
+	}
+	if p.DisplayName != "Test YML" {
+		t.Errorf("DisplayName = %q, want %q", p.DisplayName, "Test YML")
+	}
+}
+
+func TestLoadPersonaFromJSONFile(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "test.json")
+
+	content := `{
+		"name": "test",
+		"display_name": "Test Persona",
+		"identity": "You are a test persona.\nMulti-line identity works.",
+		"focus": ["testing", "validation"],
+
+		"ignore": ["nothing"],
+		"severity": {
+			"major": "Big problems",
+			"minor": "Small problems",
+			"nit": "Tiny problems"
+		}
+	}`
+
+	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
+		t.Fatalf("failed to write test file: %v", err)
+	}
+
+	p, err := LoadPersona(path)
+	if err != nil {
+		t.Fatalf("LoadPersona failed: %v", err)
+	}
+
+	if p.Name != "test" {
+		t.Errorf("Name = %q, want %q", p.Name, "test")
+	}
+	if p.DisplayName != "Test Persona" {
+		t.Errorf("DisplayName = %q, want %q", p.DisplayName, "Test Persona")
+	}
+	if len(p.Focus) != 2 {
+		t.Errorf("Focus len = %d, want 2", len(p.Focus))
+	}
+	if !strings.Contains(p.Identity, "Multi-line") {
+		t.Error("Identity should contain multi-line content")
+	}
+}
+
+func TestLoadPersonaValidation(t *testing.T) {
+	tests := []struct {
+		name    string
+		content string
+		ext     string
+		wantErr string
+	}{
+		{
+			name:    "missing name yaml",
+			content: "identity: test\n",
+			ext:     ".yaml",
+			wantErr: "name is required",
+		},
+		{
+			name:    "missing identity yaml",
+			content: "name: test\n",
+			ext:     ".yaml",
+			wantErr: "identity is required",
+		},
+		{
+			name:    "missing name json",
+			content: `{"identity": "test"}`,
+			ext:     ".json",
+			wantErr: "name is required",
+		},
+		{
+			name:    "missing identity json",
+			content: `{"name": "test"}`,
+			ext:     ".json",
+			wantErr: "identity is required",
+		},
+		{
+			name:    "display_name defaults to name",
+			content: "name: test\nidentity: test identity\n",
+			ext:     ".yaml",
+			// No error expected - should succeed
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			dir := t.TempDir()
+			path := filepath.Join(dir, "test"+tt.ext)
+			if err := os.WriteFile(path, []byte(tt.content), 0644); err != nil {
+				t.Fatalf("failed to write test file: %v", err)
+			}
+
+			p, err := LoadPersona(path)
+			if tt.wantErr != "" {
+				if err == nil {
+					t.Errorf("expected error containing %q, got nil", tt.wantErr)
+					return
+				}
+				if !strings.Contains(err.Error(), tt.wantErr) {
+					t.Errorf("error = %q, want containing %q", err.Error(), tt.wantErr)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			// Check display_name defaulting
+			if p.DisplayName == "" {
+				t.Error("DisplayName should default to Name")
+			}
+			if p.DisplayName != p.Name {
+				t.Errorf("DisplayName should default to Name, got %q", p.DisplayName)
+			}
+		})
+	}
+}
+
+func TestLoadPersonaFileNotFound(t *testing.T) {
+	_, err := LoadPersona("/nonexistent/path/persona.yaml")
+	if err == nil {
+		t.Error("expected error for nonexistent file")
+	}
+}
+
+func TestLoadPersonaInvalidYAML(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "invalid.yaml")
+	if err := os.WriteFile(path, []byte("not valid yaml:\n  - [broken"), 0644); err != nil {
+		t.Fatalf("failed to write test file: %v", err)
+	}
+
+	_, err := LoadPersona(path)
+	if err == nil {
+		t.Error("expected error for invalid YAML")
+	}
+}
+
+func TestLoadPersonaInvalidJSON(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "invalid.json")
+	if err := os.WriteFile(path, []byte("not valid json {"), 0644); err != nil {
+		t.Fatalf("failed to write test file: %v", err)
+	}
+
+	_, err := LoadPersona(path)
+	if err == nil {
+		t.Error("expected error for invalid JSON")
+	}
+}
+
+func TestLoadPersonaCaseInsensitiveExtension(t *testing.T) {
+	tests := []struct {
+		name string
+		ext  string
+	}{
+		{"lowercase yaml", ".yaml"},
+		{"uppercase YAML", ".YAML"},
+		{"mixed case Yaml", ".Yaml"},
+		{"lowercase yml", ".yml"},
+		{"uppercase YML", ".YML"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			dir := t.TempDir()
+			path := filepath.Join(dir, "test"+tt.ext)
+			content := "name: test\nidentity: test identity\n"
+			if err := os.WriteFile(path, []byte(content), 0644); err != nil {
+				t.Fatalf("failed to write test file: %v", err)
+			}
+
+			p, err := LoadPersona(path)
+			if err != nil {
+				t.Fatalf("LoadPersona failed for extension %s: %v", tt.ext, err)
+			}
+			if p.Name != "test" {
+				t.Errorf("Name = %q, want %q", p.Name, "test")
+			}
+		})
+	}
+}
+
+func TestCapitalizeFirst(t *testing.T) {
+	tests := []struct {
+		input string
+		want  string
+	}{
+		{"hello", "Hello"},
+		{"Hello", "Hello"},
+		{"HELLO", "HELLO"},
+		{"a", "A"},
+		{"", ""},
+		{"日本語", "日本語"}, // Non-ASCII: Japanese doesn't have case
+		{"über", "Über"},   // German umlaut
+		{"élève", "Élève"}, // French accent
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.input, func(t *testing.T) {
+			got := CapitalizeFirst(tt.input)
+			if got != tt.want {
+				t.Errorf("CapitalizeFirst(%q) = %q, want %q", tt.input, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestListBuiltinPersonasReturnsEmptySlice(t *testing.T) {
+	// ListBuiltinPersonas should return an empty slice (not nil) on error.
+	// We can't easily test the error case, but we can verify the success case
+	// returns a proper slice.
+	names := ListBuiltinPersonas()
+	if names == nil {
+		t.Error("ListBuiltinPersonas should return empty slice, not nil")
+	}
+}
+
+func TestYAMLMultilineStrings(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "multiline.yaml")
+
+	// Test literal block scalar (|) which preserves newlines
+	content := `name: multiline
+display_name: Multiline Test
+identity: |
+  First line.
+  Second line.
+  Third line.
+focus:
+  - item one
+ignore: []
+severity:
+  major: Major issue
+  minor: Minor issue
+  nit: Nit
+`
+
+	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
+		t.Fatalf("failed to write test file: %v", err)
+	}
+
+	p, err := LoadPersona(path)
+	if err != nil {
+		t.Fatalf("LoadPersona failed: %v", err)
+	}
+
+	// Literal block scalar preserves newlines
+	if !strings.Contains(p.Identity, "\n") {
+		t.Error("Identity should contain newlines from literal block scalar")
+	}
+	if !strings.Contains(p.Identity, "Second line") {
+		t.Error("Identity should contain 'Second line'")
+	}
+}
+
+func TestYAMLComments(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "comments.yaml")
+
+	content := `# This is a comment
+name: commented  # inline comment
+display_name: Commented Persona
+# Another comment
+identity: Test identity
+focus:
+  - item  # comment after item
+ignore: []
+severity:
+  major: Major
+  minor: Minor
+  nit: Nit
+`
+
+	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
+		t.Fatalf("failed to write test file: %v", err)
+	}
+
+	p, err := LoadPersona(path)
+	if err != nil {
+		t.Fatalf("LoadPersona failed: %v", err)
+	}
+
+	// Comments should be ignored
+	if p.Name != "commented" {
+		t.Errorf("Name = %q, want %q", p.Name, "commented")
+	}
+	if p.Focus[0] != "item" {
+		t.Errorf("Focus[0] = %q, want %q", p.Focus[0], "item")
+	}
+}
+
+func TestYAMLDeeplyNestedRejection(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "deeply-nested.yaml")
+
+	// Build a deeply nested YAML structure that exceeds MaxYAMLDepth (20).
+	// Depth accumulation trace for "nested: \n  level0: \n    level1: ...":
+	//   - Document root parsed at depth 0
+	//   - Root MappingNode children (MappingValueNodes) visited at depth 1
+	//   - "nested" MappingValueNode: key at depth 2, value at depth 2
+	//   - Each levelN adds depth via MappingValueNode traversal (key + value)
+	//   - Exact depth per level depends on AST structure (MappingNode wrapping),
+	//     but 25 levels reliably exceeds MaxYAMLDepth (20) with comfortable margin.
+	// The test uses 25 levels rather than exactly 21 to avoid brittleness.
+	var sb strings.Builder
+	sb.WriteString("name: test\nidentity: test\nnested:\n")
+	indent := "  "
+	for i := 0; i < 25; i++ {
+		sb.WriteString(strings.Repeat(indent, i+1))
+		sb.WriteString(fmt.Sprintf("level%d:\n", i))
+	}
+	sb.WriteString(strings.Repeat(indent, 26))
+	sb.WriteString("value: too-deep\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.Error("expected error for deeply nested YAML, got nil")
+	}
+	if !strings.Contains(err.Error(), "nesting depth exceeds") {
+		t.Errorf("error = %q, want containing 'nesting depth exceeds'", err.Error())
+	}
+}
+
+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")
+
+	// Create a file larger than MaxPersonaFileSize (64 KB)
+	content := "name: test\nidentity: " + strings.Repeat("x", MaxPersonaFileSize+1) + "\n"
+	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
+		t.Fatalf("failed to write test file: %v", err)
+	}
+
+	_, err := LoadPersona(path)
+	if err == nil {
+		t.Error("expected error for oversized file, got nil")
+	}
+	if !strings.Contains(err.Error(), "exceeds maximum size") {
+		t.Errorf("error = %q, want containing 'exceeds maximum size'", err.Error())
+	}
+}
+
+func TestYAMLAliasCycleDetection(t *testing.T) {
+	// Test that our checkYAMLDepth function handles alias cycles gracefully
+	// by using the visiting map to prevent infinite recursion.
+
+	// Create a node structure where an alias points to a parent node,
+	// simulating what could happen with crafted input.
+	parent := &ast.MappingNode{
+		Values: []*ast.MappingValueNode{
+			{
+				Key:   &ast.StringNode{Value: "name"},
+				Value: &ast.StringNode{Value: "test"},
+			},
+		},
+	}
+
+	// Create a child that aliases back to the parent (artificial cycle)
+	aliasToParent := &ast.AliasNode{
+		Value: parent,
+	}
+	parent.Values = append(parent.Values, &ast.MappingValueNode{
+		Key:   &ast.StringNode{Value: "nested"},
+		Value: aliasToParent,
+	})
+
+	nodeCount := 0
+	validated := make(map[ast.Node]int)
+	visiting := make(map[ast.Node]bool)
+
+	// This should NOT hang or stack overflow - cycle detection prevents infinite recursion
+	err := checkYAMLDepth(parent, 0, MaxYAMLDepth, MaxYAMLNodes, validated, visiting, &nodeCount)
+	if err != nil {
+		t.Errorf("unexpected error traversing cyclic structure: %v", err)
+	}
+
+	// Verify we tracked the parent in the validated map
+	if _, ok := validated[parent]; !ok {
+		t.Error("parent node not tracked in validated map")
+	}
+}
+
+func TestYAMLMultiDocumentRejection(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "multi.yaml")
+
+	// Multi-document YAML (documents separated by ---)
+	content := `name: first
+identity: first document
+---
+name: second
+identity: second document
+`
+	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
+		t.Fatalf("failed to write test file: %v", err)
+	}
+
+	_, err := LoadPersona(path)
+	if err == nil {
+		t.Error("expected error for multi-document YAML, got nil")
+	}
+	if !strings.Contains(err.Error(), "multi-document") {
+		t.Errorf("error = %q, want containing 'multi-document'", err.Error())
+	}
+}
+
+func TestYAMLNodeCountLimit(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "wide.yaml")
+
+	// Build a YAML structure that's shallow but wide - many keys at the same level
+	// to test the node count limit (should exceed MaxYAMLNodes = 1000)
+	var sb strings.Builder
+	sb.WriteString("name: test\nidentity: test\n")
+	for i := 0; i < 600; i++ {
+		sb.WriteString(fmt.Sprintf("key%d: value%d\n", i, i))
+	}
+
+	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.Error("expected error for wide YAML exceeding node count, got nil")
+	}
+	if !strings.Contains(err.Error(), "node count exceeds") {
+		t.Errorf("error = %q, want containing 'node count exceeds'", err.Error())
+	}
+}
+
+func TestCheckYAMLDepthCycleDetectionDirect(t *testing.T) {
+	// Direct test of cycle detection in checkYAMLDepth by creating
+	// a node structure with an artificial cycle.
+	node := &ast.MappingNode{
+		Values: []*ast.MappingValueNode{
+			{
+				Key:   &ast.StringNode{Value: "key"},
+				Value: &ast.StringNode{Value: "value"},
+			},
+		},
+	}
+
+	// Create a cycle by making a child reference the parent
+	cycleChild := &ast.AliasNode{
+		Value: node, // Points back to the parent
+	}
+	node.Values = append(node.Values, &ast.MappingValueNode{
+		Key:   &ast.StringNode{Value: "cyclic"},
+		Value: cycleChild,
+	})
+
+	nodeCount := 0
+	validated := make(map[ast.Node]int)
+	visiting := make(map[ast.Node]bool)
+	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
+	if len(validated) < 2 {
+		t.Errorf("validated map has %d entries, expected at least 2", len(validated))
+	}
+}
+
+func TestYAMLAliasDepthBypass(t *testing.T) {
+	// Test that an anchored subtree first validated at a shallow depth is
+	// re-checked when referenced via alias at a deeper position. Without the
+	// depth-aware validated map, the alias reference would skip re-checking
+	// and allow the effective nesting to exceed MaxYAMLDepth.
+
+	dir := t.TempDir()
+	path := filepath.Join(dir, "alias-depth-bypass.yaml")
+
+	// Build YAML with an anchor at shallow depth containing a subtree near the limit,
+	// then reference it via alias deep enough that effective depth exceeds MaxYAMLDepth.
+	var sb strings.Builder
+	sb.WriteString("name: test\nidentity: test\n")
+
+	// Create the anchored subtree at depth 1 (key level) that nests 15 levels deep.
+	sb.WriteString("anchor_key: &deep_anchor\n")
+	for i := 0; i < 15; i++ {
+		sb.WriteString(strings.Repeat("  ", i+1))
+		sb.WriteString(fmt.Sprintf("level%d:\n", i))
+	}
+	sb.WriteString(strings.Repeat("  ", 16))
+	sb.WriteString("leaf: value\n")
+
+	// Create a wrapper that nests 6 levels deep, then references the anchor.
+	// Effective depth at alias target = 6 (wrapper nesting) + 1 (alias) + 15 (subtree) = 22 > 20
+	sb.WriteString("wrapper:\n")
+	for i := 0; i < 6; i++ {
+		sb.WriteString(strings.Repeat("  ", i+1))
+		sb.WriteString(fmt.Sprintf("n%d:\n", i))
+	}
+	sb.WriteString(strings.Repeat("  ", 7))
+	sb.WriteString("alias_ref: *deep_anchor\n")
+
+	if err := os.WriteFile(path, []byte(sb.String()), 0644); err != nil {
+		t.Fatalf("failed to write test file: %v", err)
+	}
+
+	_, err := LoadPersona(path)
+	if err == nil {
+		t.Fatal("expected error for alias depth bypass, got nil")
+	}
+	if !strings.Contains(err.Error(), "nesting depth exceeds") {
+		t.Errorf("error = %q, want containing 'nesting depth exceeds'", err.Error())
+	}
+}
+
+func TestListBuiltinPersonasSortedOrder(t *testing.T) {
+	names := ListBuiltinPersonas()
+	if len(names) < 2 {
+		t.Skip("need at least 2 personas to test ordering")
+	}
+
+	// Verify the list is sorted
+	for i := 1; i < len(names); i++ {
+		if names[i-1] > names[i] {
+			t.Errorf("ListBuiltinPersonas not sorted: %q > %q", names[i-1], names[i])
+		}
+	}
+}
+
+func TestYAMLUnknownFieldsRejected(t *testing.T) {
+	tests := []struct {
+		name    string
+		content string
+		wantErr string
+	}{
+		{
+			name: "unknown top-level field",
+			content: `name: test
+identity: test identity
+unknown_field: should fail
+`,
+			wantErr: "unknown_field",
+		},
+		{
+			name: "typo in field name",
+			content: `name: test
+identiy: typo should fail
+`,
+			wantErr: "identiy",
+		},
+		{
+			name: "unknown field in severity",
+			content: `name: test
+identity: test
+severity:
+  major: Major
+  minro: typo
+`,
+			wantErr: "minro",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			dir := t.TempDir()
+			path := filepath.Join(dir, "unknown.yaml")
+			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.Errorf("expected error for unknown field %q, got nil", tt.wantErr)
+				return
+			}
+			if !strings.Contains(err.Error(), tt.wantErr) {
+				t.Errorf("error = %q, want containing %q", err.Error(), tt.wantErr)
+			}
+		})
+	}
+}
+
+func TestJSONUnknownFieldsRejected(t *testing.T) {
+	tests := []struct {
+		name    string
+		content string
+		wantErr string
+	}{
+		{
+			name: "unknown top-level field",
+			content: `{
+				"name": "test",
+				"identity": "test identity",
+				"unknown_field": "should fail"
+			}`,
+			wantErr: "unknown_field",
+		},
+		{
+			name: "typo in field name",
+			content: `{
+				"name": "test",
+				"identiy": "typo should fail"
+			}`,
+			wantErr: "identiy",
+		},
+		{
+			name: "unknown field in severity",
+			content: `{
+				"name": "test",
+				"identity": "test",
+				"severity": {
+					"major": "ok",
+					"miner": "typo"
+				}
+			}`,
+			wantErr: "miner",
+		},
+	}
+
+	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 unknown field, got nil")
+			}
+			if !strings.Contains(err.Error(), tt.wantErr) {
+				t.Errorf("error = %q, want to contain %q", err.Error(), tt.wantErr)
+			}
+		})
+	}
+}
+
+func TestLoadPersonaSymlink(t *testing.T) {
+	// Create a regular persona file
+	dir := t.TempDir()
+	realFile := filepath.Join(dir, "real.yaml")
+	content := `name: test
+identity: test identity
+`
+	if err := os.WriteFile(realFile, []byte(content), 0644); err != nil {
+		t.Fatalf("failed to write test file: %v", err)
+	}
+
+	// Create a symlink to it
+	symlink := filepath.Join(dir, "link.yaml")
+	if err := os.Symlink(realFile, symlink); err != nil {
+		t.Fatalf("failed to create symlink: %v", err)
+	}
+
+	// LoadPersona should work via symlink
+	p, err := LoadPersona(symlink)
+	if err != nil {
+		t.Fatalf("LoadPersona via symlink failed: %v", err)
+	}
+	if p.Name != "test" {
+		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())
+	}
+}

review/personas/architect.yaml

@@ -0,0 +1,37 @@
+# Software Architect Persona
+# Focuses on design quality, patterns, and code organization
+
+name: architect
+display_name: Software Architect
+
+identity: |
+  You are a software architect reviewing code for design quality.
+
+  Your expertise:
+  - Design patterns and anti-patterns
+  - Code organization and module boundaries
+  - API design and contracts
+  - Testability and dependency injection
+  - Consistency with existing architecture
+  - Technical debt identification
+
+focus:
+  - Design pattern violations or misuse
+  - Module boundary violations (inappropriate coupling)
+  - API design issues (unclear contracts, leaky abstractions)
+  - Testability problems (hidden dependencies, god objects)
+  - Inconsistency with existing codebase patterns
+  - Unnecessary complexity or over-engineering
+  - Missing abstractions or premature abstraction
+
+ignore:
+  - Security vulnerabilities (security persona handles these)
+  - Performance micro-optimizations
+  - Code style and formatting
+  - Documentation typos
+  - Test implementation details
+
+severity:
+  major: "Architectural violations that will cause maintenance problems or make the codebase harder to evolve"
+  minor: "Design issues that reduce clarity or testability but don't block progress"
+  nit: "Minor pattern deviations or style preferences"

review/personas/docs.yaml

@@ -0,0 +1,36 @@
+# Documentation Reviewer Persona
+# Focuses on clarity, documentation quality, and self-documenting code
+
+name: docs
+display_name: Documentation Reviewer
+
+identity: |
+  You are a documentation specialist reviewing code for clarity and documentation quality.
+
+  Your expertise:
+  - API documentation and examples
+  - Code comments and their accuracy
+  - Error message clarity
+  - README and guide quality
+  - Naming clarity and self-documenting code
+
+focus:
+  - Missing or outdated documentation
+  - Unclear or misleading comments
+  - Poor error messages (cryptic, unhelpful, missing context)
+  - Confusing naming (functions, variables, types)
+  - Missing examples for complex APIs
+  - Inconsistent terminology
+  - Documentation that contradicts the code
+
+ignore:
+  - Security vulnerabilities
+  - Performance issues
+  - Design patterns
+  - Test coverage
+  - Code style (unless it affects readability)
+
+severity:
+  major: "Documentation that actively misleads or missing docs for critical functionality"
+  minor: "Unclear documentation or poor error messages that will confuse users"
+  nit: "Minor clarity improvements or typo fixes"

review/personas/security.yaml

@@ -0,0 +1,37 @@
+# Security Specialist Persona
+# Focuses on vulnerabilities, auth issues, and security best practices
+
+name: security
+display_name: Security Specialist
+
+identity: |
+  You are a security specialist reviewing code for vulnerabilities.
+
+  Your expertise:
+  - OWASP Top 10 vulnerabilities
+  - Injection attacks (SQL, command, path traversal, template)
+  - Authentication and authorization patterns
+  - Secrets management and exposure risks
+  - Race conditions with security implications
+  - Event sourcing attack vectors (replay attacks, event injection)
+
+focus:
+  - Injection attacks (SQL, command, path traversal, template injection)
+  - Authentication and authorization gaps or bypasses
+  - Secrets exposure (hardcoded credentials, tokens in logs, config leaks)
+  - Input validation failures (unsanitized input, unsafe deserialization)
+  - Race conditions that could be exploited
+  - Cryptographic weaknesses (weak algorithms, improper key handling)
+  - Information disclosure through error messages or logs
+
+ignore:
+  - Code style and naming conventions
+  - Performance optimizations (unless security-related)
+  - Documentation quality
+  - General code quality or readability
+  - Test coverage
+
+severity:
+  major: "Exploitable vulnerabilities: auth bypass, injection, data exfiltration, privilege escalation, RCE"
+  minor: "Defense-in-depth issues: missing rate limiting, verbose errors, weak input validation"
+  nit: "Theoretical risks with low exploitability or impact"

review/prompt.go

@@ -7,6 +7,28 @@ import (
 	"strings"
 )
 
+// outputSchemaJSON is the shared JSON output format specification used by both
+// the generic reviewer and persona-based reviewers.
+const outputSchemaJSON = `{
+  "verdict": "APPROVE" or "REQUEST_CHANGES",
+  "summary": "Brief overall assessment (1-3 sentences)",
+  "findings": [
+    {
+      "severity": "MAJOR" or "MINOR" or "NIT",
+      "file": "path/to/file",
+      "line": <line number from the diff>,
+      "finding": "Description of the issue"
+    }
+  ],
+  "recommendation": "Full recommendation text explaining your verdict"
+}`
+
+// verdictRules is the shared verdict determination rules.
+const verdictRules = `Rules:
+- If there are any MAJOR findings → verdict must be REQUEST_CHANGES
+- If there are no MAJOR findings → verdict should be APPROVE
+- If CI has failed → verdict must be REQUEST_CHANGES with a finding noting the CI failure`
+
 // BuildSystemBase returns the core system prompt instructions without
 // patterns or conventions. Used by the budget package to separate
 // trimmable from non-trimmable content.
@@ -23,24 +45,10 @@ func BuildSystemBase() string {
 	sb.WriteString("2. Consider the CI status — if CI has failed, that is an automatic REQUEST_CHANGES regardless of code quality.\n")
 	sb.WriteString("3. Output your review as structured JSON (and ONLY JSON, no markdown fences or other text).\n\n")
 	sb.WriteString("Output format:\n")
-	sb.WriteString("{\n")
-	sb.WriteString("  \"verdict\": \"APPROVE\" or \"REQUEST_CHANGES\",\n")
-	sb.WriteString("  \"summary\": \"Brief overall assessment (1-3 sentences)\",\n")
-	sb.WriteString("  \"findings\": [\n")
-	sb.WriteString("    {\n")
-	sb.WriteString("      \"severity\": \"MAJOR\" or \"MINOR\" or \"NIT\",\n")
-	sb.WriteString("      \"file\": \"path/to/file\",\n")
-	sb.WriteString("      \"line\": <line number from the diff>,\n")
-	sb.WriteString("      \"finding\": \"Description of the issue\"\n")
-	sb.WriteString("    }\n")
-	sb.WriteString("  ],\n")
-	sb.WriteString("  \"recommendation\": \"Full recommendation text explaining your verdict\"\n")
-	sb.WriteString("}\n\n")
-	sb.WriteString("Rules:\n")
-	sb.WriteString("- If there are any MAJOR findings → verdict must be REQUEST_CHANGES\n")
-	sb.WriteString("- If there are no MAJOR findings → verdict should be APPROVE\n")
-	sb.WriteString("- If CI has failed → verdict must be REQUEST_CHANGES with a finding noting the CI failure\n")
-	sb.WriteString("- Be thorough but fair. Don't nitpick style unless it impacts readability significantly.\n")
+	sb.WriteString(outputSchemaJSON)
+	sb.WriteString("\n\n")
+	sb.WriteString(verdictRules)
+	sb.WriteString("\n- Be thorough but fair. Don't nitpick style unless it impacts readability significantly.\n")
 	sb.WriteString("- Line numbers should reference the new file line numbers from the diff headers.\n")
 	sb.WriteString("- If the diff is empty or trivial (only formatting/whitespace), APPROVE with no findings.\n")
 

review/repo_persona.go

@@ -0,0 +1,150 @@
+package review
+
+import (
+	"context"
+	"log/slog"
+	"strings"
+)
+
+// RepoPersonaPath is the directory path where repo-specific personas are stored.
+const RepoPersonaPath = ".review-bot/personas"
+
+// GiteaClient defines the subset of gitea.Client methods needed for loading repo personas.
+// This interface allows for easier testing and decouples the review package from gitea.
+type GiteaClient interface {
+	ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error)
+	GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error)
+}
+
+// ContentEntry represents a file or directory entry from the contents API.
+// This mirrors gitea.ContentEntry to avoid import cycles.
+type ContentEntry struct {
+	Name string `json:"name"`
+	Path string `json:"path"`
+	Type string `json:"type"` // "file" or "dir"
+}
+
+// LoadRepoPersonas fetches personas from a repository's .review-bot/personas/ directory.
+// Returns an empty map (not nil) if the directory doesn't exist or is empty.
+// Individual parse failures are logged and skipped; the remaining personas are still returned.
+// Auth errors and other non-404 errors are propagated.
+// Files exceeding MaxPersonaFileSize are rejected to prevent resource exhaustion.
+func LoadRepoPersonas(ctx context.Context, client GiteaClient, owner, repo string) (map[string]*Persona, error) {
+	result := make(map[string]*Persona)
+
+	entries, err := client.ListContents(ctx, owner, repo, RepoPersonaPath)
+	if err != nil {
+		// Check if this is a 404 (directory doesn't exist) - expected case
+		if isNotFoundError(err) {
+			slog.Debug("no repo personas directory found", "repo", owner+"/"+repo)
+			return result, nil
+		}
+		// Other errors (auth, server) should propagate
+		return nil, err
+	}
+
+	if len(entries) == 0 {
+		slog.Debug("repo personas directory is empty", "repo", owner+"/"+repo)
+		return result, nil
+	}
+
+	for _, entry := range entries {
+		if entry.Type != "file" {
+			continue
+		}
+		// Only process YAML files
+		if !isYAMLFile(entry.Name) {
+			continue
+		}
+
+		content, err := client.GetFileContent(ctx, owner, repo, entry.Path)
+		if err != nil {
+			slog.Warn("could not fetch repo persona file",
+				"file", entry.Path,
+				"repo", owner+"/"+repo,
+				"error", err)
+			continue
+		}
+
+		// Enforce size limit before parsing to prevent resource exhaustion
+		if len(content) > MaxPersonaFileSize {
+			slog.Warn("repo persona file exceeds maximum size",
+				"file", entry.Path,
+				"repo", owner+"/"+repo,
+				"size", len(content),
+				"max", MaxPersonaFileSize)
+			continue
+		}
+
+		persona, err := ParsePersonaBytes([]byte(content), entry.Path)
+		if err != nil {
+			slog.Warn("could not parse repo persona file",
+				"file", entry.Path,
+				"repo", owner+"/"+repo,
+				"error", err)
+			continue
+		}
+
+		result[persona.Name] = persona
+		slog.Debug("loaded repo persona",
+			"name", persona.Name,
+			"file", entry.Path,
+			"repo", owner+"/"+repo)
+	}
+
+	return result, nil
+}
+
+// MergePersonas combines built-in personas with repo personas.
+// Repo personas take precedence on name collision.
+// Returns a new map; inputs are not modified.
+func MergePersonas(builtin, repo map[string]*Persona) map[string]*Persona {
+	result := make(map[string]*Persona, len(builtin)+len(repo))
+
+	// Copy built-in personas first
+	for name, p := range builtin {
+		result[name] = p
+	}
+
+	// Overlay repo personas (override on collision)
+	for name, p := range repo {
+		if _, exists := result[name]; exists {
+			slog.Debug("repo persona overrides built-in", "name", name)
+		}
+		result[name] = p
+	}
+
+	return result
+}
+
+// GetBuiltinPersonasMap returns all built-in personas as a map keyed by name.
+// Returns an empty map (not nil) if loading fails.
+func GetBuiltinPersonasMap() map[string]*Persona {
+	result := make(map[string]*Persona)
+	for _, name := range ListBuiltinPersonas() {
+		p, err := LoadBuiltinPersona(name)
+		if err != nil {
+			slog.Warn("could not load built-in persona", "name", name, "error", err)
+			continue
+		}
+		result[name] = p
+	}
+	return result
+}
+
+// isYAMLFile checks if a filename has a YAML extension.
+func isYAMLFile(name string) bool {
+	lower := strings.ToLower(name)
+	return strings.HasSuffix(lower, ".yaml") || strings.HasSuffix(lower, ".yml")
+}
+
+// isNotFoundError checks if an error represents a 404 response.
+// This uses a specific "HTTP 404" substring match rather than a generic "not found"
+// match to avoid masking authentication failures or transport errors that might
+// contain "not found" in their message.
+func isNotFoundError(err error) bool {
+	if err == nil {
+		return false
+	}
+	return strings.Contains(err.Error(), "HTTP 404")
+}

review/repo_persona_test.go

@@ -0,0 +1,443 @@
+package review
+
+import (
+	"context"
+	"errors"
+	"strings"
+	"testing"
+)
+
+func TestParsePersonaBytes(t *testing.T) {
+	tests := []struct {
+		name       string
+		data       string
+		source     string
+		wantName   string
+		wantErr    string
+	}{
+		{
+			name: "valid yaml",
+			data: `name: test
+identity: test identity
+focus:
+  - testing
+`,
+			source:   "test.yaml",
+			wantName: "test",
+		},
+		{
+			name:    "missing name",
+			data:    "identity: test\n",
+			source:  "test.yaml",
+			wantErr: "name is required",
+		},
+		{
+			name:    "invalid yaml",
+			data:    "not: valid:\n  yaml: [broken",
+			source:  "test.yaml",
+			wantErr: "parse",
+		},
+		{
+			name: "json format by extension",
+			data: `{"name": "jsontest", "identity": "json identity"}`,
+			source:   "test.json",
+			wantName: "jsontest",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			p, err := ParsePersonaBytes([]byte(tt.data), tt.source)
+			if tt.wantErr != "" {
+				if err == nil {
+					t.Fatalf("expected error containing %q, got nil", tt.wantErr)
+				}
+				if !strings.Contains(err.Error(), tt.wantErr) {
+					t.Errorf("error = %q, want containing %q", err.Error(), tt.wantErr)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if p.Name != tt.wantName {
+				t.Errorf("Name = %q, want %q", p.Name, tt.wantName)
+			}
+		})
+	}
+}
+
+// mockGiteaClient implements GiteaClient for testing.
+type mockGiteaClient struct {
+	contents map[string][]ContentEntry // path -> entries
+	files    map[string]string         // path -> content
+	listErr  error
+	fileErr  map[string]error // path -> error
+}
+
+func (m *mockGiteaClient) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) {
+	if m.listErr != nil {
+		return nil, m.listErr
+	}
+	entries, ok := m.contents[path]
+	if !ok {
+		return nil, errors.New("list contents .review-bot/personas: HTTP 404: not found")
+	}
+	return entries, nil
+}
+
+func (m *mockGiteaClient) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
+	if m.fileErr != nil {
+		if err, ok := m.fileErr[filepath]; ok {
+			return "", err
+		}
+	}
+	content, ok := m.files[filepath]
+	if !ok {
+		return "", errors.New("HTTP 404: file not found")
+	}
+	return content, nil
+}
+
+func TestLoadRepoPersonas(t *testing.T) {
+	ctx := context.Background()
+
+	t.Run("directory not found returns empty map", func(t *testing.T) {
+		client := &mockGiteaClient{} // No contents configured -> 404
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if personas == nil {
+			t.Error("expected empty map, got nil")
+		}
+		if len(personas) != 0 {
+			t.Errorf("expected 0 personas, got %d", len(personas))
+		}
+	})
+
+	t.Run("empty directory returns empty map", func(t *testing.T) {
+		client := &mockGiteaClient{
+			contents: map[string][]ContentEntry{
+				RepoPersonaPath: {},
+			},
+		}
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if len(personas) != 0 {
+			t.Errorf("expected 0 personas, got %d", len(personas))
+		}
+	})
+
+	t.Run("loads valid personas", func(t *testing.T) {
+		client := &mockGiteaClient{
+			contents: map[string][]ContentEntry{
+				RepoPersonaPath: {
+					{Name: "trading.yaml", Path: ".review-bot/personas/trading.yaml", Type: "file"},
+					{Name: "crypto.yaml", Path: ".review-bot/personas/crypto.yaml", Type: "file"},
+				},
+			},
+			files: map[string]string{
+				".review-bot/personas/trading.yaml": `name: trading
+display_name: Trading Expert
+identity: You are a trading expert.
+focus:
+  - order handling
+  - risk management
+`,
+				".review-bot/personas/crypto.yaml": `name: crypto
+display_name: Crypto Expert
+identity: You are a cryptography expert.
+focus:
+  - key management
+  - encryption
+`,
+			},
+		}
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if len(personas) != 2 {
+			t.Fatalf("expected 2 personas, got %d", len(personas))
+		}
+		if personas["trading"] == nil {
+			t.Error("expected trading persona")
+		}
+		if personas["crypto"] == nil {
+			t.Error("expected crypto persona")
+		}
+		if personas["trading"].DisplayName != "Trading Expert" {
+			t.Errorf("trading display name = %q, want %q", personas["trading"].DisplayName, "Trading Expert")
+		}
+	})
+
+	t.Run("skips invalid persona files", func(t *testing.T) {
+		client := &mockGiteaClient{
+			contents: map[string][]ContentEntry{
+				RepoPersonaPath: {
+					{Name: "valid.yaml", Path: ".review-bot/personas/valid.yaml", Type: "file"},
+					{Name: "invalid.yaml", Path: ".review-bot/personas/invalid.yaml", Type: "file"},
+				},
+			},
+			files: map[string]string{
+				".review-bot/personas/valid.yaml": `name: valid
+identity: Valid persona
+`,
+				".review-bot/personas/invalid.yaml": "not valid yaml: [broken",
+			},
+		}
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		// Should have the valid one, skip the invalid
+		if len(personas) != 1 {
+			t.Fatalf("expected 1 persona (skipped invalid), got %d", len(personas))
+		}
+		if personas["valid"] == nil {
+			t.Error("expected valid persona")
+		}
+	})
+
+	t.Run("skips non-yaml files", func(t *testing.T) {
+		client := &mockGiteaClient{
+			contents: map[string][]ContentEntry{
+				RepoPersonaPath: {
+					{Name: "persona.yaml", Path: ".review-bot/personas/persona.yaml", Type: "file"},
+					{Name: "README.md", Path: ".review-bot/personas/README.md", Type: "file"},
+					{Name: "notes.txt", Path: ".review-bot/personas/notes.txt", Type: "file"},
+				},
+			},
+			files: map[string]string{
+				".review-bot/personas/persona.yaml": `name: test
+identity: Test persona
+`,
+				".review-bot/personas/README.md": "# Personas\n\nPut your personas here.",
+			},
+		}
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if len(personas) != 1 {
+			t.Fatalf("expected 1 persona (yaml only), got %d", len(personas))
+		}
+	})
+
+	t.Run("skips subdirectories", func(t *testing.T) {
+		client := &mockGiteaClient{
+			contents: map[string][]ContentEntry{
+				RepoPersonaPath: {
+					{Name: "persona.yaml", Path: ".review-bot/personas/persona.yaml", Type: "file"},
+					{Name: "subdir", Path: ".review-bot/personas/subdir", Type: "dir"},
+				},
+			},
+			files: map[string]string{
+				".review-bot/personas/persona.yaml": `name: test
+identity: Test persona
+`,
+			},
+		}
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if len(personas) != 1 {
+			t.Fatalf("expected 1 persona (files only), got %d", len(personas))
+		}
+	})
+
+	t.Run("propagates auth errors", func(t *testing.T) {
+		client := &mockGiteaClient{
+			listErr: errors.New("HTTP 401: unauthorized"),
+		}
+		_, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err == nil {
+			t.Fatal("expected error for auth failure")
+		}
+		if !strings.Contains(err.Error(), "401") {
+			t.Errorf("error = %q, want containing '401'", err.Error())
+		}
+	})
+
+	t.Run("skips files that fail to fetch", func(t *testing.T) {
+		client := &mockGiteaClient{
+			contents: map[string][]ContentEntry{
+				RepoPersonaPath: {
+					{Name: "good.yaml", Path: ".review-bot/personas/good.yaml", Type: "file"},
+					{Name: "bad.yaml", Path: ".review-bot/personas/bad.yaml", Type: "file"},
+				},
+			},
+			files: map[string]string{
+				".review-bot/personas/good.yaml": `name: good
+identity: Good persona
+`,
+			},
+			fileErr: map[string]error{
+				".review-bot/personas/bad.yaml": errors.New("HTTP 500: internal server error"),
+			},
+		}
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if len(personas) != 1 {
+			t.Fatalf("expected 1 persona (skipped failed fetch), got %d", len(personas))
+		}
+	})
+
+	t.Run("skips oversized files", func(t *testing.T) {
+		// Create a content string that exceeds MaxPersonaFileSize (64KB)
+		oversizedContent := strings.Repeat("a", MaxPersonaFileSize+1)
+		client := &mockGiteaClient{
+			contents: map[string][]ContentEntry{
+				RepoPersonaPath: {
+					{Name: "normal.yaml", Path: ".review-bot/personas/normal.yaml", Type: "file"},
+					{Name: "huge.yaml", Path: ".review-bot/personas/huge.yaml", Type: "file"},
+				},
+			},
+			files: map[string]string{
+				".review-bot/personas/normal.yaml": `name: normal
+identity: Normal sized persona
+`,
+				".review-bot/personas/huge.yaml": oversizedContent,
+			},
+		}
+		personas, err := LoadRepoPersonas(ctx, client, "owner", "repo")
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		// Should have the normal one, skip the oversized
+		if len(personas) != 1 {
+			t.Fatalf("expected 1 persona (skipped oversized), got %d", len(personas))
+		}
+		if personas["normal"] == nil {
+			t.Error("expected normal persona")
+		}
+	})
+}
+
+func TestMergePersonas(t *testing.T) {
+	builtin := map[string]*Persona{
+		"security": {Name: "security", Identity: "Built-in security"},
+		"docs":     {Name: "docs", Identity: "Built-in docs"},
+	}
+	repo := map[string]*Persona{
+		"security": {Name: "security", Identity: "Repo security override"},
+		"trading":  {Name: "trading", Identity: "Repo trading"},
+	}
+
+	merged := MergePersonas(builtin, repo)
+
+	t.Run("repo overrides builtin on collision", func(t *testing.T) {
+		if merged["security"].Identity != "Repo security override" {
+			t.Errorf("security identity = %q, want repo override", merged["security"].Identity)
+		}
+	})
+
+	t.Run("builtin preserved when no collision", func(t *testing.T) {
+		if merged["docs"].Identity != "Built-in docs" {
+			t.Errorf("docs identity = %q, want built-in", merged["docs"].Identity)
+		}
+	})
+
+	t.Run("repo-only persona added", func(t *testing.T) {
+		if merged["trading"] == nil {
+			t.Error("expected trading persona from repo")
+		}
+		if merged["trading"].Identity != "Repo trading" {
+			t.Errorf("trading identity = %q, want repo", merged["trading"].Identity)
+		}
+	})
+
+	t.Run("original maps not modified", func(t *testing.T) {
+		if builtin["trading"] != nil {
+			t.Error("builtin map was modified")
+		}
+		if len(repo) != 2 {
+			t.Error("repo map was modified")
+		}
+	})
+}
+
+func TestGetBuiltinPersonasMap(t *testing.T) {
+	personas := GetBuiltinPersonasMap()
+
+	if len(personas) == 0 {
+		t.Fatal("expected at least one built-in persona")
+	}
+
+	// Verify expected personas exist
+	expected := []string{"security", "architect", "docs"}
+	for _, name := range expected {
+		if personas[name] == nil {
+			t.Errorf("expected built-in persona %q", name)
+		}
+	}
+
+	// Verify personas are valid
+	for name, p := range personas {
+		if p.Name != name {
+			t.Errorf("persona %q has mismatched name %q", name, p.Name)
+		}
+		if p.Identity == "" {
+			t.Errorf("persona %q has empty identity", name)
+		}
+	}
+}
+
+func TestIsYAMLFile(t *testing.T) {
+	tests := []struct {
+		name string
+		want bool
+	}{
+		{"test.yaml", true},
+		{"test.yml", true},
+		{"test.YAML", true},
+		{"test.YML", true},
+		{"test.json", false},
+		{"test.md", false},
+		{"test.txt", false},
+		{"yaml", false},
+		{"yaml.md", false},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := isYAMLFile(tt.name); got != tt.want {
+				t.Errorf("isYAMLFile(%q) = %v, want %v", tt.name, got, tt.want)
+			}
+		})
+	}
+}
+
+func TestIsNotFoundError(t *testing.T) {
+	tests := []struct {
+		err  error
+		want bool
+	}{
+		{nil, false},
+		{errors.New("HTTP 404: not found"), true},
+		{errors.New("HTTP 404"), true},
+		// Intentionally false: generic "not found" could mask auth/transport errors.
+		// Only explicit HTTP 404 responses should be treated as "directory doesn't exist".
+		{errors.New("something not found"), false},
+		{errors.New("HTTP 401: unauthorized"), false},
+		{errors.New("connection refused"), false},
+	}
+
+	for _, tt := range tests {
+		name := "nil"
+		if tt.err != nil {
+			name = tt.err.Error()
+		}
+		t.Run(name, func(t *testing.T) {
+			if got := isNotFoundError(tt.err); got != tt.want {
+				t.Errorf("isNotFoundError(%v) = %v, want %v", tt.err, got, tt.want)
+			}
+		})
+	}
+}

scripts/check-deps.sh

@@ -0,0 +1,127 @@
+#!/usr/bin/env bash
+# check-deps.sh - Enforces the strict dependency allowlist from CONVENTIONS.md
+# Exit 1 if any unapproved import is found.
+#
+# Requires: Bash 4+ (for associative arrays), Go toolchain
+# 
+# The allowlist is parsed from CONVENTIONS.md to maintain a single source of truth.
+# Enforces Scope column: "test only" packages cannot appear in non-test code.
+
+set -euo pipefail
+
+# Check bash version
+if ((BASH_VERSINFO[0] < 4)); then
+    echo "❌ Bash 4+ required (found ${BASH_VERSION})"
+    echo "   On macOS: brew install bash"
+    exit 1
+fi
+
+CONVENTIONS_FILE="${1:-CONVENTIONS.md}"
+
+if [ ! -f "$CONVENTIONS_FILE" ]; then
+    echo "❌ CONVENTIONS.md not found"
+    exit 1
+fi
+
+# Parse approved packages from CONVENTIONS.md table using awk (POSIX-compatible)
+# Format: | `package` | use case | scope |
+declare -A ALLOWED_PROD=()
+declare -A ALLOWED_TEST=()
+
+while IFS= read -r line; do
+    # Use awk to extract package and scope from table row
+    pkg=$(echo "$line" | awk -F'|' '{gsub(/^[[:space:]]*`|`[[:space:]]*$/, "", $2); print $2}')
+    scope=$(echo "$line" | awk -F'|' '{gsub(/^[[:space:]]+|[[:space:]]+$/, "", $4); print tolower($4)}')
+    
+    if [ -n "$pkg" ] && [ "$pkg" != "Package" ] && [[ "$pkg" =~ ^[a-zA-Z] ]]; then
+        if [[ "$scope" == *"test"* ]]; then
+            ALLOWED_TEST["$pkg"]=1
+        else
+            ALLOWED_PROD["$pkg"]=1
+        fi
+    fi
+done < <(grep '| `' "$CONVENTIONS_FILE" 2>/dev/null || true)
+
+ALL_ALLOWED=("${!ALLOWED_PROD[@]}" "${!ALLOWED_TEST[@]}")
+
+if [ ${#ALL_ALLOWED[@]} -eq 0 ]; then
+    echo "⚠️  No approved packages found in $CONVENTIONS_FILE"
+    echo "   (This is fine if you want stdlib-only)"
+fi
+
+# Helper: check if import matches any package in an associative array (literal prefix, no glob)
+matches_allowlist() {
+    local import="$1"
+    shift
+    local -n allowlist=$1
+    
+    for allowed in "${!allowlist[@]}"; do
+        # Exact match
+        if [ "$import" = "$allowed" ]; then
+            return 0
+        fi
+        # Literal prefix match for subpackages: must match "pkg/" exactly
+        if [ "${import#"$allowed/"}" != "$import" ]; then
+            return 0
+        fi
+    done
+    return 1
+}
+
+# Get direct module dependencies from go.mod
+DIRECT_IMPORTS=$(go list -m -f '{{if and (not .Indirect) (not .Main)}}{{.Path}}{{end}}' all 2>&1) || {
+    echo "❌ Failed to list dependencies: $DIRECT_IMPORTS"
+    exit 1
+}
+DIRECT_IMPORTS=$(echo "$DIRECT_IMPORTS" | grep -v '^$' || true)
+
+if [ -z "$DIRECT_IMPORTS" ]; then
+    echo "✅ No external dependencies"
+    exit 0
+fi
+
+# Check ALL direct dependencies are in some allowlist
+VIOLATIONS=""
+while IFS= read -r import; do
+    [ -z "$import" ] && continue
+    
+    if ! matches_allowlist "$import" ALLOWED_PROD && ! matches_allowlist "$import" ALLOWED_TEST; then
+        VIOLATIONS="${VIOLATIONS}  - ${import} (not in allowlist)"$'\n'
+    fi
+done <<< "$DIRECT_IMPORTS"
+
+if [ -n "$VIOLATIONS" ]; then
+    echo "❌ UNAPPROVED DEPENDENCIES DETECTED"
+    echo ""
+    echo "The following imports are not in the allowlist:"
+    printf "%s" "$VIOLATIONS"
+    echo ""
+    echo "To add a dependency, update CONVENTIONS.md (requires Aaron's approval)"
+    exit 1
+fi
+
+# Enforce Scope: test-only packages must not appear in non-test code
+# Get imports used by non-test code only (go list -deps without -test excludes test deps)
+PROD_IMPORTS=$(go list -deps -f '{{if not .Standard}}{{.ImportPath}}{{end}}' ./... 2>/dev/null || true)
+
+TEST_ONLY_IN_PROD=""
+for test_pkg in "${!ALLOWED_TEST[@]}"; do
+    # Use word-boundary matching: exact match or followed by /
+    if echo "$PROD_IMPORTS" | grep -qE "^${test_pkg}(/|\$|$)"; then
+        TEST_ONLY_IN_PROD="${TEST_ONLY_IN_PROD}  - ${test_pkg} (marked 'test only' but used in production code)"$'\n'
+    fi
+done
+
+if [ -n "$TEST_ONLY_IN_PROD" ]; then
+    echo "❌ TEST-ONLY DEPENDENCIES IN PRODUCTION CODE"
+    echo ""
+    printf "%s" "$TEST_ONLY_IN_PROD"
+    echo ""
+    echo "These packages are marked 'test only' in CONVENTIONS.md"
+    echo "and must only be imported from *_test.go files."
+    exit 1
+fi
+
+echo "✅ All dependencies are approved"
+echo "   Direct module deps: $(echo "$DIRECT_IMPORTS" | wc -l | tr -d ' ')"
+echo "   Production allowlist: ${#ALLOWED_PROD[@]}, Test-only allowlist: ${#ALLOWED_TEST[@]}"