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
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:
+30
-22
@@ -17,6 +17,10 @@ import (
|
|||||||
// Update this when SAP AI Core releases a new stable version.
|
// Update this when SAP AI Core releases a new stable version.
|
||||||
const AICoreOpenAIAPIVersion = "2024-12-01-preview"
|
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.
|
// AICoreConfig holds SAP AI Core authentication and connection settings.
|
||||||
type AICoreConfig struct {
|
type AICoreConfig struct {
|
||||||
ClientID string
|
ClientID string
|
||||||
@@ -28,6 +32,10 @@ type AICoreConfig struct {
|
|||||||
|
|
||||||
// AICoreClient wraps AI Core authentication and deployment discovery.
|
// AICoreClient wraps AI Core authentication and deployment discovery.
|
||||||
// Thread-safe for concurrent use after construction.
|
// 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 {
|
type AICoreClient struct {
|
||||||
config AICoreConfig
|
config AICoreConfig
|
||||||
http *http.Client
|
http *http.Client
|
||||||
@@ -35,12 +43,7 @@ type AICoreClient struct {
|
|||||||
mu sync.RWMutex
|
mu sync.RWMutex
|
||||||
token string
|
token string
|
||||||
tokenExpiry time.Time
|
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.
|
// NewAICoreClient creates a new AI Core client with the given configuration.
|
||||||
@@ -49,7 +52,7 @@ func NewAICoreClient(cfg AICoreConfig) *AICoreClient {
|
|||||||
return &AICoreClient{
|
return &AICoreClient{
|
||||||
config: cfg,
|
config: cfg,
|
||||||
http: &http.Client{Timeout: 5 * time.Minute},
|
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
|
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.
|
// getToken returns a valid OAuth token, refreshing if necessary.
|
||||||
func (c *AICoreClient) getToken(ctx context.Context) (string, error) {
|
func (c *AICoreClient) getToken(ctx context.Context) (string, error) {
|
||||||
c.mu.RLock()
|
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 {
|
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 {
|
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.
|
// getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.
|
||||||
func (c *AICoreClient) getDeploymentURL(ctx context.Context, model string) (string, error) {
|
func (c *AICoreClient) getDeploymentURL(ctx context.Context, model string) (string, error) {
|
||||||
c.mu.RLock()
|
c.mu.RLock()
|
||||||
if d, ok := c.deployments[model]; ok {
|
if url, ok := c.deployments[model]; ok {
|
||||||
c.mu.RUnlock()
|
c.mu.RUnlock()
|
||||||
return d.URL, nil
|
return url, nil
|
||||||
}
|
}
|
||||||
c.mu.RUnlock()
|
c.mu.RUnlock()
|
||||||
|
|
||||||
@@ -151,16 +163,16 @@ func (c *AICoreClient) getDeploymentURL(ctx context.Context, model string) (stri
|
|||||||
defer c.mu.Unlock()
|
defer c.mu.Unlock()
|
||||||
|
|
||||||
// Double-check after acquiring write lock
|
// 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 {
|
if err := c.fetchDeployments(ctx, token); err != nil {
|
||||||
return "", err
|
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)
|
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 {
|
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 {
|
var deployResp struct {
|
||||||
Resources []struct {
|
Resources []struct {
|
||||||
ID string `json:"id"`
|
|
||||||
DeploymentURL string `json:"deploymentUrl"`
|
DeploymentURL string `json:"deploymentUrl"`
|
||||||
Status string `json:"status"`
|
Status string `json:"status"`
|
||||||
Details struct {
|
Details struct {
|
||||||
@@ -217,10 +228,7 @@ func (c *AICoreClient) fetchDeployments(ctx context.Context, token string) error
|
|||||||
if modelName == "" {
|
if modelName == "" {
|
||||||
continue
|
continue
|
||||||
}
|
}
|
||||||
c.deployments[modelName] = deployment{
|
c.deployments[modelName] = r.DeploymentURL
|
||||||
ID: r.ID,
|
|
||||||
URL: r.DeploymentURL,
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
|
|
||||||
return nil
|
return nil
|
||||||
@@ -290,7 +298,7 @@ func (c *AICoreClient) CompleteAnthropic(ctx context.Context, model string, mess
|
|||||||
}
|
}
|
||||||
|
|
||||||
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
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
|
var anthropicResp anthropicResponse
|
||||||
@@ -360,7 +368,7 @@ func (c *AICoreClient) CompleteOpenAI(ctx context.Context, model string, message
|
|||||||
}
|
}
|
||||||
|
|
||||||
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
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
|
var openaiResp ChatResponse
|
||||||
|
|||||||
Reference in New Issue
Block a user