fix: address review findings (body truncation, unused field, whitespace)

Self-review fixes for PR #54:

- Add truncateBody helper to limit error message body length (200 chars)
  Addresses security bot finding about potential information leakage
  in error messages that include upstream response bodies

- Remove unused deployment.ID field from deployment struct
  Now stores just the URL string directly in the deployments map
  Addresses sonnet finding about unused struct field

- Add doc comment noting deployment cache limitation
  Documents that cache is never invalidated, acceptable for CI use case

- Fix trailing whitespace in action.yml aicore-resource-group default

All existing tests pass.

.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'

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.
+//
+// Note: 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. See issue #54 review discussion.
 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
+	deployments map[string]string // model name -> deployment URL
-}
-
-type deployment struct {
-	ID  string
-	URL string
 }
 
 // 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 {
@@ -135,9 +147,9 @@ 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) {
 	c.mu.RLock()
-	if d, ok := c.deployments[model]; ok {
+	if url, ok := c.deployments[model]; ok {
 		c.mu.RUnlock()
-		return d.URL, nil
+		return url, nil
 	}
 	c.mu.RUnlock()
 
@@ -151,16 +163,16 @@ func (c *AICoreClient) getDeploymentURL(ctx context.Context, model string) (stri
 	defer c.mu.Unlock()
 
 	// Double-check after acquiring write lock
-	if d, ok := c.deployments[model]; ok {
+	if url, ok := c.deployments[model]; ok {
-		return d.URL, nil
+		return url, nil
 	}
 
 	if err := c.fetchDeployments(ctx, token); err != nil {
 		return "", err
 	}
 
-	if d, ok := c.deployments[model]; ok {
+	if url, ok := c.deployments[model]; ok {
-		return d.URL, nil
+		return url, nil
 	}
 	return "", fmt.Errorf("no deployment found for model %q", model)
 }
@@ -186,12 +198,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 +228,7 @@ func (c *AICoreClient) fetchDeployments(ctx context.Context, token string) error
 		if modelName == "" {
 			continue
 		}
-		c.deployments[modelName] = deployment{
+		c.deployments[modelName] = r.DeploymentURL
-			ID:  r.ID,
-			URL: r.DeploymentURL,
-		}
 	}
 
 	return nil
@@ -290,7 +298,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
@@ -360,7 +368,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