fix: address review findings (body truncation, unused field, whitespace)
CI / test (pull_request) Successful in 15s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 32s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m34s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m28s
CI / test (pull_request) Successful in 15s
CI / review (anthropic--claude-4.6-sonnet, sonnet, SONNET_REVIEW_TOKEN) (pull_request) Successful in 32s
CI / review (gpt-5, gpt, GPT_REVIEW_TOKEN) (pull_request) Successful in 1m34s
CI / review (gpt-5, security, SECURITY_REVIEW.md, SECURITY_REVIEW_TOKEN) (pull_request) Successful in 2m28s
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:
@@ -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
|
||||
|
||||
+30
-22
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user