fix: address review findings (body truncation, unused field, whitespace)
CI / test (pull_request) Successful in 14s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 37s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 1m42s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 2m13s

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.
This commit is contained in:
Rodin
2026-05-10 01:26:13 -07:00
parent 6d4c33a7a4
commit 327649606b
2 changed files with 31 additions and 23 deletions
+30 -22
View File
@@ -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
}
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 {
@@ -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 {
return d.URL, nil
if url, ok := c.deployments[model]; ok {
return url, nil
}
if err := c.fetchDeployments(ctx, token); err != nil {
return "", err
}
if d, ok := c.deployments[model]; ok {
return d.URL, nil
if url, ok := c.deployments[model]; ok {
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{
ID: r.ID,
URL: r.DeploymentURL,
}
c.deployments[modelName] = 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