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

@@ -59,7 +59,7 @@ inputs:
   aicore-resource-group:
     description: 'SAP AI Core resource group (default: default)'
     required: false
-    default: 'default' 
+    default: 'default'
   conventions-file:
     description: 'Path to conventions file in the repo (e.g. CLAUDE.md)'
     required: false

.gitea/workflows/ci.yml

@@ -19,7 +19,9 @@ jobs:
       - run: go build -o review-bot ./cmd/review-bot
 
   # Self-review using native SAP AI Core provider
-  # Models must match SAP AI Core deployments (use 'anthropic--' prefix for Claude)
+  # 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'

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

@@ -0,0 +1,40 @@
+name: PR Ready Gate
+
+on:
+  pull_request:
+    types: [synchronize]
+
+jobs:
+  clear-labels:
+    runs-on: ubuntu-24.04
+    if: |
+      contains(github.event.pull_request.labels.*.name, 'ready') ||
+      contains(github.event.pull_request.labels.*.name, 'self-reviewed')
+    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}"

TODO.md

@@ -1,19 +0,0 @@
-## Self-Review: feat/aicore-provider — 2026-05-09
-
-### Verdict: PASS
-
-No blocking issues found — ready for human review.
-
-#### Notes (informational, not blocking)
-
-**[fit]** `staticcheck` reports:
-- `llm/aicore.go:237` and `llm/client.go:231`: struct literal conversion style (S1016) — minor style nit, existing in both old and new code
-- `gitea/diff.go:78`: HasPrefix return ignored (SA4017) — pre-existing, not introduced by this PR
-- `cmd/review-bot/main_test.go:347`: nil Context (SA1012) — pre-existing, not introduced by this PR
-
-**[fit]** Body length validation: `aicore.go` does not include the Content-Length vs body length validation that `doRequest` has in `client.go`. This is acceptable because:
-1. AI Core uses OAuth tokens which are short-lived, so truncation is less likely
-2. The retry logic still applies via "read response" error pattern
-3. Adding complexity to aicore.go for an edge case that hasn't manifested is premature
-
-**[completeness]** Tests pass (go test ./...), go vet clean, no uncommitted changes.

cmd/review-bot/main.go

@@ -340,6 +340,24 @@ func main() {
 
 	sentinel := fmt.Sprintf("<!-- review-bot:%s -->", *reviewerName)
 
+	// Stale check: verify HEAD hasn't moved since we started
+	evaluatedSHA := pr.Head.Sha
+	var currentSHA string
+	currentPR, err := giteaClient.GetPullRequest(ctx, owner, repoName, prNumber)
+	if err != nil {
+		slog.Warn("could not re-fetch PR for stale check", "pr", prNumber, "error", err)
+		// currentSHA stays empty — shouldSkipStaleReview will return false
+	} else {
+		currentSHA = currentPR.Head.Sha
+	}
+	if shouldSkipStaleReview(evaluatedSHA, currentSHA) {
+		slog.Warn("HEAD moved during review — skipping stale review",
+			"evaluated", evaluatedSHA,
+			"current", currentSHA,
+			"pr", prNumber)
+		return
+	}
+
 	// Map findings to inline comments for lines present in the diff
 	diffRanges := gitea.ParseDiffNewLines(diff)
 	var inlineComments []gitea.ReviewComment
@@ -691,3 +709,16 @@ 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
+}

cmd/review-bot/main_test.go

@@ -862,3 +862,53 @@ func TestFindAllOwnReviews(t *testing.T) {
 		}
 	}
 }
+
+func TestShouldSkipStaleReview(t *testing.T) {
+	tests := []struct {
+		name         string
+		evaluatedSHA string
+		currentSHA   string
+		wantSkip     bool
+	}{
+		{
+			name:         "matching SHAs",
+			evaluatedSHA: "abc123def456",
+			currentSHA:   "abc123def456",
+			wantSkip:     false,
+		},
+		{
+			name:         "different SHAs",
+			evaluatedSHA: "abc123def456",
+			currentSHA:   "xyz789abc123",
+			wantSkip:     true,
+		},
+		{
+			name:         "empty current SHA (re-fetch failed)",
+			evaluatedSHA: "abc123def456",
+			currentSHA:   "",
+			wantSkip:     false,
+		},
+		{
+			name:         "both empty (edge case)",
+			evaluatedSHA: "",
+			currentSHA:   "",
+			wantSkip:     false,
+		},
+		{
+			name:         "only current empty",
+			evaluatedSHA: "abc123",
+			currentSHA:   "",
+			wantSkip:     false,
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := shouldSkipStaleReview(tc.evaluatedSHA, tc.currentSHA)
+			if got != tc.wantSkip {
+				t.Errorf("shouldSkipStaleReview(%q, %q) = %v, want %v",
+					tc.evaluatedSHA, tc.currentSHA, got, tc.wantSkip)
+			}
+		})
+	}
+}

llm/aicore.go

@@ -17,6 +17,10 @@ import (
 // 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
@@ -28,6 +32,10 @@ type AICoreConfig struct {
 
 // 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
@@ -35,12 +43,7 @@ type AICoreClient struct {
 	mu          sync.RWMutex
 	token       string
 	tokenExpiry time.Time
-	deployments map[string]deployment // model name -> deployment info
-}
-
-type deployment struct {
-	ID  string
-	URL string
+	deployments map[string]string // model name -> deployment URL
 }
 
 // NewAICoreClient creates a new AI Core client with the given configuration.
@@ -49,7 +52,7 @@ func NewAICoreClient(cfg AICoreConfig) *AICoreClient {
 	return &AICoreClient{
 		config:      cfg,
 		http:        &http.Client{Timeout: 5 * time.Minute},
-		deployments: make(map[string]deployment),
+		deployments: make(map[string]string),
 	}
 }
 
@@ -60,6 +63,15 @@ func (c *AICoreClient) WithTimeout(d time.Duration) *AICoreClient {
 	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()
@@ -113,7 +125,7 @@ func (c *AICoreClient) fetchToken(ctx context.Context) (string, time.Time, error
 	}
 
 	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		return "", time.Time{}, fmt.Errorf("token request failed (status %d): %s", resp.StatusCode, string(body))
+		return "", time.Time{}, fmt.Errorf("token request failed (status %d): %s", resp.StatusCode, truncateBody(body))
 	}
 
 	var tokenResp struct {
@@ -133,36 +145,48 @@ func (c *AICoreClient) fetchToken(ctx context.Context) (string, time.Time, error
 }
 
 // getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.
-func (c *AICoreClient) getDeploymentURL(ctx context.Context, model string) (string, error) {
+// 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 d, ok := c.deployments[model]; ok {
+	if u, ok := c.deployments[model]; ok {
 		c.mu.RUnlock()
-		return d.URL, nil
+		// 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 deadlock)
-	token, err := c.getToken(ctx)
+	// 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)
+		return "", "", fmt.Errorf("get token for deployments: %w", err)
 	}
 
 	c.mu.Lock()
 	defer c.mu.Unlock()
 
 	// Double-check after acquiring write lock
-	if d, ok := c.deployments[model]; ok {
-		return d.URL, nil
+	if u, ok := c.deployments[model]; ok {
+		return u, token, nil
 	}
 
 	if err := c.fetchDeployments(ctx, token); err != nil {
-		return "", err
+		return "", "", err
 	}
 
-	if d, ok := c.deployments[model]; ok {
-		return d.URL, nil
+	if u, ok := c.deployments[model]; ok {
+		return u, token, nil
 	}
-	return "", fmt.Errorf("no deployment found for model %q", model)
+	return "", "", fmt.Errorf("no deployment found for model %q", model)
 }
 
 func (c *AICoreClient) fetchDeployments(ctx context.Context, token string) error {
@@ -186,12 +210,11 @@ func (c *AICoreClient) fetchDeployments(ctx context.Context, token string) error
 	}
 
 	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		return fmt.Errorf("deployments request failed (status %d): %s", resp.StatusCode, string(body))
+		return fmt.Errorf("deployments request failed (status %d): %s", resp.StatusCode, truncateBody(body))
 	}
 
 	var deployResp struct {
 		Resources []struct {
-			ID            string `json:"id"`
 			DeploymentURL string `json:"deploymentUrl"`
 			Status        string `json:"status"`
 			Details       struct {
@@ -217,10 +240,7 @@ func (c *AICoreClient) fetchDeployments(ctx context.Context, token string) error
 		if modelName == "" {
 			continue
 		}
-		c.deployments[modelName] = deployment{
-			ID:  r.ID,
-			URL: r.DeploymentURL,
-		}
+		c.deployments[modelName] = r.DeploymentURL
 	}
 
 	return nil
@@ -228,12 +248,7 @@ func (c *AICoreClient) fetchDeployments(ctx context.Context, token string) error
 
 // 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, err := c.getDeploymentURL(ctx, model)
-	if err != nil {
-		return "", err
-	}
-
-	token, err := c.getToken(ctx)
+	deployURL, token, err := c.getDeploymentURL(ctx, model)
 	if err != nil {
 		return "", err
 	}
@@ -290,7 +305,7 @@ func (c *AICoreClient) CompleteAnthropic(ctx context.Context, model string, mess
 	}
 
 	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		return "", fmt.Errorf("AI Core API error (status %d): %s", resp.StatusCode, string(body))
+		return "", fmt.Errorf("AI Core API error (status %d): %s", resp.StatusCode, truncateBody(body))
 	}
 
 	var anthropicResp anthropicResponse
@@ -317,12 +332,7 @@ func (c *AICoreClient) CompleteAnthropic(ctx context.Context, model string, mess
 
 // 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, err := c.getDeploymentURL(ctx, model)
-	if err != nil {
-		return "", err
-	}
-
-	token, err := c.getToken(ctx)
+	deployURL, token, err := c.getDeploymentURL(ctx, model)
 	if err != nil {
 		return "", err
 	}
@@ -360,7 +370,7 @@ func (c *AICoreClient) CompleteOpenAI(ctx context.Context, model string, message
 	}
 
 	if resp.StatusCode < 200 || resp.StatusCode >= 300 {
-		return "", fmt.Errorf("AI Core API error (status %d): %s", resp.StatusCode, string(body))
+		return "", fmt.Errorf("AI Core API error (status %d): %s", resp.StatusCode, truncateBody(body))
 	}
 
 	var openaiResp ChatResponse

llm/aicore_test.go

@@ -142,7 +142,7 @@ func TestAICoreClient_DeploymentFetch(t *testing.T) {
 	})
 
 	// Should find running deployment
-	url, err := client.getDeploymentURL(context.Background(), "anthropic--claude-4.6-sonnet")
+	url, _, err := client.getDeploymentURL(context.Background(), "anthropic--claude-4.6-sonnet")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -151,7 +151,7 @@ func TestAICoreClient_DeploymentFetch(t *testing.T) {
 	}
 
 	// Should find running gpt-5, not stopped one
-	url, err = client.getDeploymentURL(context.Background(), "gpt-5")
+	url, _, err = client.getDeploymentURL(context.Background(), "gpt-5")
 	if err != nil {
 		t.Fatalf("unexpected error: %v", err)
 	}
@@ -160,14 +160,14 @@ func TestAICoreClient_DeploymentFetch(t *testing.T) {
 	}
 
 	// Should error on unknown model
-	_, err = client.getDeploymentURL(context.Background(), "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) {
-	// Use a pointer to capture the server URL for use in the handler
+	// 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) {