feat: native SAP AI Core support

Add native SAP AI Core provider that handles OAuth token management and
deployment discovery automatically. This eliminates the need for the
external LLM proxy when running in SAP environments.

Changes:
- Add AICoreClient with OAuth token caching and deployment URL discovery
- Support both Anthropic and OpenAI models via AI Core deployments
- Update CI to use native AI Core provider
- Update action inputs to accept AI Core credentials
- Update README with AI Core configuration examples

Model names must match AI Core deployment names (e.g. anthropic--claude-4.6-sonnet, gpt-5).

.gitea/actions/review/action.yml

@@ -26,18 +26,40 @@ inputs:
     required: false
     default: ''
   llm-base-url:
-    description: 'OpenAI-compatible LLM API base URL'
-    required: true
+    description: 'OpenAI-compatible LLM API base URL (not required for aicore provider)'
+    required: false
+    default: ''
   llm-api-key:
-    description: 'LLM API key'
-    required: true
+    description: 'LLM API key (not required for aicore provider)'
+    required: false
+    default: ''
   llm-model:
     description: 'LLM model name'
     required: true
   llm-provider:
-    description: 'LLM API provider: openai or anthropic (default openai)'
+    description: 'LLM API provider: openai, anthropic, or aicore (default openai)'
     required: false
-    default: 'openai' 
+    default: 'openai'
+  aicore-client-id:
+    description: 'SAP AI Core client ID (required for aicore provider)'
+    required: false
+    default: ''
+  aicore-client-secret:
+    description: 'SAP AI Core client secret (required for aicore provider)'
+    required: false
+    default: ''
+  aicore-auth-url:
+    description: 'SAP AI Core authentication URL (required for aicore provider)'
+    required: false
+    default: ''
+  aicore-api-url:
+    description: 'SAP AI Core API URL (required for aicore provider)'
+    required: false
+    default: ''
+  aicore-resource-group:
+    description: 'SAP AI Core resource group (default: default)'
+    required: false
+    default: 'default'
   conventions-file:
     description: 'Path to conventions file in the repo (e.g. CLAUDE.md)'
     required: false
@@ -79,7 +101,7 @@ inputs:
     required: false
     default: ''
   persona-file:
-    description: 'Path to persona JSON file with custom review focus'
+    description: 'Path to custom persona JSON file'
     required: false
     default: ''
 
@@ -165,6 +187,11 @@ runs:
         SYSTEM_PROMPT_FILE: ${{ inputs.system-prompt-file }}
         PERSONA: ${{ inputs.persona }}
         PERSONA_FILE: ${{ inputs.persona-file }}
+        AICORE_CLIENT_ID: ${{ inputs.aicore-client-id }}
+        AICORE_CLIENT_SECRET: ${{ inputs.aicore-client-secret }}
+        AICORE_AUTH_URL: ${{ inputs.aicore-auth-url }}
+        AICORE_API_URL: ${{ inputs.aicore-api-url }}
+        AICORE_RESOURCE_GROUP: ${{ inputs.aicore-resource-group }}
       run: |
         ARGS=""
         if [ "${{ inputs.dry-run }}" = "true" ]; then

.gitea/workflows/ci.yml

@@ -18,8 +18,10 @@ jobs:
       - run: go vet ./...
       - run: go build -o review-bot ./cmd/review-bot
 
-  # Self-review: builds from source since we're pre-release
-  # Models configured to match SAP AI Core deployments
+  # Self-review using native SAP AI Core provider
+  # Models must match SAP AI Core deployments
+  # Available models: gpt-5, anthropic--claude-4.6-sonnet, anthropic--claude-4.6-opus
+  # Removed gpt-4.1, gpt-5-mini, gpt-4.1-mini - not deployed on AI Core
   review:
     runs-on: ubuntu-24.04
     if: github.event_name == 'pull_request'
@@ -29,18 +31,12 @@ jobs:
         include:
           - name: sonnet
             token_secret: SONNET_REVIEW_TOKEN
-            provider: anthropic
-            llm_path: /anthropic/v1
             model: anthropic--claude-4.6-sonnet
           - name: gpt
             token_secret: GPT_REVIEW_TOKEN
-            provider: openai
-            llm_path: /openai/v1
             model: gpt-5
           - name: security
             token_secret: SECURITY_REVIEW_TOKEN
-            provider: openai
-            llm_path: /openai/v1
             model: gpt-5
             system_prompt_file: SECURITY_REVIEW.md
     steps:
@@ -56,10 +52,13 @@ jobs:
           PR_NUMBER: ${{ github.event.pull_request.number }}
           REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }}
           REVIEWER_NAME: ${{ matrix.name }}
-          LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }}${{ matrix.llm_path }}
-          LLM_API_KEY: ${{ secrets.LLM_API_KEY }}
+          LLM_PROVIDER: aicore
           LLM_MODEL: ${{ matrix.model }}
-          LLM_PROVIDER: ${{ matrix.provider }}
+          AICORE_CLIENT_ID: ${{ secrets.AICORE_CLIENT_ID }}
+          AICORE_CLIENT_SECRET: ${{ secrets.AICORE_CLIENT_SECRET }}
+          AICORE_AUTH_URL: ${{ secrets.AICORE_AUTH_URL }}
+          AICORE_API_URL: ${{ secrets.AICORE_API_URL }}
+          AICORE_RESOURCE_GROUP: ${{ secrets.AICORE_RESOURCE_GROUP }}
           CONVENTIONS_FILE: "CONVENTIONS.md"
           PATTERNS_REPO: "rodin/go-patterns"
           PATTERNS_FILES: "README.md,patterns/"

README.md

@@ -4,7 +4,7 @@ AI-powered code review bot for Gitea pull requests. Fetches diff + context, send
 
 ## Features
 
-- **Multi-provider**: OpenAI-compatible and Anthropic Messages API
+- **Multi-provider**: OpenAI-compatible, Anthropic Messages API, and SAP AI Core
 - **Context-aware**: Fetches full file content, conventions, language patterns, CI status
 - **Smart budget**: Automatically trims context to fit model token limits
 - **Idempotent reviews**: Posts new review, then cleans up stale ones (one review per bot)
@@ -168,16 +168,41 @@ Prints the review to CI logs without posting to the PR. Useful for testing promp
     llm-provider: anthropic
 ```
 
+### Using SAP AI Core
+
+For SAP environments with AI Core deployments, use the `aicore` provider for native authentication:
+
+```yaml
+- uses: https://gitea.weiker.me/rodin/review-bot/.gitea/actions/review@v0.1.0
+  with:
+    reviewer-token: ${{ secrets.REVIEW_TOKEN }}
+    reviewer-name: aicore-review
+    llm-model: anthropic--claude-4.6-sonnet  # or gpt-5
+    llm-provider: aicore
+    aicore-client-id: ${{ secrets.AICORE_CLIENT_ID }}
+    aicore-client-secret: ${{ secrets.AICORE_CLIENT_SECRET }}
+    aicore-auth-url: ${{ secrets.AICORE_AUTH_URL }}
+    aicore-api-url: ${{ secrets.AICORE_API_URL }}
+    aicore-resource-group: default
+```
+
+AI Core handles OAuth token management and deployment discovery automatically. Model names must match the deployment name in AI Core (e.g. `anthropic--claude-4.6-sonnet`, `gpt-5`).
+
 ## Action Inputs
 
 | Input | Required | Default | Description |
 |-------|----------|---------|-------------|
 | `reviewer-token` | Yes | — | Gitea token for posting reviews (needs `write:issue`, `write:repository`) |
 | `reviewer-name` | No | `""` | Logical identity for this reviewer. Used as sentinel for idempotent cleanup. Set this when running multiple review bots on the same PR. |
-| `llm-base-url` | Yes | — | LLM API base URL |
-| `llm-api-key` | Yes | — | LLM API key |
+| `llm-base-url` | No* | `""` | LLM API base URL (required unless using aicore provider) |
+| `llm-api-key` | No* | `""` | LLM API key (required unless using aicore provider) |
 | `llm-model` | Yes | — | Model name |
-| `llm-provider` | No | `openai` | API provider: `openai` or `anthropic` |
+| `llm-provider` | No | `openai` | API provider: `openai`, `anthropic`, or `aicore` |
+| `aicore-client-id` | No** | `""` | SAP AI Core client ID |
+| `aicore-client-secret` | No** | `""` | SAP AI Core client secret |
+| `aicore-auth-url` | No** | `""` | SAP AI Core authentication URL |
+| `aicore-api-url` | No** | `""` | SAP AI Core API URL |
+| `aicore-resource-group` | No | `default` | SAP AI Core resource group |
 | `conventions-file` | No | `""` | Path to coding conventions file in the repo |
 | `patterns-repo` | No | `""` | Comma-separated repos with language patterns (e.g. `rodin/go-patterns`) |
 | `patterns-files` | No | `README.md` | Files/directories to fetch from pattern repos |
@@ -190,6 +215,9 @@ Prints the review to CI logs without posting to the PR. Useful for testing promp
 | `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:
@@ -380,38 +408,32 @@ Each persona posts independently with its own sentinel, so reviews don't interfe
 
 ### Custom Personas
 
-Create a YAML file with your domain-specific review focus:
+Create a JSON 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
+```json
+// .review/personas/trading.json
+{
+  "name": "trading",
+  "display_name": "Trading Domain Expert",
+  "identity": "You are a trading systems expert reviewing code for correctness.\n\nYour expertise:\n- Order lifecycle and state machines\n- Fill handling and partial fills\n- Position tracking and P&L calculations\n- 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:
@@ -420,18 +442,17 @@ Use it in CI:
 - uses: rodin/review-bot/.gitea/actions/review@v1
   with:
     reviewer-name: trading
-    persona-file: .review/personas/trading.yaml
+    persona-file: .review/personas/trading.json
     ...
 ```
 
-JSON format is also supported for backwards compatibility.
 
 ### 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) |
+| Structured format | Yes (JSON) | No (freeform) |
 | Focus/ignore lists | Yes | Manual |
 | Severity calibration | Yes | Manual |
 | Header display name | Yes | No |

cmd/review-bot/main.go

@@ -69,10 +69,17 @@ 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 or anthropic")
+	llmProvider := flag.String("llm-provider", envOrDefault("LLM_PROVIDER", "openai"), "LLM API provider: openai, anthropic, or aicore")
 	personaName := flag.String("persona", envOrDefault("PERSONA", ""), "Built-in persona name (security, architect, docs)")
 	personaFile := flag.String("persona-file", envOrDefault("PERSONA_FILE", ""), "Path to persona JSON file")
+	// AI Core specific flags (only used when provider=aicore)
+	aicoreClientID := flag.String("aicore-client-id", envOrDefault("AICORE_CLIENT_ID", ""), "SAP AI Core client ID (for provider=aicore)")
+	aicoreClientSecret := flag.String("aicore-client-secret", envOrDefault("AICORE_CLIENT_SECRET", ""), "SAP AI Core client secret (for provider=aicore)")
+	aicoreAuthURL := flag.String("aicore-auth-url", envOrDefault("AICORE_AUTH_URL", ""), "SAP AI Core auth URL (for provider=aicore)")
+	aicoreAPIURL := flag.String("aicore-api-url", envOrDefault("AICORE_API_URL", ""), "SAP AI Core API URL (for provider=aicore)")
+	aicoreResourceGroup := flag.String("aicore-resource-group", envOrDefault("AICORE_RESOURCE_GROUP", "default"), "SAP AI Core resource group (for provider=aicore)")
 
+	flag.Parse()
 	flag.Parse()
 
 	if *versionFlag {
@@ -86,10 +93,20 @@ func main() {
 	slog.Info("review-bot starting", "version", version)
 
 	// Validate required fields
-	if *giteaURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" ||
-		*llmBaseURL == "" || *llmAPIKey == "" || *llmModel == "" {
+	// For aicore provider, llm-base-url and llm-api-key are not required
+	isAICore := llm.Provider(*llmProvider) == llm.ProviderAICore
+	if *giteaURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" || *llmModel == "" {
 		fmt.Fprintf(os.Stderr, "Error: missing required flags or environment variables\n\n")
-		fmt.Fprintf(os.Stderr, "Required: --gitea-url, --repo, --pr, --reviewer-token, --llm-base-url, --llm-api-key, --llm-model\n")
+		fmt.Fprintf(os.Stderr, "Required: --gitea-url, --repo, --pr, --reviewer-token, --llm-model\n")
+		os.Exit(1)
+	}
+	if !isAICore && (*llmBaseURL == "" || *llmAPIKey == "") {
+		fmt.Fprintf(os.Stderr, "Error: --llm-base-url and --llm-api-key are required for provider=%s\n", *llmProvider)
+		os.Exit(1)
+	}
+	if isAICore && (*aicoreClientID == "" || *aicoreClientSecret == "" || *aicoreAuthURL == "" || *aicoreAPIURL == "") {
+		fmt.Fprintf(os.Stderr, "Error: AI Core credentials required for provider=aicore\n\n")
+		fmt.Fprintf(os.Stderr, "Required: --aicore-client-id, --aicore-client-secret, --aicore-auth-url, --aicore-api-url\n")
 		os.Exit(1)
 	}
 
@@ -157,8 +174,17 @@ func main() {
 	switch llm.Provider(*llmProvider) {
 	case llm.ProviderOpenAI, llm.ProviderAnthropic:
 		llmClient.WithProvider(llm.Provider(*llmProvider))
+	case llm.ProviderAICore:
+		llmClient.WithAICore(llm.AICoreConfig{
+			ClientID:      *aicoreClientID,
+			ClientSecret:  *aicoreClientSecret,
+			AuthURL:       *aicoreAuthURL,
+			APIURL:        *aicoreAPIURL,
+			ResourceGroup: *aicoreResourceGroup,
+		})
+		slog.Info("using SAP AI Core provider", "resource_group", *aicoreResourceGroup)
 	default:
-		slog.Error("invalid LLM provider", "provider", *llmProvider, "valid", "openai, anthropic")
+		slog.Error("invalid LLM provider", "provider", *llmProvider, "valid", "openai, anthropic, aicore")
 		os.Exit(1)
 	}
 	if *llmTimeout > 0 {
@@ -622,21 +648,28 @@ func validateWorkspacePath(path, pathName string) (string, error) {
 	if err != nil {
 		return "", fmt.Errorf("failed to resolve workspace path: %w", err)
 	}
+
 	// Join and clean the path
 	fullPath := filepath.Join(absWorkspace, path)
 	fullPath = filepath.Clean(fullPath)
-	// Check path is within workspace
-	if !strings.HasPrefix(fullPath, absWorkspace+string(filepath.Separator)) && fullPath != absWorkspace {
+
+	// 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)
 	}
-	if !strings.HasPrefix(resolvedPath, absWorkspace+string(filepath.Separator)) && resolvedPath != absWorkspace {
+
+	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
 }
 

cmd/review-bot/main_test.go

@@ -6,8 +6,8 @@ import (
 	"log/slog"
 	"os"
 	"os/exec"
-	"strings"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"gitea.weiker.me/rodin/review-bot/gitea"
@@ -103,11 +103,13 @@ func TestValidateWorkspacePath(t *testing.T) {
 			errMatch:  "resolves outside workspace",
 		},
 		{
-			name:      "absolute path gets normalized to relative",
+			name:      "absolute path normalized to workspace-relative",
 			workspace: tmpDir,
 			path:      "/etc/passwd",
 			wantErr:   true,
-			errMatch:  "failed to resolve", // filepath.Join strips leading / making it <workspace>/etc/passwd which doesn't exist
+			// 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",
@@ -152,7 +154,6 @@ func TestValidateWorkspacePath(t *testing.T) {
 	}
 }
 
-
 func makeReview(id int64, login, state string, stale bool, body string) gitea.Review {
 	r := gitea.Review{
 		ID:    id,
@@ -164,7 +165,6 @@ func makeReview(id int64, login, state string, stale bool, body string) gitea.Re
 	return r
 }
 
-
 func TestBuildSupersededBody(t *testing.T) {
 	original := "# Review\n\nLooks good.\n\n<!-- review-bot:sonnet -->"
 	sentinel := "<!-- review-bot:sonnet -->"
@@ -734,8 +734,8 @@ func TestExtractSentinelName_EdgeCases(t *testing.T) {
 		{"<!-- review-bot:sonnet --> rest", "sonnet"},
 		{"<!-- review-bot:gpt-review --> rest", "gpt-review"},
 		{"no sentinel here", "unknown"},
-		{"<!-- review-bot:", "unknown"},       // prefix but no suffix
-		{"prefix <!-- review-bot:abc --> end", "abc"}, // embedded in text
+		{"<!-- review-bot:", "unknown"},                   // prefix but no suffix
+		{"prefix <!-- review-bot:abc --> end", "abc"},     // embedded in text
 	}
 
 	for _, tc := range tests {

docs/DESIGN-51-personas.md

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

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 // indirect

go.sum

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

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

llm/aicore_test.go

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

llm/client.go

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

review/formatter.go

@@ -7,39 +7,7 @@ import (
 
 // FormatMarkdown formats a ReviewResult into the markdown body for a Gitea review.
 func FormatMarkdown(result *ReviewResult, reviewerName string) string {
-	var sb strings.Builder
-
-	if reviewerName != "" {
-		title := strings.ToUpper(reviewerName[:1]) + reviewerName[1:]
-		sb.WriteString(fmt.Sprintf("# %s Review\n\n", title))
-	}
-
-	sb.WriteString("## Summary\n\n")
-	sb.WriteString(result.Summary)
-	sb.WriteString("\n\n")
-
-	if len(result.Findings) > 0 {
-		sb.WriteString("## Findings\n\n")
-		sb.WriteString("| # | Severity | File | Line | Finding |\n")
-		sb.WriteString("|---|----------|------|------|--------|\n")
-
-		for i, f := range result.Findings {
-			sb.WriteString(fmt.Sprintf("| %d | [%s] | `%s` | %d | %s |\n",
-				i+1, f.Severity, f.File, f.Line, f.Finding))
-		}
-		sb.WriteString("\n")
-	}
-
-	sb.WriteString("## Recommendation\n\n")
-	sb.WriteString(fmt.Sprintf("**%s** — %s\n", result.Verdict, result.Recommendation))
-
-	if reviewerName != "" {
-		sb.WriteString(fmt.Sprintf("\n---\n*Review by %s*\n", reviewerName))
-		// Hidden sentinel for identifying this bot's reviews during cleanup
-		sb.WriteString(fmt.Sprintf("\n<!-- review-bot:%s -->\n", reviewerName))
-	}
-
-	return sb.String()
+	return FormatMarkdownWithDisplay(result, reviewerName, reviewerName)
 }
 
 // GiteaEvent converts the verdict to the Gitea API event string.
@@ -55,6 +23,8 @@ func GiteaEvent(verdict string) string {
 }
 
 // FormatMarkdownWithDisplay formats a ReviewResult with separate display name and sentinel name.
+// Note: displayName is not HTML-escaped as Gitea sanitizes rendered Markdown.
+// Persona display names are controlled by repo owners (trusted input).
 // displayName is used for the header title, sentinelName is used for the cleanup sentinel.
 // If displayName is empty, sentinelName is used for both.
 func FormatMarkdownWithDisplay(result *ReviewResult, displayName, sentinelName string) string {
@@ -67,7 +37,7 @@ func FormatMarkdownWithDisplay(result *ReviewResult, displayName, sentinelName s
 	}
 
 	if headerName != "" {
-		title := strings.ToUpper(headerName[:1]) + headerName[1:]
+		title := CapitalizeFirst(headerName)
 		sb.WriteString(fmt.Sprintf("# %s Review\n\n", title))
 	}
 

review/persona.go

@@ -5,37 +5,34 @@ import (
 	"encoding/json"
 	"fmt"
 	"os"
-	"path/filepath"
 	"strings"
-
-	"gopkg.in/yaml.v3"
+	"unicode/utf8"
 )
 
-//go:embed personas/*.yaml
+//go:embed personas/*.json
 var embeddedPersonas embed.FS
 
 // 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"`
+	Name         string   `json:"name"`
+	DisplayName  string   `json:"display_name"`
+	ModelPref    string   `json:"model_preference,omitempty"`
+	Identity     string   `json:"identity"`
+	Focus        []string `json:"focus"`
+	Ignore       []string `json:"ignore"`
+	Severity     Severity `json:"severity"`
+	OutputFormat string   `json:"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"`
+	Major string `json:"major"`
+	Minor string `json:"minor"`
+	Nit   string `json:"nit"`
 }
 
-// LoadPersona loads a persona from a file path.
-// Supports both YAML (.yaml, .yml) and JSON (.json) formats.
+// LoadPersona loads a persona from a JSON file path.
 func LoadPersona(path string) (*Persona, error) {
 	data, err := os.ReadFile(path)
 	if err != nil {
@@ -47,8 +44,8 @@ func LoadPersona(path string) (*Persona, error) {
 // LoadBuiltinPersona loads a built-in persona by name.
 // Returns an error if the persona doesn't exist.
 func LoadBuiltinPersona(name string) (*Persona, error) {
-	filename := name + ".yaml"
-	data, err := embeddedPersonas.ReadFile(filepath.Join("personas", filename))
+	filename := name + ".json"
+	data, err := embeddedPersonas.ReadFile("personas/" + filename) // embed.FS paths use forward slashes per io/fs spec
 	if err != nil {
 		available := ListBuiltinPersonas()
 		return nil, fmt.Errorf("unknown built-in persona %q (available: %s)", name, strings.Join(available, ", "))
@@ -57,10 +54,11 @@ func LoadBuiltinPersona(name string) (*Persona, error) {
 }
 
 // ListBuiltinPersonas returns the names of all built-in personas.
+// Returns an empty slice if the embedded directory cannot be read.
 func ListBuiltinPersonas() []string {
 	entries, err := embeddedPersonas.ReadDir("personas")
 	if err != nil {
-		return nil
+		return []string{}
 	}
 	var names []string
 	for _, e := range entries {
@@ -68,10 +66,8 @@ func ListBuiltinPersonas() []string {
 			continue
 		}
 		name := e.Name()
-		if strings.HasSuffix(name, ".yaml") {
-			names = append(names, strings.TrimSuffix(name, ".yaml"))
-		} else if strings.HasSuffix(name, ".yml") {
-			names = append(names, strings.TrimSuffix(name, ".yml"))
+		if strings.HasSuffix(name, ".json") {
+			names = append(names, strings.TrimSuffix(name, ".json"))
 		}
 	}
 	return names
@@ -79,20 +75,9 @@ func ListBuiltinPersonas() []string {
 
 func parsePersona(data []byte, source string) (*Persona, error) {
 	var p Persona
-	
-	// Determine format by extension or try YAML first (it's a superset of JSON)
-	ext := strings.ToLower(filepath.Ext(source))
-	if ext == ".json" {
-		if err := json.Unmarshal(data, &p); err != nil {
-			return nil, fmt.Errorf("parse persona %s: %w", source, err)
-		}
-	} else {
-		// YAML (also handles .yaml, .yml, and builtin: prefix)
-		if err := yaml.Unmarshal(data, &p); err != nil {
-			return nil, fmt.Errorf("parse persona %s: %w", source, err)
-		}
+	if err := json.Unmarshal(data, &p); err != nil {
+		return nil, fmt.Errorf("parse persona %s: %w", source, err)
 	}
-	
 	if err := validatePersona(&p, source); err != nil {
 		return nil, err
 	}
@@ -112,3 +97,16 @@ func validatePersona(p *Persona, source string) error {
 	}
 	return nil
 }
+
+// CapitalizeFirst capitalizes the first rune of a string in a Unicode-safe way.
+// Returns the original string if it's empty.
+func CapitalizeFirst(s string) string {
+	if s == "" {
+		return s
+	}
+	r, size := utf8.DecodeRuneInString(s)
+	if r == utf8.RuneError {
+		return s
+	}
+	return strings.ToUpper(string(r)) + s[size:]
+}

review/persona_prompt.go

@@ -50,7 +50,7 @@ func BuildPersonaSystemPrompt(p *Persona) string {
 		sb.WriteString("\n")
 	}
 
-	// Output format instructions (same as base, but with persona context)
+	// 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")
@@ -61,24 +61,10 @@ func BuildPersonaSystemPrompt(p *Persona) string {
 	sb.WriteString("2. Consider the CI status — if CI has failed, that is an automatic REQUEST_CHANGES regardless of code quality.\n")
 	sb.WriteString("3. Output your review as structured JSON (and ONLY JSON, no markdown fences or other text).\n\n")
 	sb.WriteString("Output format:\n")
-	sb.WriteString("{\n")
-	sb.WriteString("  \"verdict\": \"APPROVE\" or \"REQUEST_CHANGES\",\n")
-	sb.WriteString("  \"summary\": \"Brief overall assessment (1-3 sentences)\",\n")
-	sb.WriteString("  \"findings\": [\n")
-	sb.WriteString("    {\n")
-	sb.WriteString("      \"severity\": \"MAJOR\" or \"MINOR\" or \"NIT\",\n")
-	sb.WriteString("      \"file\": \"path/to/file\",\n")
-	sb.WriteString("      \"line\": <line number from the diff>,\n")
-	sb.WriteString("      \"finding\": \"Description of the issue\"\n")
-	sb.WriteString("    }\n")
-	sb.WriteString("  ],\n")
-	sb.WriteString("  \"recommendation\": \"Full recommendation text explaining your verdict\"\n")
-	sb.WriteString("}\n\n")
-	sb.WriteString("Rules:\n")
-	sb.WriteString("- If there are any MAJOR findings → verdict must be REQUEST_CHANGES\n")
-	sb.WriteString("- If there are no MAJOR findings → verdict should be APPROVE\n")
-	sb.WriteString("- If CI has failed → verdict must be REQUEST_CHANGES with a finding noting the CI failure\n")
-	sb.WriteString("- Only report findings within your focus areas. Ignore everything else.\n")
+	sb.WriteString(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")
 

review/persona_test.go

@@ -87,26 +87,22 @@ func TestListBuiltinPersonas(t *testing.T) {
 	}
 }
 
-func TestLoadPersonaFromYAMLFile(t *testing.T) {
+func TestLoadPersonaFromJSONFile(t *testing.T) {
 	dir := t.TempDir()
-	path := filepath.Join(dir, "test.yaml")
+	path := filepath.Join(dir, "test.json")
 
-	content := `
-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
-`
+	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)
@@ -131,59 +127,25 @@ severity:
 	}
 }
 
-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.",
-		"focus": ["testing"],
-		"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")
-	}
-}
-
 func TestLoadPersonaValidation(t *testing.T) {
 	tests := []struct {
 		name    string
-		yaml    string
+		json    string
 		wantErr string
 	}{
 		{
 			name:    "missing name",
-			yaml:    "identity: test",
+			json:    `{"identity": "test"}`,
 			wantErr: "name is required",
 		},
 		{
 			name:    "missing identity",
-			yaml:    "name: test",
+			json:    `{"name": "test"}`,
 			wantErr: "identity is required",
 		},
 		{
 			name: "display_name defaults to name",
-			yaml: "name: test\nidentity: test identity",
+			json: `{"name": "test", "identity": "test identity"}`,
 			// No error expected - should succeed
 		},
 	}
@@ -191,8 +153,8 @@ func TestLoadPersonaValidation(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			dir := t.TempDir()
-			path := filepath.Join(dir, "test.yaml")
-			if err := os.WriteFile(path, []byte(tt.yaml), 0644); err != nil {
+			path := filepath.Join(dir, "test.json")
+			if err := os.WriteFile(path, []byte(tt.json), 0644); err != nil {
 				t.Fatalf("failed to write test file: %v", err)
 			}
 
@@ -222,21 +184,56 @@ func TestLoadPersonaValidation(t *testing.T) {
 }
 
 func TestLoadPersonaFileNotFound(t *testing.T) {
-	_, err := LoadPersona("/nonexistent/path/persona.yaml")
+	_, err := LoadPersona("/nonexistent/path/persona.json")
 	if err == nil {
 		t.Error("expected error for nonexistent file")
 	}
 }
 
-func TestLoadPersonaInvalidYAML(t *testing.T) {
+func TestLoadPersonaInvalidJSON(t *testing.T) {
 	dir := t.TempDir()
-	path := filepath.Join(dir, "invalid.yaml")
-	if err := os.WriteFile(path, []byte("not: valid: yaml: here"), 0644); err != nil {
+	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 YAML")
+		t.Error("expected error for invalid JSON")
+	}
+}
+
+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")
 	}
 }

review/personas/architect.json

@@ -0,0 +1,26 @@
+{
+  "name": "architect",
+  "display_name": "Software Architect",
+  "identity": "You are a software architect reviewing code for design quality.\n\nYour expertise:\n- Design patterns and anti-patterns\n- Code organization and module boundaries\n- API design and contracts\n- Testability and dependency injection\n- Consistency with existing architecture\n- 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/architect.yaml

@@ -1,34 +0,0 @@
-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.json

@@ -0,0 +1,26 @@
+{
+  "name": "docs",
+  "display_name": "Documentation Reviewer",
+  "identity": "You are a documentation specialist reviewing code for clarity and documentation quality.\n\nYour expertise:\n- API documentation and examples\n- Code comments and their accuracy\n- Error message clarity\n- README and guide quality\n- 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/docs.yaml

@@ -1,33 +0,0 @@
-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.json

@@ -0,0 +1,26 @@
+{
+  "name": "security",
+  "display_name": "Security Specialist",
+  "identity": "You are a security specialist reviewing code for vulnerabilities.\n\nYour expertise:\n- OWASP Top 10 vulnerabilities\n- Injection attacks (SQL, command, path traversal, template)\n- Authentication and authorization patterns\n- Secrets management and exposure risks\n- Race conditions with security implications\n- 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/personas/security.yaml

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

review/prompt.go

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