feat(persona): add role-based review personas

Add persona system for specialized review roles. Each persona defines:
- A specific review focus (security, architecture, documentation)
- Custom system prompt additions
- Personality/tone adjustments

Built-in personas: security, architect, docs
Custom personas: load from JSON via persona-file flag

Includes workspace validation to prevent path traversal attacks.

Closes #51

.gitea/actions/review/action.yml

@@ -26,40 +26,18 @@ inputs:
     required: false
     default: ''
   llm-base-url:
-    description: 'OpenAI-compatible LLM API base URL (not required for aicore provider)'
-    required: false
-    default: ''
+    description: 'OpenAI-compatible LLM API base URL'
+    required: true
   llm-api-key:
-    description: 'LLM API key (not required for aicore provider)'
-    required: false
-    default: ''
+    description: 'LLM API key'
+    required: true
   llm-model:
     description: 'LLM model name'
     required: true
   llm-provider:
-    description: 'LLM API provider: openai, anthropic, or aicore (default openai)'
+    description: 'LLM API provider: openai or anthropic (default openai)'
     required: false
-    default: 'openai'
-  aicore-client-id:
-    description: 'SAP AI Core client ID (required for aicore provider)'
-    required: false
-    default: ''
-  aicore-client-secret:
-    description: 'SAP AI Core client secret (required for aicore provider)'
-    required: false
-    default: ''
-  aicore-auth-url:
-    description: 'SAP AI Core authentication URL (required for aicore provider)'
-    required: false
-    default: ''
-  aicore-api-url:
-    description: 'SAP AI Core API URL (required for aicore provider)'
-    required: false
-    default: ''
-  aicore-resource-group:
-    description: 'SAP AI Core resource group (default: default)'
-    required: false
-    default: 'default'
+    default: 'openai' 
   conventions-file:
     description: 'Path to conventions file in the repo (e.g. CLAUDE.md)'
     required: false
@@ -101,7 +79,7 @@ inputs:
     required: false
     default: ''
   persona-file:
-    description: 'Path to custom persona JSON file'
+    description: 'Path to persona JSON file with custom review focus'
     required: false
     default: ''
 
@@ -187,11 +165,6 @@ runs:
         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,10 +18,8 @@ jobs:
       - run: go vet ./...
       - run: go build -o review-bot ./cmd/review-bot
 
-  # Self-review using native SAP AI Core provider
-  # Models must match SAP AI Core deployments
-  # Available models: gpt-5, anthropic--claude-4.6-sonnet, anthropic--claude-4.6-opus
-  # Removed gpt-4.1, gpt-5-mini, gpt-4.1-mini - not deployed on AI Core
+  # Self-review: builds from source since we're pre-release
+  # Models configured to match SAP AI Core deployments
   review:
     runs-on: ubuntu-24.04
     if: github.event_name == 'pull_request'
@@ -31,15 +29,19 @@ jobs:
         include:
           - name: sonnet
             token_secret: SONNET_REVIEW_TOKEN
+            provider: anthropic
+            llm_path: /anthropic/v1
             model: anthropic--claude-4.6-sonnet
           - name: gpt
             token_secret: GPT_REVIEW_TOKEN
+            provider: openai
+            llm_path: /openai/v1
             model: gpt-5
           - name: security
             token_secret: SECURITY_REVIEW_TOKEN
+            provider: openai
+            llm_path: /openai/v1
             model: gpt-5
-            patterns_repo: rodin/security-patterns
-            patterns_files: "."
             system_prompt_file: SECURITY_REVIEW.md
     steps:
       - uses: actions/checkout@v4
@@ -54,16 +56,13 @@ jobs:
           PR_NUMBER: ${{ github.event.pull_request.number }}
           REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }}
           REVIEWER_NAME: ${{ matrix.name }}
-          LLM_PROVIDER: aicore
+          LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }}${{ matrix.llm_path }}
+          LLM_API_KEY: ${{ secrets.LLM_API_KEY }}
           LLM_MODEL: ${{ matrix.model }}
-          AICORE_CLIENT_ID: ${{ secrets.AICORE_CLIENT_ID }}
-          AICORE_CLIENT_SECRET: ${{ secrets.AICORE_CLIENT_SECRET }}
-          AICORE_AUTH_URL: ${{ secrets.AICORE_AUTH_URL }}
-          AICORE_API_URL: ${{ secrets.AICORE_API_URL }}
-          AICORE_RESOURCE_GROUP: ${{ secrets.AICORE_RESOURCE_GROUP }}
+          LLM_PROVIDER: ${{ matrix.provider }}
           CONVENTIONS_FILE: "CONVENTIONS.md"
-          PATTERNS_REPO: ${{ matrix.patterns_repo || 'rodin/go-patterns' }}
-          PATTERNS_FILES: ${{ matrix.patterns_files || 'README.md,patterns/' }}
+          PATTERNS_REPO: "rodin/go-patterns"
+          PATTERNS_FILES: "README.md,patterns/"
           LLM_TIMEOUT: "600"
           SYSTEM_PROMPT_FILE: ${{ matrix.system_prompt_file }}
         run: ./review-bot

CONVENTIONS.md

@@ -2,28 +2,8 @@
 
 ## 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 (persona files, config) | production |
-| `github.com/google/go-cmp` | Test comparisons (`cmp.Diff`) | test only |
-
-**Any import not in this table or the Go standard library is forbidden.**
-
-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
-
-<!-- Deviation from step 1+3 for go-yaml migration: see #91 for rationale. -->
-
-*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 check-deps precommit
+.PHONY: build test test-integration lint clean coverage
 
 build:
 	go build -o review-bot ./cmd/review-bot/
@@ -12,15 +12,9 @@ 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, Anthropic Messages API, and SAP AI Core
+- **Multi-provider**: OpenAI-compatible and Anthropic Messages API
 - **Context-aware**: Fetches full file content, conventions, language patterns, CI status
 - **Smart budget**: Automatically trims context to fit model token limits
 - **Idempotent reviews**: Posts new review, then cleans up stale ones (one review per bot)
 - **Custom prompts**: Load additional instructions from a file (e.g. security-focused review)
-- **Minimal dependencies**: Go stdlib + `gopkg.in/yaml.v3` only
+- **Zero dependencies**: Go stdlib only
 
 ## Quick Start: Composite Action
 
@@ -168,56 +168,28 @@ 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` | No* | `""` | LLM API base URL (required unless using aicore provider) |
-| `llm-api-key` | No* | `""` | LLM API key (required unless using aicore provider) |
+| `llm-base-url` | Yes | — | LLM API base URL |
+| `llm-api-key` | Yes | — | LLM API key |
 | `llm-model` | Yes | — | Model name |
-| `llm-provider` | No | `openai` | API provider: `openai`, `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 |
+| `llm-provider` | No | `openai` | API provider: `openai` or `anthropic` |
 | `conventions-file` | No | `""` | Path to coding conventions file in the repo |
 | `patterns-repo` | No | `""` | Comma-separated repos with language patterns (e.g. `rodin/go-patterns`) |
 | `patterns-files` | No | `README.md` | Files/directories to fetch from pattern repos |
 | `system-prompt-file` | No | `""` | Local file with additional system prompt instructions |
 | `persona` | No | `""` | Built-in persona name (security, architect, docs) |
-| `persona-file` | No | `""` | Path to persona file (YAML or JSON) with custom review focus |
+| `persona-file` | No | `""` | Path to persona JSON file with custom review focus |
 | `temperature` | No | `0` | LLM temperature (0 = server default) |
 | `timeout` | No | `300` | LLM request timeout in seconds |
 | `dry-run` | No | `false` | Print review to stdout instead of posting |
 | `update-existing` | No | `true` | Delete previous review from same bot before posting. Accepts: true/1/yes or false/0/no |
 | `version` | No | `latest` | review-bot version to install |
 
-*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:
@@ -329,12 +301,11 @@ 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) |
 
-Without `read:user`, the bot still works but cannot add itself to the PR's reviewer list.
+No `read:user` scope needed — the bot identifies itself from the review response.
 
 ## Development
 
@@ -418,7 +389,7 @@ 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
@@ -438,9 +409,9 @@ ignore:
   - 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"
+  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:
@@ -453,13 +424,7 @@ Use it in CI:
     ...
 ```
 
-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.
-
+JSON format is also supported for backwards compatibility.
 
 ### Persona vs system-prompt-file
 

cmd/review-bot/main.go

@@ -65,19 +65,13 @@ func main() {
 	conventionsFile := flag.String("conventions-file", envOrDefault("CONVENTIONS_FILE", ""), "Conventions file path in repo (e.g. CLAUDE.md)")
 	systemPromptFile := flag.String("system-prompt-file", envOrDefault("SYSTEM_PROMPT_FILE", ""), "Local file with additional system prompt instructions")
 	patternsRepo := flag.String("patterns-repo", envOrDefault("PATTERNS_REPO", ""), "Repo with language patterns (e.g. rodin/elixir-patterns)")
-	patternsFiles := flag.String("patterns-files", envOrDefault("PATTERNS_FILES", ""), "Comma-separated file paths to fetch from patterns repo (empty = all files)")
+	patternsFiles := flag.String("patterns-files", envOrDefault("PATTERNS_FILES", "README.md"), "Comma-separated file paths to fetch from patterns repo")
 	dryRun := flag.Bool("dry-run", false, "Print review to stdout instead of posting")
 	llmTemp := flag.Float64("llm-temperature", envOrDefaultFloat("LLM_TEMPERATURE", 0), "LLM temperature (0 = server default)")
 	llmTimeout := flag.Int("llm-timeout", envOrDefaultInt("LLM_TIMEOUT", 300), "LLM request timeout in seconds (default 300)")
-	llmProvider := flag.String("llm-provider", envOrDefault("LLM_PROVIDER", "openai"), "LLM API provider: openai, anthropic, or aicore")
+	llmProvider := flag.String("llm-provider", envOrDefault("LLM_PROVIDER", "openai"), "LLM API provider: openai or anthropic")
 	personaName := flag.String("persona", envOrDefault("PERSONA", ""), "Built-in persona name (security, architect, docs)")
 	personaFile := flag.String("persona-file", envOrDefault("PERSONA_FILE", ""), "Path to persona JSON file")
-	// 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()
 
@@ -92,20 +86,10 @@ func main() {
 	slog.Info("review-bot starting", "version", version)
 
 	// Validate required fields
-	// 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 == "" {
+	if *giteaURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" ||
+		*llmBaseURL == "" || *llmAPIKey == "" || *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-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")
+		fmt.Fprintf(os.Stderr, "Required: --gitea-url, --repo, --pr, --reviewer-token, --llm-base-url, --llm-api-key, --llm-model\n")
 		os.Exit(1)
 	}
 
@@ -115,7 +99,29 @@ func main() {
 		os.Exit(1)
 	}
 
-	// NOTE: Persona loading deferred until after Gitea client init to support repo personas
+	// Load persona if specified
+	var persona *review.Persona
+	if *personaName != "" {
+		var err error
+		persona, err = review.LoadBuiltinPersona(*personaName)
+		if err != nil {
+			slog.Error("failed to load persona", "persona", *personaName, "error", err)
+			os.Exit(1)
+		}
+		slog.Info("loaded built-in persona", "persona", persona.Name, "display", persona.DisplayName)
+	} else if *personaFile != "" {
+		resolvedPath, err := validateWorkspacePath(*personaFile, "persona-file")
+		if err != nil {
+			slog.Error("invalid persona-file path", "error", err)
+			os.Exit(1)
+		}
+		persona, err = review.LoadPersona(resolvedPath)
+		if err != nil {
+			slog.Error("failed to load persona file", "file", *personaFile, "error", err)
+			os.Exit(1)
+		}
+		slog.Info("loaded persona from file", "file", *personaFile, "persona", persona.Name)
+	}
 
 	// Validate reviewer-name: only safe characters allowed in sentinel
 	if err := validateReviewerName(*reviewerName); err != nil {
@@ -151,17 +157,8 @@ 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, aicore")
+		slog.Error("invalid LLM provider", "provider", *llmProvider, "valid", "openai, anthropic")
 		os.Exit(1)
 	}
 	if *llmTimeout > 0 {
@@ -173,43 +170,6 @@ 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
@@ -523,25 +483,11 @@ func fetchFileContext(ctx context.Context, client *gitea.Client, owner, repo, re
 // patternsRepo is comma-separated list of owner/name repos.
 // patternsFiles is comma-separated list of file paths or directories.
 // If a path ends with / or is a directory, all files within it are fetched recursively.
-// If patternsFiles is empty, all files from the repo root are fetched.
 func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patternsFiles string) string {
 	var sb strings.Builder
 
 	repos := strings.Split(patternsRepo, ",")
-
-	// Build the list of paths to fetch
-	var paths []string
-	if patternsFiles == "" {
-		// Empty patternsFiles means "fetch all files from repo root"
-		paths = []string{""}
-	} else {
-		for _, p := range strings.Split(patternsFiles, ",") {
-			p = strings.TrimSpace(p)
-			if p != "" {
-				paths = append(paths, p)
-			}
-		}
-	}
+	paths := strings.Split(patternsFiles, ",")
 
 	for _, repoRef := range repos {
 		if ctx.Err() != nil {
@@ -558,10 +504,12 @@ func fetchPatterns(ctx context.Context, client *gitea.Client, patternsRepo, patt
 		}
 		owner, repo := parts[0], parts[1]
 
-		var repoLoadedFiles []string
-		var repoSkippedFiles []string
-
 		for _, path := range paths {
+			path = strings.TrimSpace(path)
+			if path == "" {
+				continue
+			}
+
 			files, err := client.GetAllFilesInPath(ctx, owner, repo, path)
 			if err != nil {
 				slog.Warn("could not fetch patterns", "path", path, "repo", repoRef, "error", err)
@@ -571,22 +519,11 @@ 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()
 }
@@ -685,28 +622,21 @@ func validateWorkspacePath(path, pathName string) (string, error) {
 	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, "..") {
+	// Check path is within workspace
+	if !strings.HasPrefix(fullPath, absWorkspace+string(filepath.Separator)) && fullPath != absWorkspace {
 		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, "..") {
+	if !strings.HasPrefix(resolvedPath, absWorkspace+string(filepath.Separator)) && resolvedPath != absWorkspace {
 		return "", fmt.Errorf("%s symlink resolves outside workspace: resolved=%s workspace=%s", pathName, resolvedPath, absWorkspace)
 	}
-
 	return resolvedPath, nil
 }
 
@@ -820,32 +750,3 @@ func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool {
 	}
 	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,8 +6,8 @@ import (
 	"log/slog"
 	"os"
 	"os/exec"
-	"path/filepath"
 	"strings"
+	"path/filepath"
 	"testing"
 
 	"gitea.weiker.me/rodin/review-bot/gitea"
@@ -103,13 +103,11 @@ func TestValidateWorkspacePath(t *testing.T) {
 			errMatch:  "resolves outside workspace",
 		},
 		{
-			name:      "absolute path normalized to workspace-relative",
+			name:      "absolute path gets normalized to 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",
+			errMatch:  "failed to resolve", // filepath.Join strips leading / making it <workspace>/etc/passwd which doesn't exist
 		},
 		{
 			name:      "nonexistent file",
@@ -154,6 +152,7 @@ func TestValidateWorkspacePath(t *testing.T) {
 	}
 }
 
+
 func makeReview(id int64, login, state string, stale bool, body string) gitea.Review {
 	r := gitea.Review{
 		ID:    id,
@@ -165,6 +164,7 @@ 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 -->"
@@ -504,52 +504,6 @@ func TestIsPatternFile(t *testing.T) {
 	}
 }
 
-// TestBuildPatternPaths verifies the path-building logic for fetchPatterns.
-// Empty patternsFiles means "fetch all from root" (represented as [""]).
-func TestBuildPatternPaths(t *testing.T) {
-	buildPaths := func(patternsFiles string) []string {
-		if patternsFiles == "" {
-			return []string{""}
-		}
-		var paths []string
-		for _, p := range strings.Split(patternsFiles, ",") {
-			p = strings.TrimSpace(p)
-			if p != "" {
-				paths = append(paths, p)
-			}
-		}
-		return paths
-	}
-
-	tests := []struct {
-		name  string
-		input string
-		want  []string
-	}{
-		{"empty fetches root", "", []string{""}},
-		{"single file", "README.md", []string{"README.md"}},
-		{"multiple files", "README.md,PATTERNS.md", []string{"README.md", "PATTERNS.md"}},
-		{"trims whitespace", " foo.md , bar.md ", []string{"foo.md", "bar.md"}},
-		{"skips empty between commas", "foo.md,,bar.md", []string{"foo.md", "bar.md"}},
-		{"directory path", "patterns/", []string{"patterns/"}},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			got := buildPaths(tc.input)
-			if len(got) != len(tc.want) {
-				t.Errorf("buildPaths(%q) = %v, want %v", tc.input, got, tc.want)
-				return
-			}
-			for i := range got {
-				if got[i] != tc.want[i] {
-					t.Errorf("buildPaths(%q)[%d] = %q, want %q", tc.input, i, got[i], tc.want[i])
-				}
-			}
-		})
-	}
-}
-
 func TestEvaluateCIStatus(t *testing.T) {
 	tests := []struct {
 		name       string
@@ -780,8 +734,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 {

docs/DESIGN-51-personas.md

@@ -1,9 +1,5 @@
 # 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:
@@ -31,14 +27,14 @@ A persona is a named review role with:
 - **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:
+Personas are defined in YAML 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)
+3. Inline via a new `--persona-file` flag
 
 ### 2. Persona File Format
 
-```json
+```yaml
 # .review/personas/security.yaml
 name: security
 display_name: Security Specialist
@@ -81,7 +77,7 @@ output_format: |
 ### 3. New CLI Flags
 
 ```
---persona-file PATH      Path to persona JSON file (local or in repo)
+--persona-file PATH      Path to persona YAML file (local or in repo)
 --persona NAME           Built-in persona name (security, architect, domain)
 ```
 
@@ -322,13 +318,36 @@ Design says header shows "persona display name" but sentinel uses "reviewer-name
 
 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
+## Design Revision: JSON Instead of YAML
 
-**Decision:** Add `gopkg.in/yaml.v3` as a dependency.
+**Reason:** Project convention is "Go standard library only — no external dependencies."
 
-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
+YAML requires `gopkg.in/yaml.v3` or similar. To maintain zero dependencies, persona files will use JSON instead.
 
-The implementation supports both YAML (`.yaml`, `.yml`) and JSON (`.json`) for backwards compatibility, with YAML as the default for built-in personas.
+### Updated Persona File Format
+
+```json
+{
+  "name": "security",
+  "display_name": "Security Specialist",
+  "model_preference": "opus",
+  "identity": "You are a security specialist reviewing code for vulnerabilities.\nYour expertise: OWASP Top 10, injection attacks, auth/authz, secrets management.",
+  "focus": [
+    "Injection attacks (SQL, command, path traversal, template)",
+    "Authentication and authorization gaps",
+    "Secrets exposure (hardcoded credentials, tokens in logs)"
+  ],
+  "ignore": [
+    "Code style and naming conventions",
+    "Performance (unless security-related)",
+    "Documentation"
+  ],
+  "severity": {
+    "major": "Privilege escalation, information disclosure, DoS",
+    "minor": "Missing rate limiting, verbose errors",
+    "nit": "Theoretical risk with low exploitability"
+  }
+}
+```
+
+This maintains all the same fields but uses JSON encoding, which Go handles natively via `encoding/json`.

docs/DESIGN-57-yaml-persona.md

@@ -1,87 +0,0 @@
-# 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,11 +11,9 @@ import (
 	"fmt"
 	"io"
 	"log/slog"
-	"net"
 	"net/http"
 	"net/url"
 	"strings"
-	"syscall"
 	"time"
 )
 
@@ -41,26 +39,12 @@ 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
-}
-
 // 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
 }
 
 // NewClient creates a new Gitea API client.
@@ -72,12 +56,6 @@ func NewClient(baseURL, token string) *Client {
 	}
 }
 
-// SetHTTPClient sets the underlying HTTP client used for requests.
-// This is intended for testing to inject mock transports.
-func (c *Client) SetHTTPClient(hc *http.Client) {
-	c.http = hc
-}
-
 // PullRequest holds relevant PR metadata.
 type PullRequest struct {
 	Title string `json:"title"`
@@ -232,185 +210,24 @@ 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 from a URL for safe logging.
-// This prevents accidental exposure of sensitive data that future callers
-// might pass via query strings.
-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.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()
-}
-
-// doGet 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).
 func (c *Client) doGet(ctx context.Context, reqURL string) ([]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}
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, reqURL, nil)
+	if err != nil {
+		return nil, err
 	}
+	req.Header.Set("Authorization", "token "+c.token)
 
-	// 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 {
-			body, err := io.ReadAll(resp.Body)
-			resp.Body.Close()
-			if err != nil {
-				return nil, err
-			}
-			return body, nil
-		}
-
-		// Error path: limit how much we read from potentially malicious server
-		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
-		}
+	resp, err := c.http.Do(req)
+	if err != nil {
+		return nil, err
 	}
+	defer resp.Body.Close()
 
-	return nil, lastErr
+	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)
 }
 
 // escapePath escapes each segment of a relative file path for use in URLs.
@@ -434,13 +251,7 @@ 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))
@@ -453,16 +264,7 @@ func (c *Client) ListContents(ctx context.Context, owner, repo, path string) ([]
 	}
 	var entries []ContentEntry
 	if err := json.Unmarshal(body, &entries); err != nil {
-		// 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 nil, fmt.Errorf("parse contents JSON: %w", err)
 	}
 	return entries, nil
 }
@@ -515,9 +317,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,14 +6,10 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"net"
 	"net/http"
 	"net/http/httptest"
 	"strings"
-	"sync/atomic"
-	"syscall"
 	"testing"
-	"time"
 )
 
 func TestGetPullRequest(t *testing.T) {
@@ -280,64 +276,11 @@ 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 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"}`)
+			// Gitea returns 404 for contents API on files (it's not a dir)
+			http.NotFound(w, r)
 			return
 		}
 		if r.URL.Path == "/api/v1/repos/owner/repo/raw/README.md" {
@@ -641,9 +584,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},
@@ -800,347 +743,3 @@ 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:  "",
-		},
-	}
-	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)
-			}
-		})
-	}
-}

go.mod

@@ -2,4 +2,4 @@ module gitea.weiker.me/rodin/review-bot
 
 go 1.26.2
 
-require github.com/goccy/go-yaml v1.19.2
+require gopkg.in/yaml.v3 v3.0.1 // indirect

go.sum

@@ -1,2 +1,3 @@
-github.com/goccy/go-yaml v1.19.2 h1:PmFC1S6h8ljIz6gMRBopkjP1TVT7xuwrButHID66PoM=
-github.com/goccy/go-yaml v1.19.2/go.mod h1:XBurs7gK8ATbW4ZPGKgcbrY1Br56PdM69F7LkFRi1kA=
+gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
+gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
+gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=

llm/aicore.go

@@ -1,391 +0,0 @@
-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

@@ -1,535 +0,0 @@
-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), Anthropic Messages API, and SAP AI Core providers.
+// Supports OpenAI-compatible (default) and Anthropic Messages API providers.
 package llm
 
 import (
@@ -22,8 +22,6 @@ 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.
@@ -37,7 +35,6 @@ 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.
@@ -52,12 +49,8 @@ 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
 }
 
@@ -67,21 +60,12 @@ func (c *Client) WithTemperature(t float64) *Client {
 	return c
 }
 
-// WithProvider sets the API provider format (openai, anthropic, or aicore).
+// WithProvider sets the API provider format (openai or anthropic).
 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"`
@@ -98,8 +82,6 @@ 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)
 		}
@@ -124,18 +106,6 @@ 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 {
@@ -206,12 +176,11 @@ func (c *Client) completeOpenAI(ctx context.Context, messages []Message) (string
 // --- Anthropic Messages API implementation ---
 
 type anthropicRequest struct {
-	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"`
+	Model       string            `json:"model"`
+	MaxTokens   int               `json:"max_tokens"`
+	System      string            `json:"system,omitempty"`
+	Messages    []anthropicMsg    `json:"messages"`
+	Temperature float64           `json:"temperature,omitempty"`
 }
 
 type anthropicMsg struct {

review/formatter.go

@@ -7,7 +7,39 @@ 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)
+	var sb strings.Builder
+
+	if reviewerName != "" {
+		title := strings.ToUpper(reviewerName[:1]) + reviewerName[1:]
+		sb.WriteString(fmt.Sprintf("# %s Review\n\n", title))
+	}
+
+	sb.WriteString("## Summary\n\n")
+	sb.WriteString(result.Summary)
+	sb.WriteString("\n\n")
+
+	if len(result.Findings) > 0 {
+		sb.WriteString("## Findings\n\n")
+		sb.WriteString("| # | Severity | File | Line | Finding |\n")
+		sb.WriteString("|---|----------|------|------|--------|\n")
+
+		for i, f := range result.Findings {
+			sb.WriteString(fmt.Sprintf("| %d | [%s] | `%s` | %d | %s |\n",
+				i+1, f.Severity, f.File, f.Line, f.Finding))
+		}
+		sb.WriteString("\n")
+	}
+
+	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))
+		// Hidden sentinel for identifying this bot's reviews during cleanup
+		sb.WriteString(fmt.Sprintf("\n<!-- review-bot:%s -->\n", reviewerName))
+	}
+
+	return sb.String()
 }
 
 // GiteaEvent converts the verdict to the Gitea API event string.
@@ -23,8 +55,6 @@ func GiteaEvent(verdict string) string {
 }
 
 // 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 {
@@ -37,7 +67,7 @@ func FormatMarkdownWithDisplay(result *ReviewResult, displayName, sentinelName s
 	}
 
 	if headerName != "" {
-		title := CapitalizeFirst(headerName)
+		title := strings.ToUpper(headerName[:1]) + headerName[1:]
 		sb.WriteString(fmt.Sprintf("# %s Review\n\n", title))
 	}
 

review/persona.go

@@ -1,36 +1,19 @@
 package review
 
 import (
-	"bytes"
 	"embed"
 	"encoding/json"
 	"fmt"
-	"io"
 	"os"
-	"sort"
+	"path/filepath"
 	"strings"
-	"unicode/utf8"
 
-	"github.com/goccy/go-yaml"
-	"github.com/goccy/go-yaml/ast"
-	"github.com/goccy/go-yaml/parser"
+	"gopkg.in/yaml.v3"
 )
 
 //go:embed personas/*.yaml
 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"`
@@ -51,294 +34,71 @@ type Severity struct {
 	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.
+// LoadPersona loads a persona from a file path.
+// Supports both YAML (.yaml, .yml) and JSON (.json) formats.
 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)
+	filename := name + ".yaml"
+	data, err := embeddedPersonas.ReadFile(filepath.Join("personas", filename))
 	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)
+	return parsePersona(data, "builtin:"+name)
 }
 
-// ListBuiltinPersonas returns the names of all built-in personas in sorted order.
-// Returns an empty slice if the embedded directory cannot be read.
+// ListBuiltinPersonas returns the names of all built-in personas.
 func ListBuiltinPersonas() []string {
 	entries, err := embeddedPersonas.ReadDir("personas")
 	if err != nil {
-		return []string{}
+		return nil
 	}
-	seen := make(map[string]bool)
+	var names []string
 	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
+		if strings.HasSuffix(name, ".yaml") {
+			names = append(names, strings.TrimSuffix(name, ".yaml"))
+		} else if strings.HasSuffix(name, ".yml") {
+			names = append(names, strings.TrimSuffix(name, ".yml"))
 		}
-		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)
+	
+	// Determine format by extension or try YAML first (it's a superset of JSON)
+	ext := strings.ToLower(filepath.Ext(source))
+	if ext == ".json" {
+		if err := json.Unmarshal(data, &p); err != nil {
+			return nil, fmt.Errorf("parse persona %s: %w", source, err)
+		}
 	} 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")
-			}
+		// YAML (also handles .yaml, .yml, and builtin: prefix)
+		if err := yaml.Unmarshal(data, &p); err != nil {
+			return nil, fmt.Errorf("parse persona %s: %w", source, err)
 		}
 	}
-	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)
@@ -352,16 +112,3 @@ func validatePersona(p *Persona, source string) error {
 	}
 	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

@@ -50,7 +50,7 @@ func BuildPersonaSystemPrompt(p *Persona) string {
 		sb.WriteString("\n")
 	}
 
-	// Output format instructions (shared schema from prompt.go)
+	// Output format instructions (same as base, but with persona context)
 	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")
@@ -61,10 +61,24 @@ func BuildPersonaSystemPrompt(p *Persona) string {
 	sb.WriteString("2. Consider the CI status — if CI has failed, that is an automatic REQUEST_CHANGES regardless of code quality.\n")
 	sb.WriteString("3. Output your review as structured JSON (and ONLY JSON, no markdown fences or other text).\n\n")
 	sb.WriteString("Output format:\n")
-	sb.WriteString(outputSchemaJSON)
-	sb.WriteString("\n\n")
-	sb.WriteString(verdictRules)
-	sb.WriteString("\n- Only report findings within your focus areas. Ignore everything else.\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("- 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")
 

review/persona_test.go

@@ -1,13 +1,10 @@
 package review
 
 import (
-	"fmt"
 	"os"
 	"path/filepath"
 	"strings"
 	"testing"
-
-	"github.com/goccy/go-yaml/ast"
 )
 
 func TestLoadBuiltinPersona(t *testing.T) {
@@ -94,7 +91,7 @@ func TestLoadPersonaFromYAMLFile(t *testing.T) {
 	dir := t.TempDir()
 	path := filepath.Join(dir, "test.yaml")
 
-	content := `# Test persona
+	content := `
 name: test
 display_name: Test Persona
 identity: |
@@ -134,39 +131,6 @@ severity:
 	}
 }
 
-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")
@@ -174,9 +138,8 @@ func TestLoadPersonaFromJSONFile(t *testing.T) {
 	content := `{
 		"name": "test",
 		"display_name": "Test Persona",
-		"identity": "You are a test persona.\nMulti-line identity works.",
-		"focus": ["testing", "validation"],
-
+		"identity": "You are a test persona.",
+		"focus": ["testing"],
 		"ignore": ["nothing"],
 		"severity": {
 			"major": "Big problems",
@@ -200,49 +163,27 @@ func TestLoadPersonaFromJSONFile(t *testing.T) {
 	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
+		yaml    string
 		wantErr string
 	}{
 		{
-			name:    "missing name yaml",
-			content: "identity: test\n",
-			ext:     ".yaml",
+			name:    "missing name",
+			yaml:    "identity: test",
 			wantErr: "name is required",
 		},
 		{
-			name:    "missing identity yaml",
-			content: "name: test\n",
-			ext:     ".yaml",
+			name:    "missing identity",
+			yaml:    "name: test",
 			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",
+			name: "display_name defaults to name",
+			yaml: "name: test\nidentity: test identity",
 			// No error expected - should succeed
 		},
 	}
@@ -250,8 +191,8 @@ func TestLoadPersonaValidation(t *testing.T) {
 	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 {
+			path := filepath.Join(dir, "test.yaml")
+			if err := os.WriteFile(path, []byte(tt.yaml), 0644); err != nil {
 				t.Fatalf("failed to write test file: %v", err)
 			}
 
@@ -290,7 +231,7 @@ func TestLoadPersonaFileNotFound(t *testing.T) {
 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 {
+	if err := os.WriteFile(path, []byte("not: valid: yaml: here"), 0644); err != nil {
 		t.Fatalf("failed to write test file: %v", err)
 	}
 
@@ -299,661 +240,3 @@ func TestLoadPersonaInvalidYAML(t *testing.T) {
 		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

@@ -1,12 +1,9 @@
-# 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

review/personas/docs.yaml

@@ -1,12 +1,9 @@
-# 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

review/personas/security.yaml

@@ -1,12 +1,9 @@
-# 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)

review/prompt.go

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

review/repo_persona.go

@@ -1,150 +0,0 @@
-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

@@ -1,443 +0,0 @@
-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

@@ -1,127 +0,0 @@
-#!/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[@]}"