ci: retrigger after LLM_BASE_URL secret fix

Rewrites the JSON repair algorithm to address two correctness bugs
identified in code review:

1. Interior quoted word before comma: "say "yes", and go" was
   misidentified as structural close because "," followed the quote.

2. JSON-shaped content in strings: {"key": "val"} inside a string
   value was being parsed as actual JSON structure.

The new approach:
- Distinguishes keys from values (only values need repair)
- Uses first-valid-candidate scan with deep lookahead
- Verifies that after a candidate close, the continuation is not just
  a structural char but a complete valid JSON pattern
- Validates keyword tokens (true/false/null) fully, not just first char
- Checks container closes recursively for valid continuation

Adds comprehensive tests for all reported edge cases plus a complex
combined scenario with nested JSON-like content, quoted words before
commas, and multiple failure modes in one string.

.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
@@ -96,14 +74,6 @@ inputs:
     description: 'Local file with additional system prompt instructions (e.g. security review focus)'
     required: false
     default: ''
-  persona:
-    description: 'Built-in persona name (security, architect, docs)'
-    required: false
-    default: ''
-  persona-file:
-    description: 'Path to custom persona JSON file'
-    required: false
-    default: ''
 
 runs:
   using: 'composite'
@@ -185,13 +155,6 @@ runs:
         LLM_PROVIDER: ${{ inputs.llm-provider }}
         UPDATE_EXISTING: ${{ inputs.update-existing }}
         SYSTEM_PROMPT_FILE: ${{ inputs.system-prompt-file }}
-        PERSONA: ${{ inputs.persona }}
-        PERSONA_FILE: ${{ inputs.persona-file }}
-        AICORE_CLIENT_ID: ${{ inputs.aicore-client-id }}
-        AICORE_CLIENT_SECRET: ${{ inputs.aicore-client-secret }}
-        AICORE_AUTH_URL: ${{ inputs.aicore-auth-url }}
-        AICORE_API_URL: ${{ inputs.aicore-api-url }}
-        AICORE_RESOURCE_GROUP: ${{ inputs.aicore-resource-group }}
       run: |
         ARGS=""
         if [ "${{ inputs.dry-run }}" = "true" ]; then

.gitea/workflows/ci.yml

@@ -18,10 +18,7 @@ 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
   review:
     runs-on: ubuntu-24.04
     if: github.event_name == 'pull_request'
@@ -31,10 +28,10 @@ jobs:
         include:
           - name: sonnet
             token_secret: SONNET_REVIEW_TOKEN
-            model: anthropic--claude-4.6-sonnet
+            model: gpt-5
           - name: gpt
             token_secret: GPT_REVIEW_TOKEN
-            model: gpt-5
+            model: gpt-4.1
           - name: security
             token_secret: SECURITY_REVIEW_TOKEN
             model: gpt-5
@@ -52,13 +49,9 @@ 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 }}
+          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 }}
           CONVENTIONS_FILE: "CONVENTIONS.md"
           PATTERNS_REPO: "rodin/go-patterns"
           PATTERNS_FILES: "README.md,patterns/"

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

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

CONVENTIONS.md

@@ -2,26 +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 |
-|---------|----------|-------|
-| `gopkg.in/yaml.v3` | YAML parsing (persona files, config) | production |
-| `github.com/google/go-cmp` | Test comparisons (`cmp.Diff`) | test only |
-
-**Any import not in this table or the Go standard library is forbidden.**
-
-Transitive dependencies of approved packages are automatically allowed.
-
-To request a new dependency:
-1. Open a PR that ONLY updates this table
-2. Requires explicit approval from Aaron
-3. After merge, a separate PR may use the package
-
-*Enforcement: `scripts/check-deps.sh` parses this table — update only here.*
 
 ## Error Handling
 

Makefile

@@ -1,4 +1,4 @@
-.PHONY: build test test-integration lint clean coverage 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,26 @@ 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 |
 | `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:
@@ -359,151 +329,3 @@ budget/             Token estimation + context trimming
 ## License
 
 MIT
-
-## Review Personas
-
-Personas provide role-based review specialization. Instead of generic code review, each persona focuses on a specific domain (security, architecture, documentation) with tailored prompts and severity calibration.
-
-### Built-in Personas
-
-| Persona | Focus |
-|---------|-------|
-| `security` | Vulnerabilities, auth bypass, secrets exposure, injection attacks |
-| `architect` | Design patterns, code organization, API contracts, testability |
-| `docs` | Documentation quality, API clarity, error messages |
-
-### Using Built-in Personas
-
-```yaml
-- uses: rodin/review-bot/.gitea/actions/review@v1
-  with:
-    reviewer-name: security
-    persona: security
-    llm-model: claude-opus-4-20250514  # Security benefits from strong reasoning
-    ...
-```
-
-### Multiple Personas in Parallel
-
-```yaml
-jobs:
-  review:
-    strategy:
-      matrix:
-        include:
-          - name: security
-            persona: security
-          - name: architect
-            persona: architect
-    steps:
-      - uses: rodin/review-bot/.gitea/actions/review@v1
-        with:
-          reviewer-name: ${{ matrix.name }}
-          persona: ${{ matrix.persona }}
-          ...
-```
-
-Each persona posts independently with its own sentinel, so reviews don't interfere.
-
-
-### Custom Personas
-
-Create a YAML file with your domain-specific review focus:
-
-```yaml
-# .review/personas/trading.yaml
-name: trading
-display_name: Trading Domain Expert
-
-identity: |
-  You are a trading systems expert reviewing code for correctness.
-
-  Your expertise:
-  - Order lifecycle and state machines
-  - Fill handling and partial fills
-  - Position tracking and P&L calculations
-  - Event sourcing invariants
-
-focus:
-  - Order state machine correctness
-  - Fill handling edge cases (partial, overfill)
-  - Position and P&L calculation accuracy
-  - Event replay determinism
-  - Decimal precision for money
-
-ignore:
-  - Code style
-  - General performance
-  - Documentation formatting
-
-severity:
-  major: "Bugs that cause incorrect positions, fills, or money calculations"
-  minor: "Edge cases that could cause issues under unusual conditions"
-  nit: "Clarity improvements for domain logic"
-```
-
-Use it in CI:
-
-```yaml
-- uses: rodin/review-bot/.gitea/actions/review@v1
-  with:
-    reviewer-name: trading
-    persona-file: .review/personas/trading.yaml
-    ...
-```
-
-YAML is the recommended format for personas because it supports:
-- Multi-line strings with `|` blocks (cleaner identity definitions)
-- Comments for documentation
-- More readable arrays and nested structures
-
-JSON is also supported for backwards compatibility—just use `.json` extension.
-
-### Repository Personas (Auto-Discovery)
-
-Repositories can ship their own personas in `.review-bot/personas/`. When you specify `--persona <name>`, review-bot will:
-
-1. **Try to load from the target repo** — Checks `.review-bot/personas/<name>.yaml` (or `.yml`)
-2. **Fall back to built-in** — If not found in repo, uses the built-in persona
-
-This lets each repo define domain-specific personas without modifying CI config:
-
-```
-my-trading-repo/
-├── .review-bot/
-│   └── personas/
-│       ├── trading.yaml      # Custom trading persona
-│       └── regulatory.yaml   # Compliance-focused reviews
-├── lib/
-└── ...
-```
-
-```yaml
-# CI config (no persona-file needed)
-- uses: rodin/review-bot/.gitea/actions/review@v1
-  with:
-    reviewer-name: trading
-    persona: trading           # Will find .review-bot/personas/trading.yaml
-    ...
-```
-
-**Priority order:**
-1. Repo's `.review-bot/personas/<name>.yaml`
-2. Built-in persona with matching name
-3. Error if neither exists
-
-This allows repos to override built-in personas (e.g., a custom `security` persona that adds project-specific rules) while keeping the simple `persona: security` syntax in CI.
-
-
-### Persona vs system-prompt-file
-
-| Feature | `persona` / `persona-file` | `system-prompt-file` |
-|---------|---------------------------|----------------------|
-| Replaces base prompt | Yes | No (appends) |
-| Structured format | Yes (YAML/JSON) | No (freeform) |
-| Focus/ignore lists | Yes | Manual |
-| Severity calibration | Yes | Manual |
-| Header display name | Yes | No |
-| Built-in options | Yes | No |
-
-Use personas for domain-specialized reviews. Use `system-prompt-file` for minor tweaks to the generic review.

cmd/review-bot/main.go

@@ -69,15 +69,7 @@ func main() {
 	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")
-	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)")
+	llmProvider := flag.String("llm-provider", envOrDefault("LLM_PROVIDER", "openai"), "LLM API provider: openai or anthropic")
 
 	flag.Parse()
 
@@ -92,32 +84,12 @@ 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")
+		fmt.Fprintf(os.Stderr, "Required: --gitea-url, --repo, --pr, --reviewer-token, --llm-base-url, --llm-api-key, --llm-model\n")
 		os.Exit(1)
 	}
-	if !isAICore && (*llmBaseURL == "" || *llmAPIKey == "") {
-		fmt.Fprintf(os.Stderr, "Error: --llm-base-url and --llm-api-key are required for provider=%s\n", *llmProvider)
-		os.Exit(1)
-	}
-	if isAICore && (*aicoreClientID == "" || *aicoreClientSecret == "" || *aicoreAuthURL == "" || *aicoreAPIURL == "") {
-		fmt.Fprintf(os.Stderr, "Error: AI Core credentials required for provider=aicore\n\n")
-		fmt.Fprintf(os.Stderr, "Required: --aicore-client-id, --aicore-client-secret, --aicore-auth-url, --aicore-api-url\n")
-		os.Exit(1)
-	}
-
-	// Validate persona flags are mutually exclusive
-	if *personaName != "" && *personaFile != "" {
-		slog.Error("--persona and --persona-file are mutually exclusive")
-		os.Exit(1)
-	}
-
-	// Persona loading is deferred until after giteaClient is initialized,
-	// so we can try loading from the target repo first.
-	var persona *review.Persona
 
 	// Validate reviewer-name: only safe characters allowed in sentinel
 	if err := validateReviewerName(*reviewerName); err != nil {
@@ -153,17 +125,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 {
@@ -175,45 +138,6 @@ func main() {
 	ctx, cancel := context.WithTimeout(context.Background(), overallTimeout)
 	defer cancel()
 
-	// Load persona: try remote repo first, then fall back to built-in
-	if *personaName != "" {
-		// Try loading from target repo's .review-bot/personas/ directory
-		fetcher := &giteaFetcher{client: giteaClient}
-		remotePersonas, err := review.LoadRemotePersonas(ctx, fetcher, owner, repoName)
-		if err != nil {
-			slog.Warn("could not load remote personas", "repo", fmt.Sprintf("%s/%s", owner, repoName), "error", err)
-			// Assign empty map so the lookup below doesn't panic
-			remotePersonas = map[string]*review.Persona{}
-		}
-
-		if p, ok := remotePersonas[*personaName]; ok {
-			persona = p
-			slog.Info("loaded persona from target repo", "persona", persona.Name, "display", persona.DisplayName)
-		} else {
-			// Fall back to built-in persona
-			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)
-		}
-		loadedPersona, loadErr := review.LoadPersona(resolvedPath)
-		if loadErr != nil {
-			slog.Error("failed to load persona file", "file", *personaFile, "error", loadErr)
-			os.Exit(1)
-		}
-		persona = loadedPersona
-		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
@@ -277,14 +201,34 @@ func main() {
 	// Step 6b: Load additional system prompt if specified
 	additionalPrompt := ""
 	if *systemPromptFile != "" {
-		resolvedPath, err := validateWorkspacePath(*systemPromptFile, "system-prompt-file")
+		workspace := os.Getenv("GITHUB_WORKSPACE")
+		if workspace == "" {
+			workspace, _ = os.Getwd()
+		}
+		absWorkspace, err := filepath.Abs(workspace)
 		if err != nil {
-			slog.Error("invalid system-prompt-file path", "error", err)
+			slog.Error("failed to resolve workspace path", "error", err)
+			os.Exit(1)
+		}
+		promptPath := filepath.Join(absWorkspace, *systemPromptFile)
+		promptPath = filepath.Clean(promptPath)
+		if !strings.HasPrefix(promptPath, absWorkspace+string(filepath.Separator)) && promptPath != absWorkspace {
+			slog.Error("system-prompt-file resolves outside workspace", "path", promptPath, "workspace", absWorkspace)
+			os.Exit(1)
+		}
+		// Resolve symlinks and re-validate to prevent symlink traversal
+		resolvedPath, err := filepath.EvalSymlinks(promptPath)
+		if err != nil {
+			slog.Error("failed to resolve system prompt file", "path", promptPath, "error", err)
+			os.Exit(1)
+		}
+		if !strings.HasPrefix(resolvedPath, absWorkspace+string(filepath.Separator)) && resolvedPath != absWorkspace {
+			slog.Error("system-prompt-file symlink resolves outside workspace", "resolved", resolvedPath, "workspace", absWorkspace)
 			os.Exit(1)
 		}
 		data, err := os.ReadFile(resolvedPath)
 		if err != nil {
-			slog.Error("failed to read system prompt file", "path", *systemPromptFile, "error", err)
+			slog.Error("failed to read system prompt file", "path", promptPath, "error", err)
 			os.Exit(1)
 		}
 		additionalPrompt = string(data)
@@ -292,13 +236,7 @@ func main() {
 	}
 
 	// Step 7: Budget-aware prompt assembly
-	var systemBase string
-	if persona != nil {
-		systemBase = review.BuildPersonaSystemPrompt(persona)
-		slog.Debug("using persona system prompt", "persona", persona.Name)
-	} else {
-		systemBase = review.BuildSystemBase()
-	}
+	systemBase := review.BuildSystemBase()
 	if additionalPrompt != "" {
 		systemBase += "\n\n## Additional Review Instructions\n\n" + additionalPrompt
 	}
@@ -316,51 +254,30 @@ func main() {
 		slog.Warn("context trimmed to fit budget", "trimmed", budgetResult.Trimmed)
 	}
 
-	// Step 8: Call LLM (with retry on parse failure)
+	// Step 8: Call LLM
 	slog.Info("sending request to LLM", "model", *llmModel)
 	messages := []llm.Message{
 		{Role: "system", Content: budgetResult.SystemPrompt},
 		{Role: "user", Content: budgetResult.UserPrompt},
 	}
 
-	var response string
-	var result *review.ReviewResult
-	for attempt := 1; attempt <= 2; attempt++ {
-		if attempt > 1 {
-			slog.Warn("retrying LLM request after parse failure", "attempt", attempt)
-			time.Sleep(time.Second)
-		}
+	response, err := llmClient.Complete(ctx, messages)
+	if err != nil {
+		slog.Error("LLM request failed", "model", *llmModel, "error", err)
+		os.Exit(1)
+	}
+	slog.Info("LLM response received", "bytes", len(response))
 
-		response, err = llmClient.Complete(ctx, messages)
-		if err != nil {
-			slog.Error("LLM request failed", "model", *llmModel, "error", err, "attempt", attempt)
-			if attempt == 2 {
-				os.Exit(1)
-			}
-			continue
-		}
-		slog.Info("LLM response received", "bytes", len(response), "attempt", attempt)
-
-		// Step 9: Parse response
-		result, err = review.ParseResponse(response)
-		if err != nil {
-			slog.Error("failed to parse LLM response", "error", err, "attempt", attempt)
-			if attempt == 2 {
-				os.Exit(1)
-			}
-			continue
-		}
-		break
+	// Step 9: Parse response
+	result, err := review.ParseResponse(response)
+	if err != nil {
+		slog.Error("failed to parse LLM response", "error", err)
+		os.Exit(1)
 	}
 	slog.Info("review parsed", "verdict", result.Verdict, "findings", len(result.Findings))
 
 	// Step 10: Format and post review
-	var reviewBody string
-	if persona != nil && persona.DisplayName != "" {
-		reviewBody = review.FormatMarkdownWithDisplay(result, persona.DisplayName, *reviewerName)
-	} else {
-		reviewBody = review.FormatMarkdown(result, *reviewerName)
-	}
+	reviewBody := review.FormatMarkdown(result, *reviewerName)
 
 	// Add commit footer so readers know which commit was evaluated
 	if pr.Head.Sha != "" {
@@ -382,24 +299,6 @@ func main() {
 
 	sentinel := fmt.Sprintf("<!-- review-bot:%s -->", *reviewerName)
 
-	// Stale check: verify HEAD hasn't moved since we started
-	evaluatedSHA := pr.Head.Sha
-	var currentSHA string
-	currentPR, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber)
-	if err != nil {
-		slog.Warn("could not re-fetch PR for stale check", "pr", prNumber, "error", err)
-		// currentSHA stays empty — shouldSkipStaleReview will return false
-	} else {
-		currentSHA = currentPR.Head.Sha
-	}
-	if shouldSkipStaleReview(evaluatedSHA, currentSHA) {
-		slog.Warn("HEAD moved during review — skipping stale review",
-			"evaluated", evaluatedSHA,
-			"current", currentSHA,
-			"pr", prNumber)
-		return
-	}
-
 	// Map findings to inline comments for lines present in the diff
 	diffRanges := gitea.ParseDiffNewLines(diff)
 	var inlineComments []gitea.ReviewComment
@@ -654,43 +553,6 @@ func validateReviewerName(name string) error {
 	return nil
 }
 
-// validateWorkspacePath ensures a file path is within the workspace and resolves
-// symlinks to prevent traversal attacks. Returns the resolved absolute path or
-// an error if the path is outside the workspace.
-func validateWorkspacePath(path, pathName string) (string, error) {
-	workspace := os.Getenv("GITHUB_WORKSPACE")
-	if workspace == "" {
-		workspace, _ = os.Getwd()
-	}
-	absWorkspace, err := filepath.Abs(workspace)
-	if err != nil {
-		return "", fmt.Errorf("failed to resolve workspace path: %w", err)
-	}
-
-	// Join and clean the path
-	fullPath := filepath.Join(absWorkspace, path)
-	fullPath = filepath.Clean(fullPath)
-
-	// Check path is within workspace using filepath.Rel (more robust than HasPrefix)
-	rel, err := filepath.Rel(absWorkspace, fullPath)
-	if err != nil || strings.HasPrefix(rel, "..") {
-		return "", fmt.Errorf("%s resolves outside workspace: path=%s workspace=%s", pathName, fullPath, absWorkspace)
-	}
-
-	// Resolve symlinks and re-validate to prevent symlink traversal
-	resolvedPath, err := filepath.EvalSymlinks(fullPath)
-	if err != nil {
-		return "", fmt.Errorf("failed to resolve %s: %w", pathName, err)
-	}
-
-	relResolved, err := filepath.Rel(absWorkspace, resolvedPath)
-	if err != nil || strings.HasPrefix(relResolved, "..") {
-		return "", fmt.Errorf("%s symlink resolves outside workspace: resolved=%s workspace=%s", pathName, resolvedPath, absWorkspace)
-	}
-
-	return resolvedPath, nil
-}
-
 // buildSupersededBody creates the body for a superseded review: struck-through banner
 // with collapsed original content and the commit it was evaluated against.
 func buildSupersededBody(originalBody, commitSHA, newReviewURL, sentinel string) string {
@@ -788,42 +650,3 @@ func findAllOwnReviews(reviews []gitea.Review, sentinel string) []gitea.Review {
 	}
 	return result
 }
-
-// shouldSkipStaleReview reports whether to skip posting because HEAD moved.
-// Returns true (skip) if evaluatedSHA differs from currentSHA.
-// Returns false (don't skip) if:
-//   - SHAs match (no movement)
-//   - currentSHA is empty (re-fetch failed; prefer posting stale over failing)
-func shouldSkipStaleReview(evaluatedSHA, currentSHA string) bool {
-	if currentSHA == "" {
-		// Re-fetch failed; better to post potentially stale than fail
-		return false
-	}
-	return evaluatedSHA != currentSHA
-}
-
-// giteaFetcher adapts gitea.Client to review.PersonaFetcher interface.
-type giteaFetcher struct {
-	client *gitea.Client
-}
-
-func (f *giteaFetcher) ListContents(ctx context.Context, owner, repo, path string) ([]review.ContentEntry, error) {
-	entries, err := f.client.ListContents(ctx, owner, repo, path)
-	if err != nil {
-		return nil, err
-	}
-	// Convert gitea.ContentEntry to review.ContentEntry
-	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 (f *giteaFetcher) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
-	return f.client.GetFileContent(ctx, owner, repo, filepath)
-}

cmd/review-bot/main_test.go

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

docs/DESIGN-51-personas.md

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

docs/DESIGN-57-yaml-persona.md

@@ -1,108 +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 `gopkg.in/yaml.v3` (approved in CONVENTIONS.md) with explicit depth limiting
-
-## 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
-
-```go
-func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
-    var node yaml.Node
-    dec := yaml.NewDecoder(bytes.NewReader(data))
-    if err := dec.Decode(&node); err != nil {
-        return err
-    }
-    if err := checkYAMLDepth(&node, 0, maxDepth); err != nil {
-        return err
-    }
-    return node.Decode(out)
-}
-
-func checkYAMLDepth(node *yaml.Node, depth, maxDepth int) error {
-    if depth > maxDepth {
-        return fmt.Errorf("YAML nesting depth exceeds maximum (%d)", maxDepth)
-    }
-    // Handle alias nodes by following the Alias pointer
-    if node.Kind == yaml.AliasNode && node.Alias != nil {
-        return checkYAMLDepth(node.Alias, depth, maxDepth)
-    }
-    for _, child := range node.Content {
-        if err := checkYAMLDepth(child, depth+1, maxDepth); err != nil {
-            return err
-        }
-    }
-    return nil
-}
-```
-
-The `gopkg.in/yaml.v3` library does not have built-in depth protection, so we implement explicit depth checking by first decoding into a `yaml.Node`, walking the tree to verify depth (including alias resolution), then decoding into the target struct.
-
-## State/Data Model
-
-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 | Library rejects (v1.16.0+ fix) |
-| 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.

go.mod

@@ -1,5 +1,3 @@
 module gitea.weiker.me/rodin/review-bot
 
 go 1.26.2
-
-require gopkg.in/yaml.v3 v3.0.1

go.sum

@@ -1,4 +0,0 @@
-gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
-gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
-gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
-gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=

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"`
@@ -91,66 +75,12 @@ type Message struct {
 // Complete sends a chat completion request and returns the assistant's response content.
 // The first message with role "system" is treated as the system prompt.
 func (c *Client) Complete(ctx context.Context, messages []Message) (string, error) {
-	var result string
-	var err error
-
-	for attempt := 0; attempt < 2; attempt++ {
-		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)
-		}
-
-		if err == nil {
-			return result, nil
-		}
-
-		// Only retry on response body read errors (transient network issues).
-		// Do not retry on context cancellation, status errors, or parse errors
-		// that indicate a structural API problem.
-		if !isRetryableError(err) {
-			return "", err
-		}
-
-		if attempt == 0 && ctx.Err() == nil {
-			// Brief pause before retry to allow transient issues to resolve.
-			time.Sleep(500 * time.Millisecond)
-		}
+	switch c.provider {
+	case ProviderAnthropic:
+		return c.completeAnthropic(ctx, messages)
+	default:
+		return c.completeOpenAI(ctx, messages)
 	}
-
-	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 {
-		return false
-	}
-	s := err.Error()
-	// Body read failures (connection reset, truncation)
-	if strings.Contains(s, "read response") {
-		return true
-	}
-	// Unexpected body length (our content-length validation)
-	if strings.Contains(s, "body length mismatch") {
-		return true
-	}
-	return false
 }
 
 // --- OpenAI-compatible implementation ---
@@ -206,12 +136,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 {
@@ -302,12 +231,6 @@ func (c *Client) doRequest(req *http.Request, parse func([]byte) (string, error)
 		return "", fmt.Errorf("read response: %w", err)
 	}
 
-	// Validate body length against Content-Length header when present.
-	// A mismatch indicates the response was truncated in transit.
-	if cl := resp.ContentLength; cl > 0 && int64(len(body)) < cl {
-		return "", fmt.Errorf("body length mismatch: Content-Length=%d, received=%d", cl, len(body))
-	}
-
 	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
 		return "", fmt.Errorf("LLM API error (status %d): %s", resp.StatusCode, string(body))
 	}

llm/client_test.go

@@ -3,7 +3,6 @@ package llm
 import (
 	"context"
 	"encoding/json"
-	"fmt"
 	"net/http"
 	"net/http/httptest"
 	"testing"
@@ -296,131 +295,3 @@ func TestWithProvider(t *testing.T) {
 		t.Errorf("expected provider anthropic, got %s", client.provider)
 	}
 }
-
-func TestComplete_RetryOnBodyReadError(t *testing.T) {
-	attempts := 0
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		if attempts == 1 {
-			// First attempt: send headers then close connection abruptly
-			// Simulate by writing partial response and flushing with wrong Content-Length
-			w.Header().Set("Content-Length", "1000")
-			w.WriteHeader(http.StatusOK)
-			w.Write([]byte(`{"choices":[{"message":{"con`))
-			// The test HTTP server will close the connection after handler returns,
-			// but Content-Length mismatch means client gets fewer bytes than expected
-			return
-		}
-		// Second attempt: succeed
-		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: "success"}}},
-		})
-	}))
-	defer server.Close()
-
-	client := NewClient(server.URL, "key", "model")
-	got, err := client.Complete(context.Background(), []Message{{Role: "user", Content: "Hi"}})
-	if err != nil {
-		t.Fatalf("expected retry to succeed, got error: %v", err)
-	}
-	if got != "success" {
-		t.Errorf("expected %q, got %q", "success", got)
-	}
-	if attempts != 2 {
-		t.Errorf("expected 2 attempts, got %d", attempts)
-	}
-}
-
-func TestComplete_ContentLengthMismatch(t *testing.T) {
-	attempts := 0
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		if attempts == 1 {
-			// Claim Content-Length is larger than actual body
-			w.Header().Set("Content-Length", "500")
-			w.Header().Set("Content-Type", "application/json")
-			w.WriteHeader(http.StatusOK)
-			// Write less than 500 bytes
-			w.Write([]byte(`{"choices":[{"message":{"content":"partial"}}]}`))
-			return
-		}
-		// Second attempt succeeds
-		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: "complete"}}},
-		})
-	}))
-	defer server.Close()
-
-	client := NewClient(server.URL, "key", "model")
-	got, err := client.Complete(context.Background(), []Message{{Role: "user", Content: "Hi"}})
-	if err != nil {
-		t.Fatalf("expected retry to succeed on content-length mismatch, got: %v", err)
-	}
-	if got != "complete" {
-		t.Errorf("expected %q, got %q", "complete", got)
-	}
-}
-
-func TestComplete_NoRetryOnAPIError(t *testing.T) {
-	attempts := 0
-	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		attempts++
-		w.WriteHeader(http.StatusBadRequest)
-		w.Write([]byte(`{"error":"bad request"}`))
-	}))
-	defer server.Close()
-
-	client := NewClient(server.URL, "key", "model")
-	_, err := client.Complete(context.Background(), []Message{{Role: "user", Content: "Hi"}})
-	if err == nil {
-		t.Fatal("expected error for 400, got nil")
-	}
-	if attempts != 1 {
-		t.Errorf("should not retry on API errors, got %d attempts", attempts)
-	}
-}
-
-func TestIsRetryableError(t *testing.T) {
-	tests := []struct {
-		name     string
-		err      string
-		expected bool
-	}{
-		{"nil formatted", "", false},
-		{"read response error", "read response: unexpected EOF", true},
-		{"body length mismatch", "body length mismatch: Content-Length=1000, received=500", true},
-		{"API error", "LLM API error (status 400): bad request", false},
-		{"parse error", "parse response: unexpected end of JSON input", false},
-		{"request error", "LLM request: connection refused", false},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			if tt.err == "" {
-				if isRetryableError(nil) {
-					t.Error("nil error should not be retryable")
-				}
-				return
-			}
-			err := fmt.Errorf("%s", tt.err)
-			got := isRetryableError(err)
-			if got != tt.expected {
-				t.Errorf("isRetryableError(%q) = %v, want %v", tt.err, got, tt.expected)
-			}
-		})
-	}
-}

review/formatter.go

@@ -2,63 +2,15 @@ package review
 
 import (
 	"fmt"
-	"regexp"
 	"strings"
 )
 
 // FormatMarkdown formats a ReviewResult into the markdown body for a Gitea review.
 func FormatMarkdown(result *ReviewResult, reviewerName string) string {
-	return FormatMarkdownWithDisplay(result, reviewerName, reviewerName)
-}
-
-// GiteaEvent converts the verdict to the Gitea API event string.
-func GiteaEvent(verdict string) string {
-	switch verdict {
-	case "APPROVE":
-		return "APPROVED"
-	case "REQUEST_CHANGES":
-		return "REQUEST_CHANGES"
-	default:
-		return "COMMENT"
-	}
-}
-
-// markdownSpecialChars matches characters that have special meaning in Markdown.
-// We escape these to prevent untrusted input from breaking formatting.
-// Uses a quoted string since raw strings can't contain backticks.
-var markdownSpecialChars = regexp.MustCompile("([\\\\*_`\\[\\]()#<>|~])")
-
-// sanitizeMarkdownText escapes special Markdown characters in untrusted text.
-// This prevents markdown injection attacks where a malicious display name could
-// break formatting, inject links, or create unexpected rendering.
-func sanitizeMarkdownText(s string) string {
-	// First, remove any control characters and null bytes
-	cleaned := strings.Map(func(r rune) rune {
-		if r < 32 && r != '\t' && r != '\n' {
-			return -1 // drop the character
-		}
-		return r
-	}, s)
-	// Escape special Markdown characters by prepending backslash
-	return markdownSpecialChars.ReplaceAllString(cleaned, `\$1`)
-}
-
-// FormatMarkdownWithDisplay formats a ReviewResult with separate display name and sentinel name.
-// displayName is sanitized to prevent Markdown injection from untrusted remote persona metadata.
-// sentinelName is used for the cleanup sentinel comment (machine-readable, not rendered).
-// If displayName is empty, sentinelName is used for both.
-func FormatMarkdownWithDisplay(result *ReviewResult, displayName, sentinelName string) string {
 	var sb strings.Builder
 
-	// Use display name for header, or fall back to sentinel name
-	headerName := displayName
-	if headerName == "" {
-		headerName = sentinelName
-	}
-
-	if headerName != "" {
-		// Sanitize the header name to prevent Markdown injection
-		title := CapitalizeFirst(sanitizeMarkdownText(headerName))
+	if reviewerName != "" {
+		title := strings.ToUpper(reviewerName[:1]) + reviewerName[1:]
 		sb.WriteString(fmt.Sprintf("# %s Review\n\n", title))
 	}
 
@@ -81,12 +33,23 @@ func FormatMarkdownWithDisplay(result *ReviewResult, displayName, sentinelName s
 	sb.WriteString("## Recommendation\n\n")
 	sb.WriteString(fmt.Sprintf("**%s** — %s\n", result.Verdict, result.Recommendation))
 
-	if sentinelName != "" {
-		// Sanitize headerName for the footer as well
-		sb.WriteString(fmt.Sprintf("\n---\n*Review by %s*\n", sanitizeMarkdownText(headerName)))
+	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", sentinelName))
+		sb.WriteString(fmt.Sprintf("\n<!-- review-bot:%s -->\n", reviewerName))
 	}
 
 	return sb.String()
 }
+
+// GiteaEvent converts the verdict to the Gitea API event string.
+func GiteaEvent(verdict string) string {
+	switch verdict {
+	case "APPROVE":
+		return "APPROVED"
+	case "REQUEST_CHANGES":
+		return "REQUEST_CHANGES"
+	default:
+		return "COMMENT"
+	}
+}

review/formatter_test.go

@@ -159,126 +159,3 @@ func TestFormatMarkdown_RoleTitle(t *testing.T) {
 		t.Error("should not contain role title header when reviewer name is empty")
 	}
 }
-
-func TestFormatMarkdownWithDisplay(t *testing.T) {
-	result := &ReviewResult{
-		Verdict:        "APPROVE",
-		Summary:        "Test summary",
-		Findings:       nil,
-		Recommendation: "Test recommendation",
-	}
-
-	t.Run("with display name", func(t *testing.T) {
-		body := FormatMarkdownWithDisplay(result, "Security Specialist", "security")
-
-		// Header should use display name
-		if !strings.Contains(body, "# Security Specialist Review") {
-			t.Error("header should use display name")
-		}
-
-		// Sentinel should use sentinel name
-		if !strings.Contains(body, "<!-- review-bot:security -->") {
-			t.Error("sentinel should use sentinel name")
-		}
-
-		// Footer "Review by" should use display name
-		if !strings.Contains(body, "*Review by Security Specialist*") {
-			t.Error("footer should use display name")
-		}
-	})
-
-	t.Run("without display name", func(t *testing.T) {
-		body := FormatMarkdownWithDisplay(result, "", "reviewer")
-
-		// Should fall back to sentinel name for header
-		if !strings.Contains(body, "# Reviewer Review") {
-			t.Error("header should fall back to sentinel name")
-		}
-
-		if !strings.Contains(body, "<!-- review-bot:reviewer -->") {
-			t.Error("sentinel should use sentinel name")
-		}
-	})
-
-	t.Run("empty both names", func(t *testing.T) {
-		body := FormatMarkdownWithDisplay(result, "", "")
-
-		// Should not have header
-		if strings.Contains(body, "# ") && strings.Contains(body, " Review") {
-			t.Error("should not have header when both names empty")
-		}
-
-		// Should not have sentinel
-		if strings.Contains(body, "<!-- review-bot:") {
-			t.Error("should not have sentinel when sentinel name empty")
-		}
-	})
-}
-
-func TestSanitizeMarkdownText(t *testing.T) {
-	tests := []struct {
-		name  string
-		input string
-		want  string
-	}{
-		{
-			name:  "plain text unchanged",
-			input: "Security Specialist",
-			want:  "Security Specialist",
-		},
-		{
-			name:  "escapes asterisks",
-			input: "**bold** attack",
-			want:  `\*\*bold\*\* attack`,
-		},
-		{
-			name:  "escapes brackets for links",
-			input: "[click me](http://evil.com)",
-			want:  `\[click me\]\(http://evil.com\)`,
-		},
-		{
-			name:  "escapes backticks",
-			input: "`code` injection",
-			want:  "\\`code\\` injection",
-		},
-		{
-			name:  "escapes angle brackets",
-			input: "<script>alert(1)</script>",
-			want:  `\<script\>alert\(1\)\</script\>`,
-		},
-		{
-			name:  "escapes hash for headers",
-			input: "# Fake Header",
-			want:  `\# Fake Header`,
-		},
-		{
-			name:  "escapes pipe for tables",
-			input: "col1 | col2",
-			want:  `col1 \| col2`,
-		},
-		{
-			name:  "removes control characters",
-			input: "hello\x00world\x1f",
-			want:  "helloworld",
-		},
-		{
-			name:  "preserves tabs and newlines",
-			input: "line1\n\tindented",
-			want:  "line1\n\tindented",
-		},
-		{
-			name:  "escapes tilde for strikethrough",
-			input: "~~strikethrough~~",
-			want:  `\~\~strikethrough\~\~`,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got := sanitizeMarkdownText(tt.input)
-			if got != tt.want {
-				t.Errorf("sanitizeMarkdownText(%q) = %q, want %q", tt.input, got, tt.want)
-			}
-		})
-	}
-}

review/parser.go

@@ -33,14 +33,7 @@ func ParseResponse(response string) (*ReviewResult, error) {
 		// Try to repair before giving up.
 		repaired := repairJSON(cleaned)
 		if err2 := json.Unmarshal([]byte(repaired), &result); err2 != nil {
-			// Include diagnostic info: lengths help identify truncation
-			rawLen := len(response)
-			cleanedLen := len(cleaned)
-			preview := cleaned
-			if len(preview) > 200 {
-				preview = preview[:100] + "..." + preview[len(preview)-100:]
-			}
-			return nil, fmt.Errorf("parse LLM response as JSON: %w\nRaw length: %d, cleaned length: %d\nCleaned preview: %s", err, rawLen, cleanedLen, preview)
+			return nil, fmt.Errorf("parse LLM response as JSON: %w\nRaw response: %s", err, response)
 		}
 	}
 

review/persona.go

@@ -1,252 +0,0 @@
-package review
-
-import (
-	"bytes"
-	"embed"
-	"encoding/json"
-	"fmt"
-	"os"
-	"sort"
-	"strings"
-	"unicode/utf8"
-
-	"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"`
-	DisplayName  string   `json:"display_name" yaml:"display_name"`
-	ModelPref    string   `json:"model_preference,omitempty" yaml:"model_preference,omitempty"`
-	Identity     string   `json:"identity" yaml:"identity"`
-	Focus        []string `json:"focus" yaml:"focus"`
-	Ignore       []string `json:"ignore" yaml:"ignore"`
-	Severity     Severity `json:"severity" yaml:"severity"`
-	OutputFormat string   `json:"output_format,omitempty" yaml:"output_format,omitempty"`
-}
-
-// Severity defines what constitutes each severity level for this persona.
-// These are prompt guidance for the LLM, not output format changes.
-type Severity struct {
-	Major string `json:"major" yaml:"major"`
-	Minor string `json:"minor" yaml:"minor"`
-	Nit   string `json:"nit" yaml:"nit"`
-}
-
-// LoadPersona loads a persona from a JSON or YAML file path.
-// Format is detected by file extension: .yaml/.yml for YAML, .json or other for JSON.
-// Files larger than MaxPersonaFileSize are rejected.
-//
-// Symlinks are supported: os.Stat follows symlinks, so a symlink pointing to
-// a regular file will pass the IsRegular() check. Symlinks to non-regular files
-// (directories, FIFOs, devices) are still rejected.
-func LoadPersona(path string) (*Persona, error) {
-	// os.Stat follows symlinks, so symlinks to regular files are supported.
-	// The IsRegular() check operates on the target, not the symlink itself.
-	info, err := os.Stat(path)
-	if err != nil {
-		return nil, fmt.Errorf("read persona file %s: %w", path, err)
-	}
-	if !info.Mode().IsRegular() {
-		return nil, fmt.Errorf("persona file %s is not a regular file", path)
-	}
-	if info.Size() > MaxPersonaFileSize {
-		return nil, fmt.Errorf("persona file %s exceeds maximum size (%d bytes)", path, MaxPersonaFileSize)
-	}
-	data, err := os.ReadFile(path)
-	if err != nil {
-		return nil, fmt.Errorf("read persona file %s: %w", path, err)
-	}
-	// Re-check size after read to defend against TOCTOU races where file
-	// grows between stat and read (e.g., appending process, replaced file).
-	if len(data) > MaxPersonaFileSize {
-		return nil, fmt.Errorf("persona file %s exceeds maximum size (%d bytes)", path, MaxPersonaFileSize)
-	}
-	return parsePersona(data, path)
-}
-
-// LoadBuiltinPersona loads a built-in persona by name.
-// Returns an error if the persona doesn't exist.
-// Built-in personas are stored in YAML format only (see embed directive).
-func LoadBuiltinPersona(name string) (*Persona, error) {
-	yamlFile := name + ".yaml"
-	data, err := embeddedPersonas.ReadFile("personas/" + yamlFile)
-	if err != nil {
-		available := ListBuiltinPersonas()
-		return nil, fmt.Errorf("unknown built-in persona %q (available: %s)", name, strings.Join(available, ", "))
-	}
-	return parsePersona(data, "builtin:"+yamlFile)
-}
-
-// ListBuiltinPersonas returns the names of all built-in personas in sorted order.
-// Returns an empty slice if the embedded directory cannot be read.
-func ListBuiltinPersonas() []string {
-	entries, err := embeddedPersonas.ReadDir("personas")
-	if err != nil {
-		return []string{}
-	}
-	seen := make(map[string]bool)
-	for _, e := range entries {
-		if e.IsDir() {
-			continue
-		}
-		name := e.Name()
-		// Strip extension to get persona name
-		var personaName string
-		switch {
-		case strings.HasSuffix(name, ".yaml"):
-			personaName = strings.TrimSuffix(name, ".yaml")
-		case strings.HasSuffix(name, ".yml"):
-			personaName = strings.TrimSuffix(name, ".yml")
-		case strings.HasSuffix(name, ".json"):
-			personaName = strings.TrimSuffix(name, ".json")
-		default:
-			continue
-		}
-		if !seen[personaName] {
-			seen[personaName] = true
-		}
-	}
-	names := make([]string, 0, len(seen))
-	for name := range seen {
-		names = append(names, name)
-	}
-	sort.Strings(names)
-	return names
-}
-
-// parsePersona parses persona data from JSON or YAML format.
-// Format is detected by the source file extension.
-func parsePersona(data []byte, source string) (*Persona, error) {
-	lowerSource := strings.ToLower(source)
-	isYAML := strings.HasSuffix(lowerSource, ".yaml") || strings.HasSuffix(lowerSource, ".yml")
-
-	var p Persona
-	var err error
-	if isYAML {
-		err = unmarshalYAMLWithDepthLimit(data, &p, MaxYAMLDepth)
-	} else {
-		// Use json.Decoder with DisallowUnknownFields for consistency with
-		// YAML's KnownFields(true) - both reject unknown fields to catch typos.
-		dec := json.NewDecoder(bytes.NewReader(data))
-		dec.DisallowUnknownFields()
-		err = dec.Decode(&p)
-	}
-	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 explicit depth limiting
-// and strict field checking. This protects against stack exhaustion from deeply
-// nested structures and catches typos in field names.
-// Multi-document YAML files are rejected to prevent silent data loss.
-func unmarshalYAMLWithDepthLimit(data []byte, out any, maxDepth int) error {
-	// First pass: decode into a yaml.Node to check depth limits and node counts.
-	// This prevents stack exhaustion before we attempt to decode into structs.
-	var node yaml.Node
-	dec := yaml.NewDecoder(bytes.NewReader(data))
-	if err := dec.Decode(&node); err != nil {
-		return err
-	}
-
-	// Reject multi-document YAML files - silently ignoring additional documents
-	// could lead to confusing behavior where users think their changes take effect.
-	var extra yaml.Node
-	if dec.Decode(&extra) == nil {
-		return fmt.Errorf("multi-document YAML is not supported; only single-document files are allowed")
-	}
-
-	nodeCount := 0
-	if err := checkYAMLDepth(&node, 0, maxDepth, MaxYAMLNodes, make(map[*yaml.Node]struct{}), &nodeCount); err != nil {
-		return err
-	}
-
-	// Second pass: decode with strict field checking enabled.
-	// KnownFields(true) rejects unknown keys, catching typos like "focuss" or "identiy".
-	// We must re-decode from the original data because yaml.Node.Decode() doesn't
-	// support the KnownFields option.
-	strictDec := yaml.NewDecoder(bytes.NewReader(data))
-	strictDec.KnownFields(true)
-	return strictDec.Decode(out)
-}
-
-// checkYAMLDepth recursively checks that YAML nodes don't exceed the depth limit
-// or the total node count limit. It also detects alias cycles to prevent infinite
-// recursion from crafted YAML with self-referential aliases.
-func checkYAMLDepth(node *yaml.Node, depth, maxDepth, maxNodes int, seen map[*yaml.Node]struct{}, nodeCount *int) error {
-	if depth > maxDepth {
-		return fmt.Errorf("YAML nesting depth exceeds maximum (%d)", maxDepth)
-	}
-
-	// Track total nodes visited as defense-in-depth against wide-but-shallow attacks.
-	*nodeCount++
-	if *nodeCount > maxNodes {
-		return fmt.Errorf("YAML node count exceeds maximum (%d)", maxNodes)
-	}
-
-	// Cycle detection: if we've seen this node before, we're in a cycle.
-	if _, ok := seen[node]; ok {
-		return nil // Already validated this subtree, skip to avoid infinite recursion.
-	}
-	seen[node] = struct{}{}
-
-	// Handle alias nodes: follow the alias to its anchor target.
-	// Increment depth when following aliases since they expand the effective structure.
-	if node.Kind == yaml.AliasNode && node.Alias != nil {
-		return checkYAMLDepth(node.Alias, depth+1, maxDepth, maxNodes, seen, nodeCount)
-	}
-
-	for _, child := range node.Content {
-		if err := checkYAMLDepth(child, depth+1, maxDepth, maxNodes, seen, nodeCount); err != nil {
-			return err
-		}
-	}
-	return nil
-}
-
-func validatePersona(p *Persona, source string) error {
-	if p.Name == "" {
-		return fmt.Errorf("persona %s: name is required", source)
-	}
-	if p.Identity == "" {
-		return fmt.Errorf("persona %s: identity is required", source)
-	}
-	// DisplayName defaults to Name if not set
-	if p.DisplayName == "" {
-		p.DisplayName = p.Name
-	}
-	return nil
-}
-
-// CapitalizeFirst capitalizes the first rune of a string in a Unicode-safe way.
-// Returns the original string if it's empty.
-func CapitalizeFirst(s string) string {
-	if s == "" {
-		return s
-	}
-	r, size := utf8.DecodeRuneInString(s)
-	if r == utf8.RuneError {
-		return s
-	}
-	return strings.ToUpper(string(r)) + s[size:]
-}

review/persona_prompt.go

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

review/persona_prompt_test.go

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

review/persona_test.go

@@ -1,778 +0,0 @@
-package review
-
-import (
-	"fmt"
-	"os"
-	"path/filepath"
-	"strings"
-	"testing"
-
-	"gopkg.in/yaml.v3"
-)
-
-func TestLoadBuiltinPersona(t *testing.T) {
-	tests := []struct {
-		name        string
-		personaName string
-		wantErr     bool
-		wantDisplay string
-	}{
-		{
-			name:        "security persona",
-			personaName: "security",
-			wantErr:     false,
-			wantDisplay: "Security Specialist",
-		},
-		{
-			name:        "architect persona",
-			personaName: "architect",
-			wantErr:     false,
-			wantDisplay: "Software Architect",
-		},
-		{
-			name:        "docs persona",
-			personaName: "docs",
-			wantErr:     false,
-			wantDisplay: "Documentation Reviewer",
-		},
-		{
-			name:        "unknown persona",
-			personaName: "nonexistent",
-			wantErr:     true,
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			p, err := LoadBuiltinPersona(tt.personaName)
-			if tt.wantErr {
-				if err == nil {
-					t.Error("expected error, got nil")
-				}
-				return
-			}
-			if err != nil {
-				t.Fatalf("unexpected error: %v", err)
-			}
-			if p.Name != tt.personaName {
-				t.Errorf("Name = %q, want %q", p.Name, tt.personaName)
-			}
-			if p.DisplayName != tt.wantDisplay {
-				t.Errorf("DisplayName = %q, want %q", p.DisplayName, tt.wantDisplay)
-			}
-			if p.Identity == "" {
-				t.Error("Identity should not be empty")
-			}
-			if len(p.Focus) == 0 {
-				t.Error("Focus should not be empty")
-			}
-		})
-	}
-}
-
-func TestListBuiltinPersonas(t *testing.T) {
-	names := ListBuiltinPersonas()
-	if len(names) == 0 {
-		t.Fatal("expected at least one built-in persona")
-	}
-
-	// Check for expected personas
-	expected := map[string]bool{"security": false, "architect": false, "docs": false}
-	for _, name := range names {
-		if _, ok := expected[name]; ok {
-			expected[name] = true
-		}
-	}
-	for name, found := range expected {
-		if !found {
-			t.Errorf("expected built-in persona %q not found", name)
-		}
-	}
-}
-
-func TestLoadPersonaFromYAMLFile(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "test.yaml")
-
-	content := `# Test persona
-name: test
-display_name: Test Persona
-identity: |
-  You are a test persona.
-  Multi-line identity works.
-focus:
-  - testing
-  - validation
-ignore:
-  - nothing
-severity:
-  major: Big problems
-  minor: Small problems
-  nit: Tiny problems
-`
-
-	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	p, err := LoadPersona(path)
-	if err != nil {
-		t.Fatalf("LoadPersona failed: %v", err)
-	}
-
-	if p.Name != "test" {
-		t.Errorf("Name = %q, want %q", p.Name, "test")
-	}
-	if p.DisplayName != "Test Persona" {
-		t.Errorf("DisplayName = %q, want %q", p.DisplayName, "Test Persona")
-	}
-	if len(p.Focus) != 2 {
-		t.Errorf("Focus len = %d, want 2", len(p.Focus))
-	}
-	if !strings.Contains(p.Identity, "Multi-line") {
-		t.Error("Identity should contain multi-line content")
-	}
-}
-
-func TestLoadPersonaFromYMLFile(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "test.yml")
-
-	content := `name: test
-display_name: Test YML
-identity: Test identity
-focus:
-  - testing
-ignore: []
-severity:
-  major: Big
-  minor: Small
-  nit: Tiny
-`
-
-	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	p, err := LoadPersona(path)
-	if err != nil {
-		t.Fatalf("LoadPersona failed: %v", err)
-	}
-
-	if p.Name != "test" {
-		t.Errorf("Name = %q, want %q", p.Name, "test")
-	}
-	if p.DisplayName != "Test YML" {
-		t.Errorf("DisplayName = %q, want %q", p.DisplayName, "Test YML")
-	}
-}
-
-func TestLoadPersonaFromJSONFile(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "test.json")
-
-	content := `{
-		"name": "test",
-		"display_name": "Test Persona",
-		"identity": "You are a test persona.\nMulti-line identity works.",
-		"focus": ["testing", "validation"],
-
-		"ignore": ["nothing"],
-		"severity": {
-			"major": "Big problems",
-			"minor": "Small problems",
-			"nit": "Tiny problems"
-		}
-	}`
-
-	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	p, err := LoadPersona(path)
-	if err != nil {
-		t.Fatalf("LoadPersona failed: %v", err)
-	}
-
-	if p.Name != "test" {
-		t.Errorf("Name = %q, want %q", p.Name, "test")
-	}
-	if p.DisplayName != "Test Persona" {
-		t.Errorf("DisplayName = %q, want %q", p.DisplayName, "Test Persona")
-	}
-	if len(p.Focus) != 2 {
-		t.Errorf("Focus len = %d, want 2", len(p.Focus))
-	}
-	if !strings.Contains(p.Identity, "Multi-line") {
-		t.Error("Identity should contain multi-line content")
-	}
-}
-
-func TestLoadPersonaValidation(t *testing.T) {
-	tests := []struct {
-		name    string
-		content string
-		ext     string
-		wantErr string
-	}{
-		{
-			name:    "missing name yaml",
-			content: "identity: test\n",
-			ext:     ".yaml",
-			wantErr: "name is required",
-		},
-		{
-			name:    "missing identity yaml",
-			content: "name: test\n",
-			ext:     ".yaml",
-			wantErr: "identity is required",
-		},
-		{
-			name:    "missing name json",
-			content: `{"identity": "test"}`,
-			ext:     ".json",
-			wantErr: "name is required",
-		},
-		{
-			name:    "missing identity json",
-			content: `{"name": "test"}`,
-			ext:     ".json",
-			wantErr: "identity is required",
-		},
-		{
-			name:    "display_name defaults to name",
-			content: "name: test\nidentity: test identity\n",
-			ext:     ".yaml",
-			// No error expected - should succeed
-		},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			dir := t.TempDir()
-			path := filepath.Join(dir, "test"+tt.ext)
-			if err := os.WriteFile(path, []byte(tt.content), 0644); err != nil {
-				t.Fatalf("failed to write test file: %v", err)
-			}
-
-			p, err := LoadPersona(path)
-			if tt.wantErr != "" {
-				if err == nil {
-					t.Errorf("expected error containing %q, got nil", tt.wantErr)
-					return
-				}
-				if !strings.Contains(err.Error(), tt.wantErr) {
-					t.Errorf("error = %q, want containing %q", err.Error(), tt.wantErr)
-				}
-				return
-			}
-			if err != nil {
-				t.Fatalf("unexpected error: %v", err)
-			}
-			// Check display_name defaulting
-			if p.DisplayName == "" {
-				t.Error("DisplayName should default to Name")
-			}
-			if p.DisplayName != p.Name {
-				t.Errorf("DisplayName should default to Name, got %q", p.DisplayName)
-			}
-		})
-	}
-}
-
-func TestLoadPersonaFileNotFound(t *testing.T) {
-	_, err := LoadPersona("/nonexistent/path/persona.yaml")
-	if err == nil {
-		t.Error("expected error for nonexistent file")
-	}
-}
-
-func TestLoadPersonaInvalidYAML(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "invalid.yaml")
-	if err := os.WriteFile(path, []byte("not valid yaml:\n  - [broken"), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	_, err := LoadPersona(path)
-	if err == nil {
-		t.Error("expected error for invalid YAML")
-	}
-}
-
-func TestLoadPersonaInvalidJSON(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "invalid.json")
-	if err := os.WriteFile(path, []byte("not valid json {"), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	_, err := LoadPersona(path)
-	if err == nil {
-		t.Error("expected error for invalid JSON")
-	}
-}
-
-func TestLoadPersonaCaseInsensitiveExtension(t *testing.T) {
-	tests := []struct {
-		name string
-		ext  string
-	}{
-		{"lowercase yaml", ".yaml"},
-		{"uppercase YAML", ".YAML"},
-		{"mixed case Yaml", ".Yaml"},
-		{"lowercase yml", ".yml"},
-		{"uppercase YML", ".YML"},
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			dir := t.TempDir()
-			path := filepath.Join(dir, "test"+tt.ext)
-			content := "name: test\nidentity: test identity\n"
-			if err := os.WriteFile(path, []byte(content), 0644); err != nil {
-				t.Fatalf("failed to write test file: %v", err)
-			}
-
-			p, err := LoadPersona(path)
-			if err != nil {
-				t.Fatalf("LoadPersona failed for extension %s: %v", tt.ext, err)
-			}
-			if p.Name != "test" {
-				t.Errorf("Name = %q, want %q", p.Name, "test")
-			}
-		})
-	}
-}
-
-func TestCapitalizeFirst(t *testing.T) {
-	tests := []struct {
-		input string
-		want  string
-	}{
-		{"hello", "Hello"},
-		{"Hello", "Hello"},
-		{"HELLO", "HELLO"},
-		{"a", "A"},
-		{"", ""},
-		{"日本語", "日本語"}, // Non-ASCII: Japanese doesn't have case
-		{"über", "Über"},   // German umlaut
-		{"élève", "Élève"}, // French accent
-	}
-
-	for _, tt := range tests {
-		t.Run(tt.input, func(t *testing.T) {
-			got := CapitalizeFirst(tt.input)
-			if got != tt.want {
-				t.Errorf("CapitalizeFirst(%q) = %q, want %q", tt.input, got, tt.want)
-			}
-		})
-	}
-}
-
-func TestListBuiltinPersonasReturnsEmptySlice(t *testing.T) {
-	// ListBuiltinPersonas should return an empty slice (not nil) on error.
-	// We can't easily test the error case, but we can verify the success case
-	// returns a proper slice.
-	names := ListBuiltinPersonas()
-	if names == nil {
-		t.Error("ListBuiltinPersonas should return empty slice, not nil")
-	}
-}
-
-func TestYAMLMultilineStrings(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "multiline.yaml")
-
-	// Test literal block scalar (|) which preserves newlines
-	content := `name: multiline
-display_name: Multiline Test
-identity: |
-  First line.
-  Second line.
-  Third line.
-focus:
-  - item one
-ignore: []
-severity:
-  major: Major issue
-  minor: Minor issue
-  nit: Nit
-`
-
-	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	p, err := LoadPersona(path)
-	if err != nil {
-		t.Fatalf("LoadPersona failed: %v", err)
-	}
-
-	// Literal block scalar preserves newlines
-	if !strings.Contains(p.Identity, "\n") {
-		t.Error("Identity should contain newlines from literal block scalar")
-	}
-	if !strings.Contains(p.Identity, "Second line") {
-		t.Error("Identity should contain 'Second line'")
-	}
-}
-
-func TestYAMLComments(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "comments.yaml")
-
-	content := `# This is a comment
-name: commented  # inline comment
-display_name: Commented Persona
-# Another comment
-identity: Test identity
-focus:
-  - item  # comment after item
-ignore: []
-severity:
-  major: Major
-  minor: Minor
-  nit: Nit
-`
-
-	if err := os.WriteFile(path, []byte(content), 0644); err != nil {
-		t.Fatalf("failed to write test file: %v", err)
-	}
-
-	p, err := LoadPersona(path)
-	if err != nil {
-		t.Fatalf("LoadPersona failed: %v", err)
-	}
-
-	// Comments should be ignored
-	if p.Name != "commented" {
-		t.Errorf("Name = %q, want %q", p.Name, "commented")
-	}
-	if p.Focus[0] != "item" {
-		t.Errorf("Focus[0] = %q, want %q", p.Focus[0], "item")
-	}
-}
-
-func TestYAMLDeeplyNestedRejection(t *testing.T) {
-	dir := t.TempDir()
-	path := filepath.Join(dir, "deeply-nested.yaml")
-
-	// Build a deeply nested YAML structure that exceeds MaxYAMLDepth (20).
-	// Each level adds 2 to the depth count (key + value mapping).
-	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 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 seen map to prevent infinite recursion.
-	// We test this directly because go-yaml's parser handles most cycles
-	// at parse time, but we need to ensure our checker is robust.
-
-	// Create a node structure where an alias points to a parent node,
-	// simulating what could happen with malicious input that bypasses
-	// go-yaml's cycle detection.
-	parent := &yaml.Node{
-		Kind: yaml.MappingNode,
-		Content: []*yaml.Node{
-			{Kind: yaml.ScalarNode, Value: "name"},
-			{Kind: yaml.ScalarNode, Value: "test"},
-			{Kind: yaml.ScalarNode, Value: "nested"},
-		},
-	}
-
-	// Create a child that aliases back to the parent (artificial cycle)
-	aliasToParent := &yaml.Node{
-		Kind:  yaml.AliasNode,
-		Alias: parent,
-	}
-	parent.Content = append(parent.Content, aliasToParent)
-
-	nodeCount := 0
-	seen := make(map[*yaml.Node]struct{})
-
-	// This should NOT hang or stack overflow - the seen map prevents infinite recursion
-	err := checkYAMLDepth(parent, 0, MaxYAMLDepth, MaxYAMLNodes, seen, &nodeCount)
-	if err != nil {
-		t.Errorf("unexpected error traversing cyclic structure: %v", err)
-	}
-
-	// Verify we tracked the parent in the seen map
-	if _, ok := seen[parent]; !ok {
-		t.Error("parent node not tracked in seen 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.
-	// This tests the seen map logic independent of go-yaml's parsing.
-	node := &yaml.Node{
-		Kind: yaml.MappingNode,
-		Content: []*yaml.Node{
-			{Kind: yaml.ScalarNode, Value: "key"},
-			{Kind: yaml.ScalarNode, Value: "value"},
-		},
-	}
-
-	// Create a cycle by making a child reference the parent
-	cycleChild := &yaml.Node{
-		Kind:  yaml.AliasNode,
-		Alias: node, // Points back to the parent
-	}
-	node.Content = append(node.Content,
-		&yaml.Node{Kind: yaml.ScalarNode, Value: "cyclic"},
-		cycleChild,
-	)
-
-	nodeCount := 0
-	seen := make(map[*yaml.Node]struct{})
-	err := checkYAMLDepth(node, 0, MaxYAMLDepth, MaxYAMLNodes, seen, &nodeCount)
-
-	// Should complete without infinite recursion due to cycle detection
-	if err != nil {
-		t.Errorf("unexpected error: %v", err)
-	}
-	// The seen map should contain multiple entries
-	if len(seen) < 2 {
-		t.Errorf("seen map has %d entries, expected at least 2", len(seen))
-	}
-}
-
-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")
-	}
-}

review/personas/architect.yaml

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

review/personas/docs.yaml

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

review/personas/security.yaml

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

review/prompt.go

@@ -7,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/remote_persona.go

@@ -1,171 +0,0 @@
-package review
-
-import (
-	"context"
-	"fmt"
-	"log/slog"
-	"sort"
-	"strings"
-)
-
-// PersonaFetcher abstracts fetching files from a remote repository.
-// This allows persona loading to work with any Git host API.
-type PersonaFetcher interface {
-	// ListContents returns file/directory entries at a path.
-	// Returns an error if the path doesn't exist or isn't accessible.
-	ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error)
-
-	// GetFileContent returns the raw content of a file from the default branch.
-	GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error)
-}
-
-// ContentEntry represents a file or directory entry.
-type ContentEntry struct {
-	Name string // filename or directory name
-	Path string // full path from repo root
-	Type string // "file" or "dir"
-}
-
-// DefaultPersonasPath is the conventional location for repo-specific personas.
-const DefaultPersonasPath = ".review-bot/personas"
-
-// LoadRemotePersonas fetches personas from a remote repository's .review-bot/personas/ directory.
-// Returns a map of persona name to Persona. If the directory doesn't exist or is empty,
-// returns an empty map with no error (graceful fallback to built-in personas).
-//
-// Files larger than MaxPersonaFileSize are logged and skipped.
-// Invalid YAML files are logged and skipped (partial success model).
-// Only .yaml and .yml files are processed; other files are ignored.
-func LoadRemotePersonas(ctx context.Context, fetcher PersonaFetcher, owner, repo string) (map[string]*Persona, error) {
-	return LoadRemotePersonasFromPath(ctx, fetcher, owner, repo, DefaultPersonasPath)
-}
-
-// LoadRemotePersonasFromPath loads personas from a custom path in a remote repository.
-// It behaves the same as LoadRemotePersonas but allows specifying a path other than
-// the default .review-bot/personas directory.
-func LoadRemotePersonasFromPath(ctx context.Context, fetcher PersonaFetcher, owner, repo, path string) (map[string]*Persona, error) {
-	entries, err := fetcher.ListContents(ctx, owner, repo, path)
-	if err != nil {
-		// 404 is expected when repo doesn't have personas — return empty, not error
-		if isNotFoundError(err) {
-			slog.Debug("no remote personas directory found", "repo", fmt.Sprintf("%s/%s", owner, repo), "path", path)
-			return map[string]*Persona{}, nil
-		}
-		return nil, fmt.Errorf("list remote personas: %w", err)
-	}
-
-	// Cap the number of files to process to prevent resource exhaustion
-	// from repos with thousands of small files.
-	const maxPersonaFiles = 50
-
-	result := make(map[string]*Persona)
-	processed := 0
-	for _, entry := range entries {
-		if processed >= maxPersonaFiles {
-			slog.Warn("persona file limit reached", "limit", maxPersonaFiles, "repo", fmt.Sprintf("%s/%s", owner, repo))
-			break
-		}
-		if ctx.Err() != nil {
-			return nil, ctx.Err()
-		}
-
-		// Skip directories and non-YAML files
-		if entry.Type != "file" {
-			continue
-		}
-		if !isYAMLFile(entry.Name) {
-			continue
-		}
-
-		content, err := fetcher.GetFileContent(ctx, owner, repo, entry.Path)
-		if err != nil {
-			slog.Warn("could not fetch remote persona file", "file", entry.Path, "error", err)
-			continue
-		}
-
-		// Check size before parsing (defense in depth)
-		if len(content) > MaxPersonaFileSize {
-			slog.Warn("remote persona file exceeds size limit", "file", entry.Path, "size", len(content), "limit", MaxPersonaFileSize)
-			continue
-		}
-
-		// YAML parsing uses parsePersona which has defenses against YAML DoS attacks:
-		// - MaxPersonaFileSize (above) caps raw input size before any parsing
-		// - maxPersonaFiles (above) limits the number of files processed per repo
-		// - unmarshalYAMLWithDepthLimit enforces MaxYAMLDepth to prevent stack exhaustion
-		// - checkYAMLDepth tracks node counts (MaxYAMLNodes) against "billion laughs" expansion
-		// - Alias cycles are detected and capped by seen-node tracking
-		// See persona.go for the implementation details.
-		persona, err := parsePersona([]byte(content), entry.Path)
-		if err != nil {
-			slog.Warn("could not parse remote persona file", "file", entry.Path, "error", err)
-			continue
-		}
-
-		result[persona.Name] = persona
-		processed++
-		slog.Debug("loaded remote persona", "name", persona.Name, "file", entry.Path)
-	}
-
-	return result, nil
-}
-
-// MergePersonas combines remote and built-in personas.
-// Remote personas take precedence on name collision.
-// Returns the merged map and a list of persona names in sorted order.
-func MergePersonas(remote, builtin map[string]*Persona) (map[string]*Persona, []string) {
-	merged := make(map[string]*Persona)
-
-	// Add built-in first
-	for name, p := range builtin {
-		merged[name] = p
-	}
-
-	// Remote overrides built-in on collision
-	for name, p := range remote {
-		if _, exists := merged[name]; exists {
-			slog.Debug("remote persona overrides built-in", "name", name)
-		}
-		merged[name] = p
-	}
-
-	// Collect sorted names
-	names := make([]string, 0, len(merged))
-	for name := range merged {
-		names = append(names, name)
-	}
-	sort.Strings(names)
-
-	return merged, names
-}
-
-// LoadAllBuiltinPersonas loads all built-in personas into a map.
-func LoadAllBuiltinPersonas() 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 returns true if the 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 indicates a 404 response.
-// This is a simple string check to avoid importing the gitea package
-// (which would create a circular dependency).
-func isNotFoundError(err error) bool {
-	if err == nil {
-		return false
-	}
-	errStr := err.Error()
-	return strings.Contains(errStr, "HTTP 404")
-}

review/remote_persona_test.go

@@ -1,394 +0,0 @@
-package review
-
-import (
-	"context"
-	"errors"
-	"testing"
-)
-
-// mockFetcher implements PersonaFetcher for testing.
-type mockFetcher struct {
-	contents     map[string][]ContentEntry // path -> entries
-	files        map[string]string         // path -> content
-	listErr      error                     // error to return from ListContents
-	getFileErr   map[string]error          // path -> error for GetFileContent
-	listNotFound bool                      // return 404-style error
-}
-
-func newMockFetcher() *mockFetcher {
-	return &mockFetcher{
-		contents:   make(map[string][]ContentEntry),
-		files:      make(map[string]string),
-		getFileErr: make(map[string]error),
-	}
-}
-
-func (m *mockFetcher) ListContents(ctx context.Context, owner, repo, path string) ([]ContentEntry, error) {
-	if m.listNotFound {
-		return nil, errors.New("HTTP 404: not found")
-	}
-	if m.listErr != nil {
-		return nil, m.listErr
-	}
-	entries, ok := m.contents[path]
-	if !ok {
-		return nil, errors.New("HTTP 404: not found")
-	}
-	return entries, nil
-}
-
-func (m *mockFetcher) GetFileContent(ctx context.Context, owner, repo, filepath string) (string, error) {
-	if err, ok := m.getFileErr[filepath]; ok {
-		return "", err
-	}
-	content, ok := m.files[filepath]
-	if !ok {
-		return "", errors.New("HTTP 404: file not found")
-	}
-	return content, nil
-}
-
-func TestLoadRemotePersonas_NoDirectory(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.listNotFound = true
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("expected no error for missing directory, got: %v", err)
-	}
-	if len(result) != 0 {
-		t.Errorf("expected empty map, got %d personas", len(result))
-	}
-}
-
-func TestLoadRemotePersonas_EmptyDirectory(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{}
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 0 {
-		t.Errorf("expected empty map, got %d personas", len(result))
-	}
-}
-
-func TestLoadRemotePersonas_SinglePersona(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "trading.yaml", Path: ".review-bot/personas/trading.yaml", Type: "file"},
-	}
-	fetcher.files[".review-bot/personas/trading.yaml"] = `
-name: trading
-display_name: Trading Expert
-identity: You are a trading systems expert.
-focus:
-  - order execution
-  - market data
-`
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 1 {
-		t.Fatalf("expected 1 persona, got %d", len(result))
-	}
-	if result["trading"] == nil {
-		t.Fatal("expected 'trading' persona")
-	}
-	if result["trading"].DisplayName != "Trading Expert" {
-		t.Errorf("expected display name 'Trading Expert', got %q", result["trading"].DisplayName)
-	}
-}
-
-func TestLoadRemotePersonas_MultiplePersonas(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "one.yaml", Path: ".review-bot/personas/one.yaml", Type: "file"},
-		{Name: "two.yml", Path: ".review-bot/personas/two.yml", Type: "file"},
-	}
-	fetcher.files[".review-bot/personas/one.yaml"] = `
-name: one
-identity: First persona.
-`
-	fetcher.files[".review-bot/personas/two.yml"] = `
-name: two
-identity: Second persona.
-`
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 2 {
-		t.Fatalf("expected 2 personas, got %d", len(result))
-	}
-	if result["one"] == nil || result["two"] == nil {
-		t.Error("expected both personas to be loaded")
-	}
-}
-
-func TestLoadRemotePersonas_SkipsNonYAML(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "valid.yaml", Path: ".review-bot/personas/valid.yaml", Type: "file"},
-		{Name: "readme.md", Path: ".review-bot/personas/readme.md", Type: "file"},
-		{Name: "config.json", Path: ".review-bot/personas/config.json", Type: "file"},
-	}
-	fetcher.files[".review-bot/personas/valid.yaml"] = `
-name: valid
-identity: Valid persona.
-`
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 1 {
-		t.Fatalf("expected 1 persona (skipping non-YAML), got %d", len(result))
-	}
-}
-
-func TestLoadRemotePersonas_SkipsDirectories(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "valid.yaml", Path: ".review-bot/personas/valid.yaml", Type: "file"},
-		{Name: "subdir", Path: ".review-bot/personas/subdir", Type: "dir"},
-	}
-	fetcher.files[".review-bot/personas/valid.yaml"] = `
-name: valid
-identity: Valid persona.
-`
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 1 {
-		t.Fatalf("expected 1 persona (skipping dir), got %d", len(result))
-	}
-}
-
-func TestLoadRemotePersonas_SkipsInvalidYAML(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "valid.yaml", Path: ".review-bot/personas/valid.yaml", Type: "file"},
-		{Name: "invalid.yaml", Path: ".review-bot/personas/invalid.yaml", Type: "file"},
-	}
-	fetcher.files[".review-bot/personas/valid.yaml"] = `
-name: valid
-identity: Valid persona.
-`
-	fetcher.files[".review-bot/personas/invalid.yaml"] = `
-this is not valid yaml: [unclosed bracket
-`
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 1 {
-		t.Fatalf("expected 1 persona (skipping invalid), got %d", len(result))
-	}
-	if result["valid"] == nil {
-		t.Error("expected valid persona to be loaded")
-	}
-}
-
-func TestLoadRemotePersonas_SkipsOversizedFiles(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "huge.yaml", Path: ".review-bot/personas/huge.yaml", Type: "file"},
-	}
-	// Create content larger than MaxPersonaFileSize (64KB)
-	fetcher.files[".review-bot/personas/huge.yaml"] = `
-name: huge
-identity: ` + string(make([]byte, MaxPersonaFileSize+1000))
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 0 {
-		t.Errorf("expected 0 personas (oversized file skipped), got %d", len(result))
-	}
-}
-
-func TestLoadRemotePersonas_SkipsFetchErrors(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "valid.yaml", Path: ".review-bot/personas/valid.yaml", Type: "file"},
-		{Name: "error.yaml", Path: ".review-bot/personas/error.yaml", Type: "file"},
-	}
-	fetcher.files[".review-bot/personas/valid.yaml"] = `
-name: valid
-identity: Valid persona.
-`
-	fetcher.getFileErr[".review-bot/personas/error.yaml"] = errors.New("network error")
-
-	result, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err != nil {
-		t.Fatalf("unexpected error: %v", err)
-	}
-	if len(result) != 1 {
-		t.Fatalf("expected 1 persona (skipping error), got %d", len(result))
-	}
-}
-
-func TestLoadRemotePersonas_ListContentsError(t *testing.T) {
-	fetcher := newMockFetcher()
-	fetcher.listErr = errors.New("server error")
-
-	_, err := LoadRemotePersonas(context.Background(), fetcher, "owner", "repo")
-	if err == nil {
-		t.Fatal("expected error for list contents failure")
-	}
-}
-
-func TestLoadRemotePersonas_ContextCancellation(t *testing.T) {
-	ctx, cancel := context.WithCancel(context.Background())
-	cancel() // Cancel immediately
-
-	fetcher := newMockFetcher()
-	fetcher.contents[DefaultPersonasPath] = []ContentEntry{
-		{Name: "one.yaml", Path: ".review-bot/personas/one.yaml", Type: "file"},
-	}
-	fetcher.files[".review-bot/personas/one.yaml"] = `
-name: one
-identity: One.
-`
-
-	_, err := LoadRemotePersonas(ctx, fetcher, "owner", "repo")
-	if err == nil {
-		t.Fatal("expected context cancellation error")
-	}
-}
-
-func TestMergePersonas_NoOverlap(t *testing.T) {
-	remote := map[string]*Persona{
-		"trading": {Name: "trading", Identity: "Trading expert."},
-	}
-	builtin := map[string]*Persona{
-		"security": {Name: "security", Identity: "Security expert."},
-	}
-
-	merged, names := MergePersonas(remote, builtin)
-
-	if len(merged) != 2 {
-		t.Fatalf("expected 2 personas, got %d", len(merged))
-	}
-	if len(names) != 2 {
-		t.Fatalf("expected 2 names, got %d", len(names))
-	}
-	// Names should be sorted
-	if names[0] != "security" || names[1] != "trading" {
-		t.Errorf("expected sorted names [security, trading], got %v", names)
-	}
-}
-
-func TestMergePersonas_RemoteOverridesBuiltin(t *testing.T) {
-	remote := map[string]*Persona{
-		"security": {Name: "security", Identity: "Custom security expert."},
-	}
-	builtin := map[string]*Persona{
-		"security": {Name: "security", Identity: "Default security expert."},
-	}
-
-	merged, _ := MergePersonas(remote, builtin)
-
-	if merged["security"].Identity != "Custom security expert." {
-		t.Errorf("expected remote to override builtin, got identity: %q", merged["security"].Identity)
-	}
-}
-
-func TestMergePersonas_EmptyRemote(t *testing.T) {
-	remote := map[string]*Persona{}
-	builtin := map[string]*Persona{
-		"security": {Name: "security", Identity: "Security."},
-	}
-
-	merged, names := MergePersonas(remote, builtin)
-
-	if len(merged) != 1 {
-		t.Fatalf("expected 1 persona, got %d", len(merged))
-	}
-	if names[0] != "security" {
-		t.Errorf("expected 'security', got %q", names[0])
-	}
-}
-
-func TestMergePersonas_EmptyBuiltin(t *testing.T) {
-	remote := map[string]*Persona{
-		"trading": {Name: "trading", Identity: "Trading."},
-	}
-	builtin := map[string]*Persona{}
-
-	merged, names := MergePersonas(remote, builtin)
-
-	if len(merged) != 1 {
-		t.Fatalf("expected 1 persona, got %d", len(merged))
-	}
-	if names[0] != "trading" {
-		t.Errorf("expected 'trading', got %q", names[0])
-	}
-}
-
-func TestLoadAllBuiltinPersonas(t *testing.T) {
-	personas := LoadAllBuiltinPersonas()
-
-	// Should load at least the known built-in personas
-	expected := []string{"architect", "docs", "security"}
-	for _, name := range expected {
-		if personas[name] == nil {
-			t.Errorf("expected built-in persona %q to be loaded", name)
-		}
-	}
-}
-
-func TestIsYAMLFile(t *testing.T) {
-	tests := []struct {
-		name     string
-		expected bool
-	}{
-		{"test.yaml", true},
-		{"test.yml", true},
-		{"test.YAML", true},
-		{"test.YML", true},
-		{"test.json", false},
-		{"test.md", false},
-		{"yaml", false},
-		{"", false},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			if got := isYAMLFile(tc.name); got != tc.expected {
-				t.Errorf("isYAMLFile(%q) = %v, want %v", tc.name, got, tc.expected)
-			}
-		})
-	}
-}
-
-func TestIsNotFoundError(t *testing.T) {
-	tests := []struct {
-		name     string
-		err      error
-		expected bool
-	}{
-		{"nil error", nil, false},
-		{"HTTP 404", errors.New("HTTP 404: not found"), true},
-		{"not found text", errors.New("path not found"), false},
-		{"server error", errors.New("server error"), false},
-		{"HTTP 500", errors.New("HTTP 500: internal error"), false},
-	}
-
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			if got := isNotFoundError(tc.err); got != tc.expected {
-				t.Errorf("isNotFoundError(%v) = %v, want %v", tc.err, got, tc.expected)
-			}
-		})
-	}
-}

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[@]}"