From 5132a2028df26fd1c27f1a095e92ee21473c7b0f Mon Sep 17 00:00:00 2001 From: Rodin Date: Sun, 10 May 2026 01:26:13 -0700 Subject: [PATCH] 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 | 2 +- llm/aicore.go | 52 ++++++++++++++++++-------------- 2 files changed, 31 insertions(+), 23 deletions(-) diff --git a/.gitea/actions/review/action.yml b/.gitea/actions/review/action.yml index 655515c..feb64e0 100644 --- a/.gitea/actions/review/action.yml +++ b/.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 diff --git a/llm/aicore.go b/llm/aicore.go index ed58d43..9859e07 100644 --- a/llm/aicore.go +++ b/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 -} - -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