feat: native SAP AI Core support #54

Merged
rodin merged 1 commits from feat/aicore-provider-v2 into main 2026-05-10 20:03:32 +00:00
7 changed files with 1070 additions and 33 deletions
+33 -6
View File
@@ -26,18 +26,40 @@ inputs:
required: false
default: ''
llm-base-url:
description: 'OpenAI-compatible LLM API base URL'
required: true
description: 'OpenAI-compatible LLM API base URL (not required for aicore provider)'
required: false
default: ''
llm-api-key:
description: 'LLM API key'
required: true
description: 'LLM API key (not required for aicore provider)'
required: false
default: ''
llm-model:
description: 'LLM model name'
required: true
llm-provider:
description: 'LLM API provider: openai or anthropic (default openai)'
description: 'LLM API provider: openai, anthropic, or aicore (default openai)'
required: false
default: 'openai'
default: 'openai'
aicore-client-id:
description: 'SAP AI Core client ID (required for aicore provider)'
required: false
default: ''
aicore-client-secret:
description: 'SAP AI Core client secret (required for aicore provider)'
required: false
default: ''
aicore-auth-url:
description: 'SAP AI Core authentication URL (required for aicore provider)'
required: false
default: ''
aicore-api-url:
description: 'SAP AI Core API URL (required for aicore provider)'
required: false
default: ''
aicore-resource-group:
description: 'SAP AI Core resource group (default: default)'
required: false
default: 'default'
Outdated
Review

[NIT] Trailing whitespace on the default: 'default' line for aicore-resource-group. Minor but inconsistent with the rest of the file.

**[NIT]** Trailing whitespace on the `default: 'default' ` line for `aicore-resource-group`. Minor but inconsistent with the rest of the file.
conventions-file:
description: 'Path to conventions file in the repo (e.g. CLAUDE.md)'
required: false
Review

[NIT] Trailing space on the default: 'default' line (aicore-resource-group default value). Minor formatting inconsistency — matches the pre-existing similar trailing space on the original llm-provider default that was fixed in this PR.

**[NIT]** Trailing space on the `default: 'default' ` line (aicore-resource-group default value). Minor formatting inconsistency — matches the pre-existing similar trailing space on the original `llm-provider` default that was fixed in this PR.
@@ -165,6 +187,11 @@ runs:
SYSTEM_PROMPT_FILE: ${{ inputs.system-prompt-file }}
PERSONA: ${{ inputs.persona }}
PERSONA_FILE: ${{ inputs.persona-file }}
AICORE_CLIENT_ID: ${{ inputs.aicore-client-id }}
AICORE_CLIENT_SECRET: ${{ inputs.aicore-client-secret }}
AICORE_AUTH_URL: ${{ inputs.aicore-auth-url }}
AICORE_API_URL: ${{ inputs.aicore-api-url }}
AICORE_RESOURCE_GROUP: ${{ inputs.aicore-resource-group }}
run: |
ARGS=""
if [ "${{ inputs.dry-run }}" = "true" ]; then
+10 -11
View File
@@ -18,8 +18,10 @@ jobs:
- run: go vet ./...
- run: go build -o review-bot ./cmd/review-bot
# Self-review: builds from source since we're pre-release
# Models configured to match SAP AI Core deployments
# Self-review using native SAP AI Core provider
# 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'
1
@@ -29,18 +31,12 @@ jobs:
include:
- name: sonnet
token_secret: SONNET_REVIEW_TOKEN
provider: anthropic
llm_path: /anthropic/v1
model: anthropic--claude-4.6-sonnet
- name: gpt
token_secret: GPT_REVIEW_TOKEN
provider: openai
llm_path: /openai/v1
model: gpt-5
- name: security
token_secret: SECURITY_REVIEW_TOKEN
provider: openai
llm_path: /openai/v1
model: gpt-5
system_prompt_file: SECURITY_REVIEW.md
steps:
@@ -56,10 +52,13 @@ jobs:
PR_NUMBER: ${{ github.event.pull_request.number }}
REVIEWER_TOKEN: ${{ secrets[matrix.token_secret] }}
REVIEWER_NAME: ${{ matrix.name }}
LLM_BASE_URL: ${{ secrets.LLM_BASE_URL }}${{ matrix.llm_path }}
LLM_API_KEY: ${{ secrets.LLM_API_KEY }}
LLM_PROVIDER: aicore
LLM_MODEL: ${{ matrix.model }}
LLM_PROVIDER: ${{ matrix.provider }}
AICORE_CLIENT_ID: ${{ secrets.AICORE_CLIENT_ID }}
AICORE_CLIENT_SECRET: ${{ secrets.AICORE_CLIENT_SECRET }}
AICORE_AUTH_URL: ${{ secrets.AICORE_AUTH_URL }}
AICORE_API_URL: ${{ secrets.AICORE_API_URL }}
AICORE_RESOURCE_GROUP: ${{ secrets.AICORE_RESOURCE_GROUP }}
CONVENTIONS_FILE: "CONVENTIONS.md"
PATTERNS_REPO: "rodin/go-patterns"
PATTERNS_FILES: "README.md,patterns/"
+32 -4
View File
@@ -4,7 +4,7 @@ AI-powered code review bot for Gitea pull requests. Fetches diff + context, send
## Features
- **Multi-provider**: OpenAI-compatible and Anthropic Messages API
- **Multi-provider**: OpenAI-compatible, Anthropic Messages API, and SAP AI Core
- **Context-aware**: Fetches full file content, conventions, language patterns, CI status
- **Smart budget**: Automatically trims context to fit model token limits
- **Idempotent reviews**: Posts new review, then cleans up stale ones (one review per bot)
@@ -168,16 +168,41 @@ Prints the review to CI logs without posting to the PR. Useful for testing promp
llm-provider: anthropic
```
### Using SAP AI Core
For SAP environments with AI Core deployments, use the `aicore` provider for native authentication:
```yaml
- uses: https://gitea.weiker.me/rodin/review-bot/.gitea/actions/review@v0.1.0
with:
reviewer-token: ${{ secrets.REVIEW_TOKEN }}
reviewer-name: aicore-review
llm-model: anthropic--claude-4.6-sonnet # or gpt-5
llm-provider: aicore
aicore-client-id: ${{ secrets.AICORE_CLIENT_ID }}
aicore-client-secret: ${{ secrets.AICORE_CLIENT_SECRET }}
aicore-auth-url: ${{ secrets.AICORE_AUTH_URL }}
aicore-api-url: ${{ secrets.AICORE_API_URL }}
aicore-resource-group: default
```
AI Core handles OAuth token management and deployment discovery automatically. Model names must match the deployment name in AI Core (e.g. `anthropic--claude-4.6-sonnet`, `gpt-5`).
## Action Inputs
| Input | Required | Default | Description |
|-------|----------|---------|-------------|
| `reviewer-token` | Yes | — | Gitea token for posting reviews (needs `write:issue`, `write:repository`) |
| `reviewer-name` | No | `""` | Logical identity for this reviewer. Used as sentinel for idempotent cleanup. Set this when running multiple review bots on the same PR. |
| `llm-base-url` | Yes | — | LLM API base URL |
| `llm-api-key` | Yes | — | LLM API key |
| `llm-base-url` | No* | `""` | LLM API base URL (required unless using aicore provider) |
| `llm-api-key` | No* | `""` | LLM API key (required unless using aicore provider) |
| `llm-model` | Yes | — | Model name |
| `llm-provider` | No | `openai` | API provider: `openai` or `anthropic` |
| `llm-provider` | No | `openai` | API provider: `openai`, `anthropic`, or `aicore` |
| `aicore-client-id` | No** | `""` | SAP AI Core client ID |
| `aicore-client-secret` | No** | `""` | SAP AI Core client secret |
| `aicore-auth-url` | No** | `""` | SAP AI Core authentication URL |
| `aicore-api-url` | No** | `""` | SAP AI Core API URL |
| `aicore-resource-group` | No | `default` | SAP AI Core resource group |
| `conventions-file` | No | `""` | Path to coding conventions file in the repo |
| `patterns-repo` | No | `""` | Comma-separated repos with language patterns (e.g. `rodin/go-patterns`) |
| `patterns-files` | No | `README.md` | Files/directories to fetch from pattern repos |
@@ -190,6 +215,9 @@ Prints the review to CI logs without posting to the PR. Useful for testing promp
| `update-existing` | No | `true` | Delete previous review from same bot before posting. Accepts: true/1/yes or false/0/no |
| `version` | No | `latest` | review-bot version to install |
*Required for `openai` and `anthropic` providers, not for `aicore`.
**Required only for `aicore` provider.
## Runner Requirements
The composite action requires these tools on the runner:
+31 -5
View File
@@ -69,10 +69,17 @@ func main() {
dryRun := flag.Bool("dry-run", false, "Print review to stdout instead of posting")
llmTemp := flag.Float64("llm-temperature", envOrDefaultFloat("LLM_TEMPERATURE", 0), "LLM temperature (0 = server default)")
llmTimeout := flag.Int("llm-timeout", envOrDefaultInt("LLM_TIMEOUT", 300), "LLM request timeout in seconds (default 300)")
llmProvider := flag.String("llm-provider", envOrDefault("LLM_PROVIDER", "openai"), "LLM API provider: openai or anthropic")
llmProvider := flag.String("llm-provider", envOrDefault("LLM_PROVIDER", "openai"), "LLM API provider: openai, anthropic, or aicore")
personaName := flag.String("persona", envOrDefault("PERSONA", ""), "Built-in persona name (security, architect, docs)")
personaFile := flag.String("persona-file", envOrDefault("PERSONA_FILE", ""), "Path to persona JSON file")
// AI Core specific flags (only used when provider=aicore)
aicoreClientID := flag.String("aicore-client-id", envOrDefault("AICORE_CLIENT_ID", ""), "SAP AI Core client ID (for provider=aicore)")
aicoreClientSecret := flag.String("aicore-client-secret", envOrDefault("AICORE_CLIENT_SECRET", ""), "SAP AI Core client secret (for provider=aicore)")
aicoreAuthURL := flag.String("aicore-auth-url", envOrDefault("AICORE_AUTH_URL", ""), "SAP AI Core auth URL (for provider=aicore)")
aicoreAPIURL := flag.String("aicore-api-url", envOrDefault("AICORE_API_URL", ""), "SAP AI Core API URL (for provider=aicore)")
Review

[MINOR] flag.Parse() is invoked twice; the extra call is redundant and can be removed to avoid confusion.

**[MINOR]** flag.Parse() is invoked twice; the extra call is redundant and can be removed to avoid confusion.
aicoreResourceGroup := flag.String("aicore-resource-group", envOrDefault("AICORE_RESOURCE_GROUP", "default"), "SAP AI Core resource group (for provider=aicore)")
flag.Parse()
flag.Parse()
if *versionFlag {
1
@@ -86,10 +93,20 @@ func main() {
slog.Info("review-bot starting", "version", version)
// Validate required fields
if *giteaURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" ||
*llmBaseURL == "" || *llmAPIKey == "" || *llmModel == "" {
// For aicore provider, llm-base-url and llm-api-key are not required
isAICore := llm.Provider(*llmProvider) == llm.ProviderAICore
Outdated
Review

[NIT] In aicore mode, llm.NewClient is constructed with potentially empty baseURL/apiKey. While harmless since the provider switches to AI Core, consider a small comment explaining this or a helper constructor to avoid confusion.

**[NIT]** In aicore mode, llm.NewClient is constructed with potentially empty baseURL/apiKey. While harmless since the provider switches to AI Core, consider a small comment explaining this or a helper constructor to avoid confusion.
if *giteaURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" || *llmModel == "" {
fmt.Fprintf(os.Stderr, "Error: missing required flags or environment variables\n\n")
fmt.Fprintf(os.Stderr, "Required: --gitea-url, --repo, --pr, --reviewer-token, --llm-base-url, --llm-api-key, --llm-model\n")
fmt.Fprintf(os.Stderr, "Required: --gitea-url, --repo, --pr, --reviewer-token, --llm-model\n")
os.Exit(1)
}
if !isAICore && (*llmBaseURL == "" || *llmAPIKey == "") {
fmt.Fprintf(os.Stderr, "Error: --llm-base-url and --llm-api-key are required for provider=%s\n", *llmProvider)
os.Exit(1)
}
if isAICore && (*aicoreClientID == "" || *aicoreClientSecret == "" || *aicoreAuthURL == "" || *aicoreAPIURL == "") {
fmt.Fprintf(os.Stderr, "Error: AI Core credentials required for provider=aicore\n\n")
fmt.Fprintf(os.Stderr, "Required: --aicore-client-id, --aicore-client-secret, --aicore-auth-url, --aicore-api-url\n")
os.Exit(1)
}
1
@@ -157,8 +174,17 @@ func main() {
switch llm.Provider(*llmProvider) {
case llm.ProviderOpenAI, llm.ProviderAnthropic:
llmClient.WithProvider(llm.Provider(*llmProvider))
case llm.ProviderAICore:
llmClient.WithAICore(llm.AICoreConfig{
ClientID: *aicoreClientID,
ClientSecret: *aicoreClientSecret,
AuthURL: *aicoreAuthURL,
APIURL: *aicoreAPIURL,
ResourceGroup: *aicoreResourceGroup,
})
slog.Info("using SAP AI Core provider", "resource_group", *aicoreResourceGroup)
default:
slog.Error("invalid LLM provider", "provider", *llmProvider, "valid", "openai, anthropic")
slog.Error("invalid LLM provider", "provider", *llmProvider, "valid", "openai, anthropic, aicore")
os.Exit(1)
}
if *llmTimeout > 0 {
+391
View File
@@ -0,0 +1,391 @@
package llm
Outdated
Review

[MINOR] AI Core endpoints (AuthURL/APIURL) are used as provided without enforcing HTTPS. Consider validating the URL scheme to be https before sending client credentials or tokens to prevent accidental plaintext transmission due to misconfiguration.

**[MINOR]** AI Core endpoints (AuthURL/APIURL) are used as provided without enforcing HTTPS. Consider validating the URL scheme to be https before sending client credentials or tokens to prevent accidental plaintext transmission due to misconfiguration.
Review

[NIT] The maxErrorBodyLen constant is unexported and only used within aicore.go, but truncateBody is also unexported. Both are fine as-is, but maxErrorBodyLen = 200 is a magic number without a unit comment. Consider renaming to maxErrorBodyBytes or adding // bytes in the comment for clarity.

**[NIT]** The `maxErrorBodyLen` constant is unexported and only used within `aicore.go`, but `truncateBody` is also unexported. Both are fine as-is, but `maxErrorBodyLen = 200` is a magic number without a unit comment. Consider renaming to `maxErrorBodyBytes` or adding `// bytes` in the comment for clarity.
import (
"bytes"
"context"
"encoding/json"
"fmt"
"io"
"net/http"
"net/url"
"strings"
"sync"
"time"
)
// AICoreOpenAIAPIVersion is the API version used for OpenAI models through AI Core.
// 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
Outdated
Review

[MINOR] AICore endpoints (AuthURL/APIURL) are taken as-is without validating scheme; consider enforcing HTTPS to avoid accidental plaintext credential transmission or SSRF to non-HTTPS endpoints.

**[MINOR]** AICore endpoints (AuthURL/APIURL) are taken as-is without validating scheme; consider enforcing HTTPS to avoid accidental plaintext credential transmission or SSRF to non-HTTPS endpoints.
// 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
ClientSecret string
AuthURL string
APIURL string
ResourceGroup string
Outdated
Review

[NIT] The field name http shadows the net/http package name within the struct's methods. While Go resolves this correctly (the package is accessed via the import path, the field via the receiver), this is a common source of confusion. The existing llm/client.go uses the same pattern, so this is consistent with the codebase, but naming it httpClient would be clearer.

**[NIT]** The field name `http` shadows the `net/http` package name within the struct's methods. While Go resolves this correctly (the package is accessed via the import path, the field via the receiver), this is a common source of confusion. The existing `llm/client.go` uses the same pattern, so this is consistent with the codebase, but naming it `httpClient` would be clearer.
}
// AICoreClient wraps AI Core authentication and deployment discovery.
Outdated
Review

[NIT] The field name http shadows the net/http package import. While Go resolves this correctly (the field is accessed via the receiver, the package via the bare name), it's a minor readability concern. The existing llm/client.go uses the same pattern so it's consistent within the codebase, but something like httpClient would be clearer.

**[NIT]** The field name `http` shadows the `net/http` package import. While Go resolves this correctly (the field is accessed via the receiver, the package via the bare name), it's a minor readability concern. The existing `llm/client.go` uses the same pattern so it's consistent within the codebase, but something like `httpClient` would be clearer.
// Thread-safe for concurrent use after construction.
Outdated
Review

[NIT] The struct field http *http.Client shadows the imported net/http package name within the struct's method set. While Go resolves this correctly (field access via c.http.Do vs package via http.NewRequestWithContext), naming the field httpClient would be clearer and avoid any reader confusion.

**[NIT]** The struct field `http *http.Client` shadows the imported `net/http` package name within the struct's method set. While Go resolves this correctly (field access via `c.http.Do` vs package via `http.NewRequestWithContext`), naming the field `httpClient` would be clearer and avoid any reader confusion.
//
Outdated
Review

[NIT] The struct field http *http.Client shadows the stdlib package name http. While it compiles fine, it can confuse readers and linters. Consider renaming to httpClient *http.Client (consistent with how other Go stdlib code names embedded HTTP clients).

**[NIT]** The struct field `http *http.Client` shadows the stdlib package name `http`. While it compiles fine, it can confuse readers and linters. Consider renaming to `httpClient *http.Client` (consistent with how other Go stdlib code names embedded HTTP clients).
// Design: The deployment cache is populated once and never invalidated. This is
// acceptable for short-lived CI runner processes, but longer-lived deployments
Outdated
Review

[NIT] The deployment struct has an ID field that is stored but never read anywhere in the codebase. If it's not used for anything, it adds unused memory per deployment entry. Either use it (e.g., for logging) or remove it.

**[NIT]** The `deployment` struct has an `ID` field that is stored but never read anywhere in the codebase. If it's not used for anything, it adds unused memory per deployment entry. Either use it (e.g., for logging) or remove it.
// may want to add a TTL or re-fetch on errors.
type AICoreClient struct {
config AICoreConfig
http *http.Client
mu sync.RWMutex
token string
tokenExpiry time.Time
deployments map[string]string // model name -> deployment URL
}
Outdated
Review

[NIT] The AICoreClient struct doc comment references 'issue #54 review discussion' which is an internal reference that may not be accessible to all readers of the code. This is fine as a development note, but could be confusing in a public codebase.

**[NIT]** The `AICoreClient` struct doc comment references 'issue #54 review discussion' which is an internal reference that may not be accessible to all readers of the code. This is fine as a development note, but could be confusing in a public codebase.
// NewAICoreClient creates a new AI Core client with the given configuration.
// The client uses a default 5-minute timeout; use WithTimeout to customize.
func NewAICoreClient(cfg AICoreConfig) *AICoreClient {
return &AICoreClient{
Outdated
Review

[NIT] The field name http shadows the standard library package name net/http. Within the AICoreClient methods, http.MethodPost etc. still work because the package is imported separately, but this naming can confuse readers. The parent Client struct in client.go uses the same pattern, so it's consistent — but worth noting.

**[NIT]** The field name `http` shadows the standard library package name `net/http`. Within the `AICoreClient` methods, `http.MethodPost` etc. still work because the package is imported separately, but this naming can confuse readers. The parent `Client` struct in `client.go` uses the same pattern, so it's consistent — but worth noting.
config: cfg,
http: &http.Client{Timeout: 5 * time.Minute},
deployments: make(map[string]string),
}
}
Outdated
Review

[MINOR] The deployment struct has an ID field that is populated from the deployments API response but never used anywhere in the code. If it's genuinely unused, it adds noise; if it was intended for logging or future use, a comment explaining its purpose would help. Consider removing it or adding a comment.

**[MINOR]** The `deployment` struct has an `ID` field that is populated from the deployments API response but never used anywhere in the code. If it's genuinely unused, it adds noise; if it was intended for logging or future use, a comment explaining its purpose would help. Consider removing it or adding a comment.
Outdated
Review

[NIT] The field name http shadows the standard library package net/http. While this works because the package is only referenced by its qualified name within the struct methods, renaming to httpClient would avoid the potential confusion and better align with the pattern used elsewhere (e.g. c.http.Do).

**[NIT]** The field name `http` shadows the standard library package `net/http`. While this works because the package is only referenced by its qualified name within the struct methods, renaming to `httpClient` would avoid the potential confusion and better align with the pattern used elsewhere (e.g. `c.http.Do`).
Outdated
Review

[NIT] The field name http on AICoreClient (and Client) shadows the standard library package name net/http. While this compiles fine and is a common pattern in the stdlib itself (e.g. http.Client), it can be slightly surprising. Not blocking.

**[NIT]** The field name `http` on `AICoreClient` (and `Client`) shadows the standard library package name `net/http`. While this compiles fine and is a common pattern in the stdlib itself (e.g. `http.Client`), it can be slightly surprising. Not blocking.
// WithTimeout sets the HTTP request timeout for AI Core calls.
// This should be called during construction, before concurrent use.
func (c *AICoreClient) WithTimeout(d time.Duration) *AICoreClient {
c.http.Timeout = d
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 {
Outdated
Review

[MINOR] The deployment.ID field is stored but never used — only deployment.URL is referenced. This is dead storage. Either use it (e.g. in log messages for diagnostics) or remove the field and the struct altogether in favour of storing just the URL string.

**[MINOR]** The `deployment.ID` field is stored but never used — only `deployment.URL` is referenced. This is dead storage. Either use it (e.g. in log messages for diagnostics) or remove the field and the struct altogether in favour of storing just the URL 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()
if c.token != "" && time.Now().Add(5*time.Minute).Before(c.tokenExpiry) {
token := c.token
c.mu.RUnlock()
return token, nil
}
c.mu.RUnlock()
c.mu.Lock()
Outdated
Review

[MINOR] Auth and API endpoints are accepted as-is without enforcing HTTPS. Using plain HTTP would expose client credentials and tokens in transit. Consider validating scheme and warning or rejecting non-HTTPS URLs for AICoreConfig.AuthURL and APIURL.

**[MINOR]** Auth and API endpoints are accepted as-is without enforcing HTTPS. Using plain HTTP would expose client credentials and tokens in transit. Consider validating scheme and warning or rejecting non-HTTPS URLs for AICoreConfig.AuthURL and APIURL.
defer c.mu.Unlock()
Outdated
Review

[MINOR] AICore AuthURL and APIURL are accepted as-is and used to construct requests without enforcing HTTPS. If misconfigured to use plain HTTP, client credentials could be sent in cleartext. Consider validating the scheme and rejecting non-HTTPS URLs or warning explicitly.

**[MINOR]** AICore AuthURL and APIURL are accepted as-is and used to construct requests without enforcing HTTPS. If misconfigured to use plain HTTP, client credentials could be sent in cleartext. Consider validating the scheme and rejecting non-HTTPS URLs or warning explicitly.
// Double-check after acquiring write lock
if c.token != "" && time.Now().Add(5*time.Minute).Before(c.tokenExpiry) {
return c.token, nil
}
token, expiry, err := c.fetchToken(ctx)
Outdated
Review

[NIT] The token refresh threshold time.Now().Add(5*time.Minute).Before(c.tokenExpiry) is somewhat inverted in readability. c.tokenExpiry.After(time.Now().Add(5*time.Minute)) expresses the same condition more naturally: "expiry is more than 5 minutes in the future".

**[NIT]** The token refresh threshold `time.Now().Add(5*time.Minute).Before(c.tokenExpiry)` is somewhat inverted in readability. `c.tokenExpiry.After(time.Now().Add(5*time.Minute))` expresses the same condition more naturally: "expiry is more than 5 minutes in the future".
Outdated
Review

[MINOR] AuthURL is used verbatim to build the token endpoint without enforcing HTTPS. To prevent accidental plaintext credential transmission, validate the scheme and reject non-HTTPS URLs during client configuration.

**[MINOR]** AuthURL is used verbatim to build the token endpoint without enforcing HTTPS. To prevent accidental plaintext credential transmission, validate the scheme and reject non-HTTPS URLs during client configuration.
if err != nil {
return "", err
}
Outdated
Review

[MINOR] All AI Core responses are read with io.ReadAll without a size cap. For defense-in-depth against oversized or malfunctioning endpoints, consider limiting reads (e.g., io.LimitedReader) especially on error paths to mitigate memory exhaustion.

**[MINOR]** All AI Core responses are read with io.ReadAll without a size cap. For defense-in-depth against oversized or malfunctioning endpoints, consider limiting reads (e.g., io.LimitedReader) especially on error paths to mitigate memory exhaustion.
Review

[MINOR] Auth and API endpoints are accepted as-is without scheme validation. If misconfigured to use http://, client_id/client_secret and bearer tokens would be transmitted in cleartext. Enforce HTTPS (reject non-https schemes) for AICORE_AUTH_URL and AICORE_API_URL to prevent credential leakage.

**[MINOR]** Auth and API endpoints are accepted as-is without scheme validation. If misconfigured to use http://, client_id/client_secret and bearer tokens would be transmitted in cleartext. Enforce HTTPS (reject non-https schemes) for AICORE_AUTH_URL and AICORE_API_URL to prevent credential leakage.
c.token = token
Outdated
Review

[NIT] The token refresh buffer of 5 minutes (time.Now().Add(5*time.Minute).Before(c.tokenExpiry)) is hardcoded in two places in getToken. This could be extracted as a named constant (e.g. tokenRefreshBuffer = 5 * time.Minute) to make the intent clearer and avoid duplication.

**[NIT]** The token refresh buffer of 5 minutes (`time.Now().Add(5*time.Minute).Before(c.tokenExpiry)`) is hardcoded in two places in `getToken`. This could be extracted as a named constant (e.g. `tokenRefreshBuffer = 5 * time.Minute`) to make the intent clearer and avoid duplication.
c.tokenExpiry = expiry
return token, nil
}
func (c *AICoreClient) fetchToken(ctx context.Context) (string, time.Time, error) {
tokenURL := strings.TrimRight(c.config.AuthURL, "/") + "/oauth/token"
Outdated
Review

[MINOR] AuthURL is taken from configuration without scheme validation. If misconfigured to use plain http or a non-HTTPS URL, client credentials could be sent in cleartext and requests could be directed to untrusted endpoints. Consider enforcing or validating an https scheme (or at least logging/warning on non-HTTPS) before issuing the token request.

**[MINOR]** AuthURL is taken from configuration without scheme validation. If misconfigured to use plain http or a non-HTTPS URL, client credentials could be sent in cleartext and requests could be directed to untrusted endpoints. Consider enforcing or validating an https scheme (or at least logging/warning on non-HTTPS) before issuing the token request.
data := url.Values{}
data.Set("grant_type", "client_credentials")
data.Set("client_id", c.config.ClientID)
data.Set("client_secret", c.config.ClientSecret)
req, err := http.NewRequestWithContext(ctx, http.MethodPost, tokenURL, strings.NewReader(data.Encode()))
if err != nil {
return "", time.Time{}, fmt.Errorf("create token request: %w", err)
Outdated
Review

[MINOR] Errors include the full upstream response body (e.g., token/deployments/invoke failures). Upstream error bodies may echo user prompts or internal details; propagating these into logs can leak sensitive data. Consider truncating/redacting bodies in errors or logging only status codes and minimal context.

**[MINOR]** Errors include the full upstream response body (e.g., token/deployments/invoke failures). Upstream error bodies may echo user prompts or internal details; propagating these into logs can leak sensitive data. Consider truncating/redacting bodies in errors or logging only status codes and minimal context.
}
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
Review

[NIT] Error messages include upstream response bodies (truncated to 200 bytes) for token and API failures. While truncation reduces risk, upstream bodies can still contain sensitive diagnostic information. Consider omitting bodies by default or only including sanitized snippets for specific, known-safe error formats.

**[NIT]** Error messages include upstream response bodies (truncated to 200 bytes) for token and API failures. While truncation reduces risk, upstream bodies can still contain sensitive diagnostic information. Consider omitting bodies by default or only including sanitized snippets for specific, known-safe error formats.
resp, err := c.http.Do(req)
Outdated
Review

[MINOR] The code constructs token and API URLs directly from configuration without enforcing HTTPS. If misconfigured to use http://, client credentials and tokens could be transmitted in cleartext. Consider validating that AuthURL and APIURL use https and reject or warn on insecure schemes, with an explicit override only for trusted test environments.

**[MINOR]** The code constructs token and API URLs directly from configuration without enforcing HTTPS. If misconfigured to use http://, client credentials and tokens could be transmitted in cleartext. Consider validating that AuthURL and APIURL use https and reject or warn on insecure schemes, with an explicit override only for trusted test environments.
if err != nil {
Outdated
Review

[MINOR] Error messages include (truncated) upstream response bodies from the OAuth token and AI Core endpoints. While truncated to 200 bytes, consider further limiting or redacting token endpoint bodies to avoid leaking potentially sensitive details in CI logs (also applies to lines 196, 291, and 356 for other AI Core requests).

**[MINOR]** Error messages include (truncated) upstream response bodies from the OAuth token and AI Core endpoints. While truncated to 200 bytes, consider further limiting or redacting token endpoint bodies to avoid leaking potentially sensitive details in CI logs (also applies to lines 196, 291, and 356 for other AI Core requests).
return "", time.Time{}, fmt.Errorf("token request: %w", err)
Outdated
Review

[MINOR] On token request failure, the code includes the entire response body in the error string ("token request failed ...: %s"). This can lead to information leakage in logs if the auth server returns diagnostic details. Consider truncating or redacting bodies in error messages.

**[MINOR]** On token request failure, the code includes the entire response body in the error string ("token request failed ...: %s"). This can lead to information leakage in logs if the auth server returns diagnostic details. Consider truncating or redacting bodies in error messages.
}
Outdated
Review

[MINOR] In getDeploymentURL, there's a potential inefficiency: getToken is called before acquiring the write lock, which is correct for avoiding deadlock, but if fetchDeployments fails and the function is called again concurrently, multiple goroutines could redundantly call getToken (which itself acquires a lock). This is a minor performance nit rather than a correctness issue since getToken is idempotent and safe to call concurrently.

**[MINOR]** In `getDeploymentURL`, there's a potential inefficiency: `getToken` is called before acquiring the write lock, which is correct for avoiding deadlock, but if `fetchDeployments` fails and the function is called again concurrently, multiple goroutines could redundantly call `getToken` (which itself acquires a lock). This is a minor performance nit rather than a correctness issue since `getToken` is idempotent and safe to call concurrently.
defer resp.Body.Close()
Outdated
Review

[MINOR] Auth and API endpoints from configuration (AuthURL, APIURL) are used without scheme/host validation. Misconfiguration could allow plaintext HTTP or an unexpected host. Consider enforcing https scheme and optionally restricting to an allowed domain list to reduce MITM risk.

**[MINOR]** Auth and API endpoints from configuration (AuthURL, APIURL) are used without scheme/host validation. Misconfiguration could allow plaintext HTTP or an unexpected host. Consider enforcing https scheme and optionally restricting to an allowed domain list to reduce MITM risk.
body, err := io.ReadAll(resp.Body)
if err != nil {
return "", time.Time{}, fmt.Errorf("read token response: %w", err)
}
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
return "", time.Time{}, fmt.Errorf("token request failed (status %d): %s", resp.StatusCode, truncateBody(body))
}
var tokenResp struct {
AccessToken string `json:"access_token"`
ExpiresIn int `json:"expires_in"`
}
if err := json.Unmarshal(body, &tokenResp); err != nil {
return "", time.Time{}, fmt.Errorf("parse token response: %w", err)
}
Outdated
Review

[MINOR] The deployment cache is never invalidated. If an AI Core deployment becomes unavailable (transitions from RUNNING to STOPPED) after the initial fetch, the cached stale URL will be used until the process restarts. This is likely acceptable for the current use case (short-lived CI jobs), but could cause hard-to-debug failures in longer-lived deployments. A TTL on the deployment cache would make this more robust.

**[MINOR]** The deployment cache is never invalidated. If an AI Core deployment becomes unavailable (transitions from RUNNING to STOPPED) after the initial fetch, the cached stale URL will be used until the process restarts. This is likely acceptable for the current use case (short-lived CI jobs), but could cause hard-to-debug failures in longer-lived deployments. A TTL on the deployment cache would make this more robust.
if tokenResp.AccessToken == "" {
return "", time.Time{}, fmt.Errorf("empty access token in response")
}
expiry := time.Now().Add(time.Duration(tokenResp.ExpiresIn) * time.Second)
return tokenResp.AccessToken, expiry, nil
Outdated
Review

[MINOR] Duplicate doc comment on getDeploymentURL: the function comment appears twice (lines 143-144 and 145-151). The first one-liner is a leftover from before the fuller comment was added.

**[MINOR]** Duplicate doc comment on `getDeploymentURL`: the function comment appears twice (lines 143-144 and 145-151). The first one-liner is a leftover from before the fuller comment was added.
}
Outdated
Review

[MINOR] fetchDeployments is invoked while holding the write lock in getDeploymentURL, causing a network request under mutex. This can block other goroutines (token refresh or cached reads) longer than necessary. Consider fetching deployments outside the lock and updating the cache under the lock, or using a separate mutex for deployments vs token.

**[MINOR]** fetchDeployments is invoked while holding the write lock in getDeploymentURL, causing a network request under mutex. This can block other goroutines (token refresh or cached reads) longer than necessary. Consider fetching deployments outside the lock and updating the cache under the lock, or using a separate mutex for deployments vs token.
// getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.
// getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.
Outdated
Review

[MINOR] Duplicate doc comment. The getDeploymentURL function has its doc comment written twice: the first instance at line 148 is a plain 'getDeploymentURL returns the deployment URL...' and then it repeats again at line 149. One should be removed.

**[MINOR]** Duplicate doc comment. The `getDeploymentURL` function has its doc comment written twice: the first instance at line 148 is a plain 'getDeploymentURL returns the deployment URL...' and then it repeats again at line 149. One should be removed.
// Also returns a valid token for use by the caller, avoiding redundant getToken calls.
//
Outdated
Review

[NIT] Duplicate comment line above getDeploymentURL ("getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.") appears twice. Remove the duplicate for clarity.

**[NIT]** Duplicate comment line above getDeploymentURL ("getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.") appears twice. Remove the duplicate for clarity.
Outdated
Review

[MINOR] Error paths include truncated upstream response bodies (up to 200 chars) in error messages. While truncated, this may still leak sensitive upstream details into logs (e.g., error descriptions, request identifiers). Consider further redaction or logging only minimal metadata (status code, request ID) by default, with an opt-in debug flag for body snippets.

**[MINOR]** Error paths include truncated upstream response bodies (up to 200 chars) in error messages. While truncated, this may still leak sensitive upstream details into logs (e.g., error descriptions, request identifiers). Consider further redaction or logging only minimal metadata (status code, request ID) by default, with an opt-in debug flag for body snippets.
Review

[NIT] Duplicate comment line above getDeploymentURL ("getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.") — remove the repeated sentence.

**[NIT]** Duplicate comment line above getDeploymentURL ("getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.") — remove the repeated sentence.
// Note: The token is fetched before acquiring the write lock to avoid holding the lock
// during network I/O. In rare cases where multiple goroutines race and one waits a long
// time for the write lock, the token could theoretically expire. The 5-minute refresh
Outdated
Review

[MINOR] Duplicate doc comment on getDeploymentURL. The function has two consecutive doc comment blocks — one starting 'getDeploymentURL returns the deployment URL...' appears twice. The second block is the authoritative one (it includes the token-return note and the concurrency caveat), but the first redundant line should be removed.

**[MINOR]** Duplicate doc comment on `getDeploymentURL`. The function has two consecutive doc comment blocks — one starting 'getDeploymentURL returns the deployment URL...' appears twice. The second block is the authoritative one (it includes the token-return note and the concurrency caveat), but the first redundant line should be removed.
// buffer in getToken makes this extremely unlikely in practice.
func (c *AICoreClient) getDeploymentURL(ctx context.Context, model string) (deployURL, token string, err error) {
Outdated
Review

[NIT] Duplicate doc comment: // getDeploymentURL returns the deployment URL for a model, fetching deployments if needed. appears twice consecutively (lines ~154-155 and ~156-160 in the source). One copy should be removed.

**[NIT]** Duplicate doc comment: `// getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.` appears twice consecutively (lines ~154-155 and ~156-160 in the source). One copy should be removed.
Outdated
Review

[MINOR] Duplicate doc comment on getDeploymentURL. The function has two consecutive doc comment blocks starting with '// getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.' The first is a stale copy that should be removed. This is visible in both the diff and the full file.

**[MINOR]** Duplicate doc comment on `getDeploymentURL`. The function has two consecutive doc comment blocks starting with '// getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.' The first is a stale copy that should be removed. This is visible in both the diff and the full file.
c.mu.RLock()
if u, ok := c.deployments[model]; ok {
c.mu.RUnlock()
// Still need a token for the caller
Outdated
Review

[NIT] The variable url (used in if url, ok := c.deployments[model]; ok) shadows the built-in net/url package import, but net/url is imported and only used in fetchToken. While it works, renaming the local variable (e.g., deployURL) would avoid the shadow and make the code clearer.

**[NIT]** The variable `url` (used in `if url, ok := c.deployments[model]; ok`) shadows the built-in `net/url` package import, but `net/url` is imported and only used in `fetchToken`. While it works, renaming the local variable (e.g., `deployURL`) would avoid the shadow and make the code clearer.
token, err = c.getToken(ctx)
if err != nil {
return "", "", fmt.Errorf("get token: %w", err)
Outdated
Review

[MINOR] Deployment cache is never invalidated. Once deployments are fetched, they are cached indefinitely. If a deployment goes down and is replaced (new deployment URL, same model name), the client will continue using the stale URL until restart. Consider adding a TTL or a re-fetch-on-error path (e.g., clear the cache and retry when CompleteAnthropic/CompleteOpenAI gets a non-2xx response).

**[MINOR]** Deployment cache is never invalidated. Once deployments are fetched, they are cached indefinitely. If a deployment goes down and is replaced (new deployment URL, same model name), the client will continue using the stale URL until restart. Consider adding a TTL or a re-fetch-on-error path (e.g., clear the cache and retry when CompleteAnthropic/CompleteOpenAI gets a non-2xx response).
Review

[MINOR] Duplicate doc comment block: getDeploymentURL has its doc comment written twice in succession (lines 157-169 of aicore.go). The first copy should be removed.

**[MINOR]** Duplicate doc comment block: `getDeploymentURL` has its doc comment written twice in succession (lines 157-169 of aicore.go). The first copy should be removed.
}
Outdated
Review

[MINOR] Duplicate doc comment: the getDeploymentURL function has its doc comment written twice (lines 163-172 and 173-179 in the file). The first occurrence should be removed.

**[MINOR]** Duplicate doc comment: the `getDeploymentURL` function has its doc comment written twice (lines 163-172 and 173-179 in the file). The first occurrence should be removed.
return u, token, nil
}
c.mu.RUnlock()
// Fetch token first (before acquiring write lock to avoid holding lock during I/O)
Outdated
Review

[MINOR] The deployments cache is populated once and never invalidated. If a deployment becomes unavailable (status changes from RUNNING to STOPPED) after initial discovery, the client will continue trying to use the old URL until the process restarts. Consider adding a TTL or a re-fetch on 4xx/5xx responses from the invoke endpoint. This is acceptable for a short-lived CI runner process but could be an issue for longer-running deployments.

**[MINOR]** The deployments cache is populated once and never invalidated. If a deployment becomes unavailable (status changes from RUNNING to STOPPED) after initial discovery, the client will continue trying to use the old URL until the process restarts. Consider adding a TTL or a re-fetch on 4xx/5xx responses from the invoke endpoint. This is acceptable for a short-lived CI runner process but could be an issue for longer-running deployments.
token, err = c.getToken(ctx)
if err != nil {
Outdated
Review

[MINOR] getDeploymentURL holds the write mutex while calling fetchDeployments, which performs network I/O. This can block other goroutines and creates a potential denial-of-service vector if the external request is slow (even with a timeout). Release the lock around network calls and only acquire it when updating the shared map.

**[MINOR]** getDeploymentURL holds the write mutex while calling fetchDeployments, which performs network I/O. This can block other goroutines and creates a potential denial-of-service vector if the external request is slow (even with a timeout). Release the lock around network calls and only acquire it when updating the shared map.
Outdated
Review

[NIT] Duplicate comment line above getDeploymentURL: the line 'getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.' appears twice and can be deduplicated.

**[NIT]** Duplicate comment line above getDeploymentURL: the line 'getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.' appears twice and can be deduplicated.
Outdated
Review

[NIT] Duplicate comment line above getDeploymentURL ("getDeploymentURL returns the deployment URL for a model, fetching deployments if needed."). Remove the repeated line to improve readability.

**[NIT]** Duplicate comment line above getDeploymentURL ("getDeploymentURL returns the deployment URL for a model, fetching deployments if needed."). Remove the repeated line to improve readability.
return "", "", fmt.Errorf("get token for deployments: %w", err)
}
c.mu.Lock()
defer c.mu.Unlock()
Outdated
Review

[MINOR] The fetchDeployments call inside getDeploymentURL is invoked while holding c.mu (write lock), and fetchDeployments calls c.http.Do which is a network I/O operation. The design comment acknowledges fetching the token outside the lock, but the deployment fetch itself still holds the lock during network I/O. This is fine for the intended short-lived CI use case (as noted in the struct doc), but it means concurrent callers waiting for other models will block during this I/O. The code comment could explicitly mention this constraint.

**[MINOR]** The `fetchDeployments` call inside `getDeploymentURL` is invoked while holding `c.mu` (write lock), and `fetchDeployments` calls `c.http.Do` which is a network I/O operation. The design comment acknowledges fetching the token outside the lock, but the deployment fetch itself still holds the lock during network I/O. This is fine for the intended short-lived CI use case (as noted in the struct doc), but it means concurrent callers waiting for other models will block during this I/O. The code comment could explicitly mention this constraint.
Outdated
Review

[MINOR] APIURL is used verbatim to build the deployments endpoint without enforcing HTTPS. Add a scheme check to ensure only HTTPS endpoints are used.

**[MINOR]** APIURL is used verbatim to build the deployments endpoint without enforcing HTTPS. Add a scheme check to ensure only HTTPS endpoints are used.
// Double-check after acquiring write lock
if u, ok := c.deployments[model]; ok {
Outdated
Review

[MINOR] When the deployment cache lookup succeeds on the RLock path, getToken is called again (acquiring locks again), even though a fresh token was already obtained if this was just populated. This is a minor inefficiency — not a correctness issue — but the fast-path could return the already-fetched token from the cache population path. The current structure is correct but slightly suboptimal.

**[MINOR]** When the deployment cache lookup succeeds on the RLock path, `getToken` is called again (acquiring locks again), even though a fresh token was already obtained if this was just populated. This is a minor inefficiency — not a correctness issue — but the fast-path could return the already-fetched token from the cache population path. The current structure is correct but slightly suboptimal.
return u, token, nil
}
Outdated
Review

[MINOR] fetchDeployments is called while holding the write lock (c.mu.Lock() in getDeploymentURL). This means HTTP I/O (the deployments API call) happens under the mutex, which blocks all concurrent readers for the duration of the network round-trip. For a single-use CLI tool this is fine, but for a library it would be better to do the fetch outside the lock and then re-acquire to write. This is an accepted tradeoff for the current use case but worth noting.

**[MINOR]** fetchDeployments is called while holding the write lock (c.mu.Lock() in getDeploymentURL). This means HTTP I/O (the deployments API call) happens under the mutex, which blocks all concurrent readers for the duration of the network round-trip. For a single-use CLI tool this is fine, but for a library it would be better to do the fetch outside the lock and then re-acquire to write. This is an accepted tradeoff for the current use case but worth noting.
Outdated
Review

[MINOR] APIURL is used to construct the deployments endpoint without scheme validation. To reduce the risk of accidental plaintext transmission or SSRF-like misuse in misconfigured environments, consider enforcing or validating an https scheme for APIURL before making requests.

**[MINOR]** APIURL is used to construct the deployments endpoint without scheme validation. To reduce the risk of accidental plaintext transmission or SSRF-like misuse in misconfigured environments, consider enforcing or validating an https scheme for APIURL before making requests.
if err := c.fetchDeployments(ctx, token); err != nil {
return "", "", err
}
Outdated
Review

[MINOR] The client_secret is sent as a URL-encoded form parameter (data.Set("client_secret", c.config.ClientSecret)). While this is valid per OAuth2 spec (Resource Owner Password flow uses form body), many OAuth2 servers also accept/prefer HTTP Basic auth (Authorization: Basic base64(clientID:secret)). The current approach is spec-compliant but worth noting as some IdPs may reject it. Not a bug, but the comment could document why form body is used over HTTP Basic auth for this particular IdP.

**[MINOR]** The client_secret is sent as a URL-encoded form parameter (`data.Set("client_secret", c.config.ClientSecret)`). While this is valid per OAuth2 spec (Resource Owner Password flow uses form body), many OAuth2 servers also accept/prefer HTTP Basic auth (`Authorization: Basic base64(clientID:secret)`). The current approach is spec-compliant but worth noting as some IdPs may reject it. Not a bug, but the comment could document why form body is used over HTTP Basic auth for this particular IdP.
if u, ok := c.deployments[model]; ok {
return u, token, nil
}
Outdated
Review

[NIT] In getDeploymentURL, when the deployment is found in cache on the read-lock path, getToken is called after releasing the read lock. This is correct but means the token could have been refreshed unnecessarily. This is a minor inefficiency acceptable for CI runners but worth noting in longer-lived use.

**[NIT]** In `getDeploymentURL`, when the deployment is found in cache on the read-lock path, `getToken` is called after releasing the read lock. This is correct but means the token could have been refreshed unnecessarily. This is a minor inefficiency acceptable for CI runners but worth noting in longer-lived use.
return "", "", fmt.Errorf("no deployment found for model %q", model)
}
Outdated
Review

[MINOR] When multiple models exist with the same name and both are RUNNING, the last one encountered wins (the loop overwrites c.deployments[modelName]). There is no tie-breaking and the outcome is non-deterministic depending on API response order. The test for this scenario (deploy-456 is STOPPED, deploy-789 is RUNNING for gpt-5) works correctly, but if two RUNNING deployments for the same model name exist the behaviour is undefined. A comment explaining the intent would help.

**[MINOR]** When multiple models exist with the same name and both are RUNNING, the last one encountered wins (the loop overwrites `c.deployments[modelName]`). There is no tie-breaking and the outcome is non-deterministic depending on API response order. The test for this scenario (`deploy-456` is STOPPED, `deploy-789` is RUNNING for gpt-5) works correctly, but if two RUNNING deployments for the same model name exist the behaviour is undefined. A comment explaining the intent would help.
Outdated
Review

[MINOR] When multiple goroutines concurrently call getDeploymentURL for different models that are both missing from the cache, they will all serialize on the write lock and call fetchDeployments once (due to the double-check). However, if the first fetchDeployments populates only some models and a concurrent caller for a different model races past the first read-lock check but before the write-lock check, behavior is correct. This is fine. But there's a subtle issue: the token fetched before acquiring the write lock in getDeploymentURL could expire by the time fetchDeployments uses it (especially with a short-lived token). Since the code holds getToken(ctx) result and passes it to fetchDeployments, but fetchDeployments is called potentially after a long wait for the write lock, the token might be expired. Given the 5-minute refresh buffer this is extremely unlikely in practice, but worth documenting.

**[MINOR]** When multiple goroutines concurrently call `getDeploymentURL` for different models that are both missing from the cache, they will all serialize on the write lock and call `fetchDeployments` once (due to the double-check). However, if the first `fetchDeployments` populates only some models and a concurrent caller for a different model races past the first read-lock check but before the write-lock check, behavior is correct. This is fine. But there's a subtle issue: the token fetched before acquiring the write lock in `getDeploymentURL` could expire by the time `fetchDeployments` uses it (especially with a short-lived token). Since the code holds `getToken(ctx)` result and passes it to `fetchDeployments`, but `fetchDeployments` is called potentially after a long wait for the write lock, the token might be expired. Given the 5-minute refresh buffer this is extremely unlikely in practice, but worth documenting.
Outdated
Review

[NIT] Duplicate comment line above getDeploymentURL ("getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.") appears twice; remove the repeated line for clarity.

**[NIT]** Duplicate comment line above getDeploymentURL ("getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.") appears twice; remove the repeated line for clarity.
func (c *AICoreClient) fetchDeployments(ctx context.Context, token string) error {
deployURL := strings.TrimRight(c.config.APIURL, "/") + "/v2/lm/deployments"
req, err := http.NewRequestWithContext(ctx, http.MethodGet, deployURL, nil)
Outdated
Review

[MINOR] The getDeploymentURL function shadows the builtin url identifier from the net/url package imported at the top of the file. Since net/url is only used in fetchToken and not in getDeploymentURL, this shadowing doesn't cause a bug, but it is slightly surprising. Consider renaming the local variable to deployURL to be consistent with the naming used elsewhere and avoid the shadow.

**[MINOR]** The `getDeploymentURL` function shadows the builtin `url` identifier from the `net/url` package imported at the top of the file. Since `net/url` is only used in `fetchToken` and not in `getDeploymentURL`, this shadowing doesn't cause a bug, but it is slightly surprising. Consider renaming the local variable to `deployURL` to be consistent with the naming used elsewhere and avoid the shadow.
if err != nil {
return fmt.Errorf("create deployments request: %w", err)
}
req.Header.Set("Authorization", "Bearer "+token)
Outdated
Review

[MINOR] When multiple models map to the same name (e.g. two RUNNING gpt-5 deployments in TestAICoreClient_DeploymentFetch), fetchDeployments silently overwrites with the last one seen. The test verifies it picks deploy-789 over deploy-456 (stopped), but if two RUNNING deployments existed for the same model the behavior is undefined (last-write-wins from iteration order). A log warning or first-wins strategy would be more predictable, though this is unlikely in practice.

**[MINOR]** When multiple models map to the same name (e.g. two RUNNING gpt-5 deployments in `TestAICoreClient_DeploymentFetch`), `fetchDeployments` silently overwrites with the last one seen. The test verifies it picks `deploy-789` over `deploy-456` (stopped), but if two RUNNING deployments existed for the same model the behavior is undefined (last-write-wins from iteration order). A log warning or first-wins strategy would be more predictable, though this is unlikely in practice.
Review

[MINOR] The code caches and later calls the absolute deploymentUrl returned by the AI Core API and sends the OAuth bearer token to that URL. If the API (or DNS) is compromised or misconfigured, this could direct requests (with tokens) to an unexpected host. Consider validating that deploymentUrl uses https and matches the expected AI Core host (or is under the configured API domain) before use.

**[MINOR]** The code caches and later calls the absolute deploymentUrl returned by the AI Core API and sends the OAuth bearer token to that URL. If the API (or DNS) is compromised or misconfigured, this could direct requests (with tokens) to an unexpected host. Consider validating that deploymentUrl uses https and matches the expected AI Core host (or is under the configured API domain) before use.
req.Header.Set("AI-Resource-Group", c.config.ResourceGroup)
Outdated
Review

[MINOR] When tokenResp.ExpiresIn is 0 (either because the field is missing or the server returns 0), expiry will equal time.Now(), causing every subsequent call to immediately re-fetch the token. A reasonable fallback (e.g., if tokenResp.ExpiresIn <= 0 { tokenResp.ExpiresIn = 3600 }) would be more robust.

**[MINOR]** When `tokenResp.ExpiresIn` is 0 (either because the field is missing or the server returns 0), `expiry` will equal `time.Now()`, causing every subsequent call to immediately re-fetch the token. A reasonable fallback (e.g., `if tokenResp.ExpiresIn <= 0 { tokenResp.ExpiresIn = 3600 }`) would be more robust.
Outdated
Review

[MINOR] The deployment URL returned by the AI Core API is used verbatim for follow-up requests with the bearer token. As a defense-in-depth measure, consider verifying that the deployment URL host matches or is within the expected API domain to reduce the risk of token exfiltration if the deployments listing is compromised.

**[MINOR]** The deployment URL returned by the AI Core API is used verbatim for follow-up requests with the bearer token. As a defense-in-depth measure, consider verifying that the deployment URL host matches or is within the expected API domain to reduce the risk of token exfiltration if the deployments listing is compromised.
resp, err := c.http.Do(req)
if err != nil {
return fmt.Errorf("deployments request: %w", err)
}
Outdated
Review

[MINOR] On deployments API failure, the error message embeds the full response body ("deployments request failed...: %s"). This may leak internal details to logs. Consider truncating the body to a small prefix or omitting it.

**[MINOR]** On deployments API failure, the error message embeds the full response body ("deployments request failed...: %s"). This may leak internal details to logs. Consider truncating the body to a small prefix or omitting it.
defer resp.Body.Close()
body, err := io.ReadAll(resp.Body)
if err != nil {
return fmt.Errorf("read deployments response: %w", err)
}
Outdated
Review

[MINOR] getDeploymentURL holds the write lock while calling fetchDeployments, which performs network I/O. Holding a mutex during blocking I/O can stall other callers. Consider fetching deployments outside the lock (singleflight or double-checked locking) and only locking to update the cache.

**[MINOR]** getDeploymentURL holds the write lock while calling fetchDeployments, which performs network I/O. Holding a mutex during blocking I/O can stall other callers. Consider fetching deployments outside the lock (singleflight or double-checked locking) and only locking to update the cache.
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
return fmt.Errorf("deployments request failed (status %d): %s", resp.StatusCode, truncateBody(body))
}
var deployResp struct {
Resources []struct {
DeploymentURL string `json:"deploymentUrl"`
Status string `json:"status"`
Details struct {
Resources struct {
Outdated
Review

[MINOR] In CompleteAnthropic, the token is fetched after getDeploymentURL which itself calls getToken internally. This results in two calls to getToken — one inside getDeploymentURL and one explicit call afterwards. The second call will typically be a cheap cache hit, but it's redundant and slightly misleading. Consider using the token returned from getDeploymentURL by refactoring, or at minimum add a comment explaining why the second fetch is safe/cheap.

**[MINOR]** In `CompleteAnthropic`, the token is fetched after `getDeploymentURL` which itself calls `getToken` internally. This results in two calls to `getToken` — one inside `getDeploymentURL` and one explicit call afterwards. The second call will typically be a cheap cache hit, but it's redundant and slightly misleading. Consider using the token returned from `getDeploymentURL` by refactoring, or at minimum add a comment explaining why the second fetch is safe/cheap.
BackendDetails struct {
Model struct {
Name string `json:"name"`
Outdated
Review

[NIT] The anthropicRequest struct's AnthropicVersion field is set to the hard-coded string "bedrock-2023-05-31" at the call site rather than using a named constant. A package-level constant like AICoreAnthropicVersion would make this easier to update and self-documenting, similar to the already-defined AICoreOpenAIAPIVersion.

**[NIT]** The `anthropicRequest` struct's `AnthropicVersion` field is set to the hard-coded string `"bedrock-2023-05-31"` at the call site rather than using a named constant. A package-level constant like `AICoreAnthropicVersion` would make this easier to update and self-documenting, similar to the already-defined `AICoreOpenAIAPIVersion`.
} `json:"model"`
} `json:"backend_details"`
} `json:"resources"`
} `json:"details"`
Outdated
Review

[MINOR] CompleteAnthropic calls getDeploymentURL and then getToken as separate operations. Between these two calls, the token could theoretically expire (extremely unlikely in practice, but the token fetched for deployment discovery in getDeploymentURL is not reused here). This is a minor efficiency issue rather than a correctness bug since getToken handles caching and the window is tiny, but it would be slightly cleaner to fetch the token once and pass it through. Low priority given this only matters on token boundary.

**[MINOR]** CompleteAnthropic calls getDeploymentURL and then getToken as separate operations. Between these two calls, the token could theoretically expire (extremely unlikely in practice, but the token fetched for deployment discovery in getDeploymentURL is not reused here). This is a minor efficiency issue rather than a correctness bug since getToken handles caching and the window is tiny, but it would be slightly cleaner to fetch the token once and pass it through. Low priority given this only matters on token boundary.
} `json:"resources"`
}
Outdated
Review

[MINOR] The maxTokens parameter in CompleteAnthropic is part of the public signature but the only caller (completeAICore in client.go) always passes the hardcoded value 8192. This leaks an implementation detail into the public API. Consider making maxTokens an internal constant or adding it to AICoreConfig, since callers currently have no way to control it through the Client interface.

**[MINOR]** The `maxTokens` parameter in `CompleteAnthropic` is part of the public signature but the only caller (`completeAICore` in client.go) always passes the hardcoded value 8192. This leaks an implementation detail into the public API. Consider making `maxTokens` an internal constant or adding it to `AICoreConfig`, since callers currently have no way to control it through the `Client` interface.
if err := json.Unmarshal(body, &deployResp); err != nil {
return fmt.Errorf("parse deployments response: %w", err)
}
Outdated
Review

[MINOR] fetchDeployments is called while holding the write lock (c.mu.Lock()), and it performs network I/O. Although the design comment acknowledges the pre-fetch token strategy, fetchDeployments itself still executes an HTTP request under the lock. This means concurrent callers waiting for the lock are blocked during the full deployment discovery round-trip. For CI usage this is acceptable (comment in code says so), but worth noting for any future longer-lived use.

**[MINOR]** `fetchDeployments` is called while holding the write lock (`c.mu.Lock()`), and it performs network I/O. Although the design comment acknowledges the pre-fetch token strategy, `fetchDeployments` itself still executes an HTTP request under the lock. This means concurrent callers waiting for the lock are blocked during the full deployment discovery round-trip. For CI usage this is acceptable (comment in code says so), but worth noting for any future longer-lived use.
for _, r := range deployResp.Resources {
if r.Status != "RUNNING" {
continue
}
modelName := r.Details.Resources.BackendDetails.Model.Name
if modelName == "" {
continue
}
c.deployments[modelName] = r.DeploymentURL
}
return nil
}
Outdated
Review

[MINOR] If two concurrent goroutines both miss the read-lock cache check for getDeploymentURL, they will both call getToken before acquiring the write lock. Since getToken itself acquires c.mu internally, and then getDeploymentURL acquires c.mu.Lock(), there's a potential for the two goroutines to acquire c.mu.Lock() sequentially, both calling fetchDeployments. The double-check after acquiring the write lock prevents duplicate work, but the second goroutine still called getToken unnecessarily. This is a minor efficiency concern, not a correctness issue, since getToken is also protected by the same mutex and has its own double-check.

**[MINOR]** If two concurrent goroutines both miss the read-lock cache check for `getDeploymentURL`, they will both call `getToken` before acquiring the write lock. Since `getToken` itself acquires `c.mu` internally, and then `getDeploymentURL` acquires `c.mu.Lock()`, there's a potential for the two goroutines to acquire `c.mu.Lock()` sequentially, both calling `fetchDeployments`. The double-check after acquiring the write lock prevents duplicate work, but the second goroutine still called `getToken` unnecessarily. This is a minor efficiency concern, not a correctness issue, since `getToken` is also protected by the same mutex and has its own double-check.
// CompleteAnthropic sends a request to an Anthropic model via AI Core.
func (c *AICoreClient) CompleteAnthropic(ctx context.Context, model string, messages []Message, maxTokens int, temperature float64) (string, error) {
Outdated
Review

[MINOR] The code trusts the deploymentUrl returned by the AI Core API and uses it directly for subsequent requests (e.g., appending /invoke or /chat/completions). Without validating that the URL uses HTTPS and matches the expected AI Core domain, this introduces a potential SSRF vector if the upstream were compromised or misconfigured. Consider validating the scheme is https and that the host matches (or is a subdomain of) the configured API URL host before using it.

**[MINOR]** The code trusts the deploymentUrl returned by the AI Core API and uses it directly for subsequent requests (e.g., appending /invoke or /chat/completions). Without validating that the URL uses HTTPS and matches the expected AI Core domain, this introduces a potential SSRF vector if the upstream were compromised or misconfigured. Consider validating the scheme is https and that the host matches (or is a subdomain of) the configured API URL host before using it.
deployURL, token, err := c.getDeploymentURL(ctx, model)
if err != nil {
return "", err
}
// Extract system message
var system string
var userMessages []anthropicMsg
for _, m := range messages {
if m.Role == "system" {
Outdated
Review

[NIT] CompleteOpenAI includes the "model" field in the request body even though SAP AI Core deployments already determine the model. While likely harmless, omitting it could reduce confusion or potential mismatch if the payload model name diverges from the deployment.

**[NIT]** CompleteOpenAI includes the "model" field in the request body even though SAP AI Core deployments already determine the model. While likely harmless, omitting it could reduce confusion or potential mismatch if the payload model name diverges from the deployment.
Outdated
Review

[MINOR] CompleteOpenAI includes the model field in the request body even though the deployment already specifies the model; some AI Core/OpenAI-compatible endpoints may ignore or expect this differently. Consider omitting the model or documenting that it must match the deployment name to avoid mismatch issues.

**[MINOR]** CompleteOpenAI includes the model field in the request body even though the deployment already specifies the model; some AI Core/OpenAI-compatible endpoints may ignore or expect this differently. Consider omitting the model or documenting that it must match the deployment name to avoid mismatch issues.
system = m.Content
} else {
userMessages = append(userMessages, anthropicMsg{
Role: m.Role,
Content: m.Content,
Outdated
Review

[MINOR] When there are multiple RUNNING deployments for the same model name, the last one in the list silently wins (map assignment overwrites). If SAP AI Core could return multiple running deployments for the same model, this could select an unexpected one. The behavior is undocumented; at minimum a log warning would be helpful.

**[MINOR]** When there are multiple RUNNING deployments for the same model name, the last one in the list silently wins (map assignment overwrites). If SAP AI Core could return multiple running deployments for the same model, this could select an unexpected one. The behavior is undocumented; at minimum a log warning would be helpful.
Review

[MINOR] In fetchDeployments, when multiple RUNNING deployments exist for the same model name (as tested in TestAICoreClient_DeploymentFetch), the last one in iteration order wins — the map is overwritten silently. The test asserts deploy-789 (the second RUNNING gpt-5) is selected, but this is only guaranteed because JSON array order is preserved in Go's json.Unmarshal. This is fragile; a comment noting the last-write-wins behavior (or explicitly preferring the last RUNNING entry) would clarify intent.

**[MINOR]** In `fetchDeployments`, when multiple RUNNING deployments exist for the same model name (as tested in `TestAICoreClient_DeploymentFetch`), the last one in iteration order wins — the map is overwritten silently. The test asserts `deploy-789` (the second RUNNING gpt-5) is selected, but this is only guaranteed because JSON array order is preserved in Go's `json.Unmarshal`. This is fragile; a comment noting the last-write-wins behavior (or explicitly preferring the last RUNNING entry) would clarify intent.
})
}
Outdated
Review

[MINOR] The maxTokens parameter on CompleteAnthropic is always called with the hardcoded value 8192 (both from completeAICore in client.go and from the test). Consider either removing the parameter and hardcoding it internally, or making it a constant to avoid the magic number at call sites.

**[MINOR]** The `maxTokens` parameter on `CompleteAnthropic` is always called with the hardcoded value 8192 (both from `completeAICore` in client.go and from the test). Consider either removing the parameter and hardcoding it internally, or making it a constant to avoid the magic number at call sites.
}
reqBody := anthropicRequest{
AnthropicVersion: "bedrock-2023-05-31", // SAP AI Core uses Bedrock format
// Model omitted - AI Core deployment already specifies model
MaxTokens: maxTokens,
Outdated
Review

[MINOR] Consider handling 401 Unauthorized responses by refreshing the OAuth token and retrying once. This would improve resilience if the token expires earlier than anticipated despite the 5-minute refresh buffer.

**[MINOR]** Consider handling 401 Unauthorized responses by refreshing the OAuth token and retrying once. This would improve resilience if the token expires earlier than anticipated despite the 5-minute refresh buffer.
System: system,
Messages: userMessages,
}
if temperature > 0 {
Outdated
Review

[MINOR] CompleteAnthropic has a hardcoded maxTokens parameter in its signature but callers always pass 8192. This leaks an implementation detail through the API; consider making it a constant or removing the parameter since it's always 8192.

**[MINOR]** `CompleteAnthropic` has a hardcoded `maxTokens` parameter in its signature but callers always pass `8192`. This leaks an implementation detail through the API; consider making it a constant or removing the parameter since it's always 8192.
reqBody.Temperature = temperature
}
Outdated
Review

[MINOR] Same double getToken pattern in CompleteOpenAIgetDeploymentURL already fetches a token internally, then another explicit getToken call follows. Same concern as above.

**[MINOR]** Same double `getToken` pattern in `CompleteOpenAI` — `getDeploymentURL` already fetches a token internally, then another explicit `getToken` call follows. Same concern as above.
data, err := json.Marshal(reqBody)
if err != nil {
return "", fmt.Errorf("marshal request: %w", err)
Outdated
Review

[NIT] The hardcoded string "bedrock-2023-05-31" in CompleteAnthropic could be a named constant like AICoreAnthropicVersion similar to AICoreOpenAIAPIVersion, making it easier to update and self-documenting.

**[NIT]** The hardcoded string `"bedrock-2023-05-31"` in `CompleteAnthropic` could be a named constant like `AICoreAnthropicVersion` similar to `AICoreOpenAIAPIVersion`, making it easier to update and self-documenting.
}
// AI Core uses /invoke for Anthropic models
Outdated
Review

[MINOR] Error paths include the full response body in returned errors (e.g., "AI Core API error (status %d): %s"). If these errors are logged in CI or production, they may leak internal details. Consider truncating or redacting bodies in errors to reduce information exposure.

**[MINOR]** Error paths include the full response body in returned errors (e.g., "AI Core API error (status %d): %s"). If these errors are logged in CI or production, they may leak internal details. Consider truncating or redacting bodies in errors to reduce information exposure.
invokeURL := strings.TrimRight(deployURL, "/") + "/invoke"
req, err := http.NewRequestWithContext(ctx, http.MethodPost, invokeURL, bytes.NewReader(data))
if err != nil {
return "", fmt.Errorf("create request: %w", err)
}
req.Header.Set("Authorization", "Bearer "+token)
req.Header.Set("AI-Resource-Group", c.config.ResourceGroup)
req.Header.Set("Content-Type", "application/json")
resp, err := c.http.Do(req)
if err != nil {
return "", fmt.Errorf("AI Core request: %w", err)
}
defer resp.Body.Close()
body, err := io.ReadAll(resp.Body)
Outdated
Review

[MINOR] In CompleteOpenAI, the request body includes the 'model' field, but AI Core deployments already specify the model. This is usually harmless, but consider confirming it cannot conflict with deployment settings and documenting the intent (unlike the Anthropic path where model is omitted).

**[MINOR]** In CompleteOpenAI, the request body includes the 'model' field, but AI Core deployments already specify the model. This is usually harmless, but consider confirming it cannot conflict with deployment settings and documenting the intent (unlike the Anthropic path where model is omitted).
if err != nil {
Outdated
Review

[NIT] The AnthropicVersion field is hardcoded as a string literal "bedrock-2023-05-31" inside CompleteAnthropic. Consider extracting this as a named constant (e.g. AICoreAnthropicVersion) similar to AICoreOpenAIAPIVersion, for consistency and to make it easy to update.

**[NIT]** The `AnthropicVersion` field is hardcoded as a string literal `"bedrock-2023-05-31"` inside `CompleteAnthropic`. Consider extracting this as a named constant (e.g. `AICoreAnthropicVersion`) similar to `AICoreOpenAIAPIVersion`, for consistency and to make it easy to update.
return "", fmt.Errorf("read response: %w", err)
}
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
return "", fmt.Errorf("AI Core API error (status %d): %s", resp.StatusCode, truncateBody(body))
}
var anthropicResp anthropicResponse
if err := json.Unmarshal(body, &anthropicResp); err != nil {
return "", fmt.Errorf("parse response: %w", err)
}
if len(anthropicResp.Content) == 0 {
return "", fmt.Errorf("no content in response")
}
var sb strings.Builder
for _, block := range anthropicResp.Content {
if block.Type == "text" {
sb.WriteString(block.Text)
Outdated
Review

[MINOR] On Anthropic invoke API failure, the code returns the full response body in the error string. For defense-in-depth, avoid logging entire bodies from upstream services; truncate or sanitize to prevent accidental leakage of sensitive information.

**[MINOR]** On Anthropic invoke API failure, the code returns the full response body in the error string. For defense-in-depth, avoid logging entire bodies from upstream services; truncate or sanitize to prevent accidental leakage of sensitive information.
}
}
result := sb.String()
if result == "" {
return "", fmt.Errorf("no text content in response")
}
return result, nil
}
// CompleteOpenAI sends a request to an OpenAI model via AI Core.
func (c *AICoreClient) CompleteOpenAI(ctx context.Context, model string, messages []Message, temperature float64) (string, error) {
deployURL, token, err := c.getDeploymentURL(ctx, model)
if err != nil {
return "", err
}
reqBody := ChatRequest{
Outdated
Review

[NIT] Exported function IsAnthropicModel lacks a doc comment. Add a brief comment (e.g., "IsAnthropicModel reports whether the model name corresponds to an Anthropic deployment in AI Core.") to follow Go doc conventions for exported identifiers.

**[NIT]** Exported function IsAnthropicModel lacks a doc comment. Add a brief comment (e.g., "IsAnthropicModel reports whether the model name corresponds to an Anthropic deployment in AI Core.") to follow Go doc conventions for exported identifiers.
Model: model,
Temperature: temperature,
Messages: messages,
}
Outdated
Review

[MINOR] In CompleteOpenAI, the request body includes the "model" field even though AI Core deployments already bind the model. Consider omitting Model to rely solely on the deployment configuration, reducing potential mismatch risk.

**[MINOR]** In CompleteOpenAI, the request body includes the "model" field even though AI Core deployments already bind the model. Consider omitting Model to rely solely on the deployment configuration, reducing potential mismatch risk.
data, err := json.Marshal(reqBody)
if err != nil {
return "", fmt.Errorf("marshal request: %w", err)
}
// AI Core uses /chat/completions?api-version=<version> for OpenAI models
chatURL := strings.TrimRight(deployURL, "/") + "/chat/completions?api-version=" + AICoreOpenAIAPIVersion
req, err := http.NewRequestWithContext(ctx, http.MethodPost, chatURL, bytes.NewReader(data))
if err != nil {
return "", fmt.Errorf("create request: %w", err)
}
req.Header.Set("Authorization", "Bearer "+token)
req.Header.Set("AI-Resource-Group", c.config.ResourceGroup)
req.Header.Set("Content-Type", "application/json")
resp, err := c.http.Do(req)
if err != nil {
return "", fmt.Errorf("AI Core request: %w", err)
}
defer resp.Body.Close()
body, err := io.ReadAll(resp.Body)
if err != nil {
return "", fmt.Errorf("read response: %w", err)
}
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
return "", fmt.Errorf("AI Core API error (status %d): %s", resp.StatusCode, truncateBody(body))
}
var openaiResp ChatResponse
if err := json.Unmarshal(body, &openaiResp); err != nil {
return "", fmt.Errorf("parse response: %w", err)
}
if len(openaiResp.Choices) == 0 {
return "", fmt.Errorf("no choices in response")
}
return openaiResp.Choices[0].Message.Content, nil
}
// IsAnthropicModel returns true if the model name indicates an Anthropic model.
// SAP AI Core uses "anthropic--" prefix for Anthropic models (e.g., "anthropic--claude-3-5-sonnet").
func IsAnthropicModel(model string) bool {
return strings.HasPrefix(model, "anthropic--")
}
+535
View File
@@ -0,0 +1,535 @@
package llm
Outdated
Review

[NIT] The time import is used only in TestAICoreClient_TokenExpiry and TestAICoreClient_WithTimeout. The fmt import is used only in TestAICoreClient_TokenExpiry. These are fine — just noting the test file is in package llm (white-box), which is appropriate for testing unexported methods like getToken and getDeploymentURL.

**[NIT]** The `time` import is used only in `TestAICoreClient_TokenExpiry` and `TestAICoreClient_WithTimeout`. The `fmt` import is used only in `TestAICoreClient_TokenExpiry`. These are fine — just noting the test file is in `package llm` (white-box), which is appropriate for testing unexported methods like `getToken` and `getDeploymentURL`.
import (
"context"
"encoding/json"
"fmt"
Outdated
Review

[NIT] The time package is imported but used only in TestAICoreClient_TokenExpiry and TestAICoreClient_WithTimeout. Not a problem, just noting that removing "time" from the import if those tests were moved would be needed. Current state is correct.

**[NIT]** The `time` package is imported but used only in `TestAICoreClient_TokenExpiry` and `TestAICoreClient_WithTimeout`. Not a problem, just noting that removing `"time"` from the import if those tests were moved would be needed. Current state is correct.
"net/http"
"net/http/httptest"
"strings"
"sync/atomic"
"testing"
"time"
)
func TestAICoreClient_TokenFetch(t *testing.T) {
tokenCalls := int32(0)
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path == "/oauth/token" {
atomic.AddInt32(&tokenCalls, 1)
if r.Method != http.MethodPost {
t.Errorf("expected POST for token, got %s", r.Method)
}
if r.Header.Get("Content-Type") != "application/x-www-form-urlencoded" {
t.Errorf("expected form content type")
}
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]interface{}{
"access_token": "test-token-123",
"expires_in": 3600,
Outdated
Review

[NIT] The test TestAICoreClient_TokenFetch uses json.NewEncoder(w).Encode(...) without checking the error. While errors on httptest.ResponseRecorder writes are extremely rare, for consistency with the Go convention of always checking errors, consider assigning and ignoring explicitly or using must helpers.

**[NIT]** The test `TestAICoreClient_TokenFetch` uses `json.NewEncoder(w).Encode(...)` without checking the error. While errors on `httptest.ResponseRecorder` writes are extremely rare, for consistency with the Go convention of always checking errors, consider assigning and ignoring explicitly or using `must` helpers.
Outdated
Review

[NIT] Test uses json.NewEncoder(w).Encode(...) without checking the error return. While test code is less critical, this pattern appears throughout the test file. A helper like mustEncodeJSON(t, w, v) would be cleaner and consistent.

**[NIT]** Test uses `json.NewEncoder(w).Encode(...)` without checking the error return. While test code is less critical, this pattern appears throughout the test file. A helper like `mustEncodeJSON(t, w, v)` would be cleaner and consistent.
})
return
}
t.Errorf("unexpected path: %s", r.URL.Path)
}))
defer server.Close()
Outdated
Review

[NIT] The test uses json.NewEncoder(w).Encode(...) without checking its error return. While errors from writing to an httptest.ResponseRecorder are extremely rare, this is technically ignoring an error. The established project convention is to check all error returns.

**[NIT]** The test uses `json.NewEncoder(w).Encode(...)` without checking its error return. While errors from writing to an `httptest.ResponseRecorder` are extremely rare, this is technically ignoring an error. The established project convention is to check all error returns.
client := NewAICoreClient(AICoreConfig{
ClientID: "test-id",
ClientSecret: "test-secret",
AuthURL: server.URL,
APIURL: server.URL,
ResourceGroup: "default",
})
token, err := client.getToken(context.Background())
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if token != "test-token-123" {
t.Errorf("expected token 'test-token-123', got %q", token)
}
// Second call should use cached token
token2, err := client.getToken(context.Background())
if err != nil {
t.Fatalf("unexpected error: %v", err)
Outdated
Review

[NIT] json.NewEncoder(w).Encode(...) errors are silently ignored throughout the test handlers. While this is common in test code and a failure here would manifest as a test failure, adding t.Helper() and checking the error would make test failures easier to diagnose.

**[NIT]** json.NewEncoder(w).Encode(...) errors are silently ignored throughout the test handlers. While this is common in test code and a failure here would manifest as a test failure, adding t.Helper() and checking the error would make test failures easier to diagnose.
}
if token2 != "test-token-123" {
t.Errorf("expected cached token")
}
if atomic.LoadInt32(&tokenCalls) != 1 {
t.Errorf("expected 1 token call (cached), got %d", tokenCalls)
}
}
func TestAICoreClient_DeploymentFetch(t *testing.T) {
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path == "/oauth/token" {
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]interface{}{
"access_token": "test-token",
"expires_in": 3600,
})
return
}
if r.URL.Path == "/v2/lm/deployments" {
if r.Header.Get("Authorization") != "Bearer test-token" {
t.Errorf("expected Bearer auth")
}
if r.Header.Get("AI-Resource-Group") != "default" {
t.Errorf("expected resource group header")
}
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]interface{}{
"resources": []map[string]interface{}{
{
"id": "deploy-123",
"deploymentUrl": "https://example.com/v2/inference/deployments/deploy-123",
"status": "RUNNING",
"details": map[string]interface{}{
"resources": map[string]interface{}{
"backend_details": map[string]interface{}{
"model": map[string]interface{}{
"name": "anthropic--claude-4.6-sonnet",
},
},
},
},
},
{
"id": "deploy-456",
"deploymentUrl": "https://example.com/v2/inference/deployments/deploy-456",
"status": "STOPPED",
"details": map[string]interface{}{
"resources": map[string]interface{}{
"backend_details": map[string]interface{}{
"model": map[string]interface{}{
"name": "gpt-5",
},
},
},
},
},
{
"id": "deploy-789",
"deploymentUrl": "https://example.com/v2/inference/deployments/deploy-789",
"status": "RUNNING",
"details": map[string]interface{}{
"resources": map[string]interface{}{
"backend_details": map[string]interface{}{
"model": map[string]interface{}{
"name": "gpt-5",
},
},
},
},
},
},
})
return
}
t.Errorf("unexpected path: %s", r.URL.Path)
}))
defer server.Close()
client := NewAICoreClient(AICoreConfig{
ClientID: "test-id",
ClientSecret: "test-secret",
AuthURL: server.URL,
APIURL: server.URL,
ResourceGroup: "default",
})
// Should find running deployment
url, _, err := client.getDeploymentURL(context.Background(), "anthropic--claude-4.6-sonnet")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if url != "https://example.com/v2/inference/deployments/deploy-123" {
t.Errorf("unexpected URL: %s", url)
}
// Should find running gpt-5, not stopped one
url, _, err = client.getDeploymentURL(context.Background(), "gpt-5")
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if url != "https://example.com/v2/inference/deployments/deploy-789" {
t.Errorf("unexpected URL: %s", url)
}
// Should error on unknown model
_, _, err = client.getDeploymentURL(context.Background(), "unknown-model")
if err == nil {
Outdated
Review

[NIT] The comment // Use a pointer to capture the server URL for use in the handler is slightly misleading — baseURL is a string, not a pointer. The actual mechanism is that the closure captures the variable by reference. The comment could be dropped or rephrased.

**[NIT]** The comment `// Use a pointer to capture the server URL for use in the handler` is slightly misleading — `baseURL` is a `string`, not a pointer. The actual mechanism is that the closure captures the variable by reference. The comment could be dropped or rephrased.
t.Error("expected error for unknown model")
}
}
Outdated
Review

[NIT] The comment // Use a pointer to capture the server URL for use in the handler in TestAICoreClient_CompleteAnthropic is slightly misleading — baseURL is a string, not a pointer. The variable is captured by closure. The comment could simply say // baseURL is set after server creation; captured by closure in handlers.

**[NIT]** The comment `// Use a pointer to capture the server URL for use in the handler` in `TestAICoreClient_CompleteAnthropic` is slightly misleading — `baseURL` is a `string`, not a pointer. The variable is captured by closure. The comment could simply say `// baseURL is set after server creation; captured by closure in handlers.`
func TestAICoreClient_CompleteAnthropic(t *testing.T) {
// baseURL is set after server creation; captured by closure in handlers
var baseURL string
mux := http.NewServeMux()
mux.HandleFunc("/oauth/token", func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]interface{}{
"access_token": "test-token",
"expires_in": 3600,
})
})
mux.HandleFunc("/v2/lm/deployments", func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]interface{}{
"resources": []map[string]interface{}{
{
"id": "deploy-anthropic",
"deploymentUrl": baseURL + "/deployments/anthropic",
"status": "RUNNING",
"details": map[string]interface{}{
"resources": map[string]interface{}{
"backend_details": map[string]interface{}{
"model": map[string]interface{}{
"name": "anthropic--claude-4.6-sonnet",
},
},
},
},
},
},
})
})
mux.HandleFunc("/deployments/anthropic/invoke", func(w http.ResponseWriter, r *http.Request) {
if r.Header.Get("Authorization") != "Bearer test-token" {
t.Errorf("expected Bearer auth on invoke")
}
var req anthropicRequest
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
t.Fatalf("decode request: %v", err)
}
if req.AnthropicVersion != "bedrock-2023-05-31" {
t.Errorf("expected bedrock anthropic_version in request")
}
if req.System != "You are helpful" {
t.Errorf("expected system prompt: %q", req.System)
}
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]interface{}{
"content": []map[string]interface{}{
{"type": "text", "text": "Hello from AI Core!"},
},
})
})
server := httptest.NewServer(mux)
baseURL = server.URL
defer server.Close()
client := NewAICoreClient(AICoreConfig{
ClientID: "test-id",
ClientSecret: "test-secret",
AuthURL: server.URL,
APIURL: server.URL,
ResourceGroup: "default",
})
result, err := client.CompleteAnthropic(context.Background(), "anthropic--claude-4.6-sonnet", []Message{
{Role: "system", Content: "You are helpful"},
{Role: "user", Content: "Hello"},
}, 8192, 0)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if result != "Hello from AI Core!" {
t.Errorf("expected 'Hello from AI Core!', got %q", result)
}
}
func TestAICoreClient_CompleteOpenAI(t *testing.T) {
var baseURL string
mux := http.NewServeMux()
mux.HandleFunc("/oauth/token", func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]interface{}{
"access_token": "test-token",
"expires_in": 3600,
})
})
mux.HandleFunc("/v2/lm/deployments", func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]interface{}{
"resources": []map[string]interface{}{
{
Outdated
Review

[NIT] In TestAICoreClient_DeploymentFetch, when two RUNNING deployments exist for gpt-5, the test asserts the second one (deploy-789) is returned. The current implementation happens to return deploy-789 because it overwrites deploy-456 during iteration, but this is iteration-order dependent on the JSON response. The test comment 'Should find running gpt-5, not stopped one' is accurate, but the assertion on the exact URL (deploy-789 vs deploy-123 for the other case) implicitly depends on the map-overwrite behavior. This is fine given the deterministic JSON array order from the test server, but worth a comment.

**[NIT]** In `TestAICoreClient_DeploymentFetch`, when two RUNNING deployments exist for `gpt-5`, the test asserts the second one (`deploy-789`) is returned. The current implementation happens to return `deploy-789` because it overwrites `deploy-456` during iteration, but this is iteration-order dependent on the JSON response. The test comment 'Should find running gpt-5, not stopped one' is accurate, but the assertion on the exact URL (`deploy-789` vs `deploy-123` for the other case) implicitly depends on the map-overwrite behavior. This is fine given the deterministic JSON array order from the test server, but worth a comment.
"id": "deploy-openai",
"deploymentUrl": baseURL + "/deployments/openai",
"status": "RUNNING",
"details": map[string]interface{}{
"resources": map[string]interface{}{
"backend_details": map[string]interface{}{
"model": map[string]interface{}{
"name": "gpt-5",
},
},
},
},
},
},
})
})
mux.HandleFunc("/deployments/openai/chat/completions", func(w http.ResponseWriter, r *http.Request) {
if r.URL.Query().Get("api-version") != AICoreOpenAIAPIVersion {
t.Errorf("expected api-version %s, got %s", AICoreOpenAIAPIVersion, r.URL.Query().Get("api-version"))
}
var req ChatRequest
if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
t.Fatalf("decode request: %v", err)
}
if req.Model != "gpt-5" {
t.Errorf("expected model gpt-5, got %s", req.Model)
}
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(ChatResponse{
Choices: []struct {
Message struct {
Content string `json:"content"`
} `json:"message"`
}{
{Message: struct {
Content string `json:"content"`
}{Content: "Hello from GPT-5!"}},
},
})
})
server := httptest.NewServer(mux)
baseURL = server.URL
defer server.Close()
client := NewAICoreClient(AICoreConfig{
ClientID: "test-id",
ClientSecret: "test-secret",
AuthURL: server.URL,
APIURL: server.URL,
ResourceGroup: "default",
})
result, err := client.CompleteOpenAI(context.Background(), "gpt-5", []Message{
{Role: "user", Content: "Hello"},
}, 0)
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if result != "Hello from GPT-5!" {
t.Errorf("expected 'Hello from GPT-5!', got %q", result)
}
}
func TestIsAnthropicModel(t *testing.T) {
tests := []struct {
model string
expected bool
}{
// SAP AI Core uses "anthropic--" prefix for Anthropic models
{"anthropic--claude-4.6-sonnet", true},
{"anthropic--claude-4.6-opus", true},
{"anthropic--claude-3-5-sonnet", true},
// Non-prefixed model names are not detected as Anthropic
// (SAP AI Core always uses the prefix for Anthropic models)
{"claude-sonnet-4", false},
{"gpt-5", false},
{"gpt-4.1", false},
{"llama-3", false},
{"my-claude-model", false}, // Avoid false positives on "claude" substring
}
for _, tt := range tests {
got := IsAnthropicModel(tt.model)
if got != tt.expected {
t.Errorf("IsAnthropicModel(%q) = %v, want %v", tt.model, got, tt.expected)
}
}
}
func TestAICoreClient_TokenExpiry(t *testing.T) {
tokenCalls := int32(0)
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
if r.URL.Path == "/oauth/token" {
call := atomic.AddInt32(&tokenCalls, 1)
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]interface{}{
"access_token": fmt.Sprintf("token-%d", call),
"expires_in": 1, // 1 second expiry
})
return
}
}))
defer server.Close()
client := NewAICoreClient(AICoreConfig{
ClientID: "test-id",
ClientSecret: "test-secret",
AuthURL: server.URL,
APIURL: server.URL,
ResourceGroup: "default",
})
// First call
token1, err := client.getToken(context.Background())
if err != nil {
t.Fatalf("first getToken: %v", err)
}
// Force token expiry by manipulating expiry time
client.mu.Lock()
client.tokenExpiry = time.Now().Add(-time.Hour)
client.mu.Unlock()
// Should fetch new token
token2, err := client.getToken(context.Background())
if err != nil {
t.Fatalf("second getToken: %v", err)
}
Outdated
Review

[MINOR] The TestIsAnthropicModel test uses the old-style loop rather than t.Run subtests, making it harder to see which specific case failed. Per the testing patterns established in the codebase, table-driven tests should use t.Run with named subtests.

**[MINOR]** The `TestIsAnthropicModel` test uses the old-style loop rather than `t.Run` subtests, making it harder to see which specific case failed. Per the testing patterns established in the codebase, table-driven tests should use `t.Run` with named subtests.
if token1 == token2 {
t.Error("expected different tokens after expiry")
}
if atomic.LoadInt32(&tokenCalls) != 2 {
Outdated
Review

[NIT] TestAICoreClient_TokenExpiry sets expires_in: 1 in the server response but never actually waits for the token to expire naturally — it directly manipulates client.tokenExpiry. The server-side expires_in: 1 is misleading; it could just be any value since the test bypasses natural expiry. Consider using a comment or a more clearly intentional value (e.g., 3600) to avoid confusion.

**[NIT]** `TestAICoreClient_TokenExpiry` sets `expires_in: 1` in the server response but never actually waits for the token to expire naturally — it directly manipulates `client.tokenExpiry`. The server-side `expires_in: 1` is misleading; it could just be any value since the test bypasses natural expiry. Consider using a comment or a more clearly intentional value (e.g., 3600) to avoid confusion.
t.Errorf("expected 2 token calls, got %d", tokenCalls)
}
}
func TestAICoreClient_WithTimeout(t *testing.T) {
client := NewAICoreClient(AICoreConfig{
ClientID: "test-id",
ClientSecret: "test-secret",
AuthURL: "https://auth.example.com",
APIURL: "https://api.example.com",
ResourceGroup: "default",
})
// Default timeout is 5 minutes
if client.http.Timeout != 5*time.Minute {
Outdated
Review

[NIT] Test name "TestClient_WithTimeout_PropagatestoAICore" has a small typo/formatting issue; consider renaming to "TestClient_WithTimeout_PropagatesToAICore" for readability and consistency.

**[NIT]** Test name "TestClient_WithTimeout_PropagatestoAICore" has a small typo/formatting issue; consider renaming to "TestClient_WithTimeout_PropagatesToAICore" for readability and consistency.
t.Errorf("expected default timeout 5m, got %v", client.http.Timeout)
}
// WithTimeout should update the timeout
client.WithTimeout(10 * time.Minute)
if client.http.Timeout != 10*time.Minute {
t.Errorf("expected timeout 10m, got %v", client.http.Timeout)
}
}
func TestClient_WithAICore(t *testing.T) {
client := NewClient("http://example.com", "key", "model")
if client.provider != ProviderOpenAI {
t.Errorf("expected default provider openai, got %s", client.provider)
}
client.WithAICore(AICoreConfig{
ClientID: "id",
ClientSecret: "secret",
AuthURL: "https://auth.example.com",
APIURL: "https://api.example.com",
ResourceGroup: "default",
})
if client.provider != ProviderAICore {
t.Errorf("expected provider aicore, got %s", client.provider)
}
if client.aicore == nil {
t.Error("expected aicore client to be set")
}
}
func TestClient_WithTimeout_PropagatestoAICore(t *testing.T) {
client := NewClient("http://example.com", "key", "model").
WithAICore(AICoreConfig{
Outdated
Review

[NIT] Test name "TestClient_WithTimeout_PropagatestoAICore" has a small typo in the word separation ("Propagatesto"). Consider renaming to "TestClient_WithTimeout_PropagatesToAICore" for readability.

**[NIT]** Test name "TestClient_WithTimeout_PropagatestoAICore" has a small typo in the word separation ("Propagatesto"). Consider renaming to "TestClient_WithTimeout_PropagatesToAICore" for readability.
ClientID: "id",
ClientSecret: "secret",
AuthURL: "https://auth.example.com",
APIURL: "https://api.example.com",
ResourceGroup: "default",
})
// Default should be 5 minutes (inherited from parent client)
if client.aicore.http.Timeout != 5*time.Minute {
t.Errorf("expected aicore default timeout 5m, got %v", client.aicore.http.Timeout)
}
// WithTimeout should propagate to AI Core client
client.WithTimeout(15 * time.Minute)
if client.http.Timeout != 15*time.Minute {
t.Errorf("expected parent timeout 15m, got %v", client.http.Timeout)
}
if client.aicore.http.Timeout != 15*time.Minute {
t.Errorf("expected aicore timeout 15m, got %v", client.aicore.http.Timeout)
}
}
func TestClient_CompleteAICore(t *testing.T) {
var baseURL string
mux := http.NewServeMux()
mux.HandleFunc("/oauth/token", func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]interface{}{
"access_token": "test-token",
"expires_in": 3600,
})
})
mux.HandleFunc("/v2/lm/deployments", func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(map[string]interface{}{
"resources": []map[string]interface{}{
{
"id": "deploy-test",
"deploymentUrl": baseURL + "/deployments/test",
"status": "RUNNING",
"details": map[string]interface{}{
"resources": map[string]interface{}{
"backend_details": map[string]interface{}{
"model": map[string]interface{}{
"name": "gpt-5",
},
},
},
},
},
},
})
})
mux.HandleFunc("/deployments/test/chat/completions", func(w http.ResponseWriter, r *http.Request) {
w.Header().Set("Content-Type", "application/json")
json.NewEncoder(w).Encode(ChatResponse{
Choices: []struct {
Message struct {
Content string `json:"content"`
} `json:"message"`
}{
{Message: struct {
Content string `json:"content"`
}{Content: "AI Core via Client works!"}},
},
})
})
server := httptest.NewServer(mux)
baseURL = server.URL
defer server.Close()
client := NewClient("", "", "gpt-5").WithAICore(AICoreConfig{
ClientID: "test-id",
ClientSecret: "test-secret",
AuthURL: server.URL,
APIURL: server.URL,
ResourceGroup: "default",
})
result, err := client.Complete(context.Background(), []Message{
{Role: "user", Content: "Hello"},
})
if err != nil {
t.Fatalf("unexpected error: %v", err)
}
if !strings.Contains(result, "AI Core via Client works!") {
t.Errorf("unexpected result: %s", result)
}
}
+38 -7
View File
@@ -1,6 +1,6 @@
// Package llm provides clients for LLM chat completion APIs.
//
// Supports OpenAI-compatible (default) and Anthropic Messages API providers.
// Supports OpenAI-compatible (default), Anthropic Messages API, and SAP AI Core providers.
package llm
import (
@@ -22,6 +22,8 @@ const (
ProviderOpenAI Provider = "openai"
// ProviderAnthropic uses the Anthropic Messages API endpoint.
ProviderAnthropic Provider = "anthropic"
// ProviderAICore uses SAP AI Core with OAuth authentication.
ProviderAICore Provider = "aicore"
)
// Client calls an LLM chat completion API.
@@ -35,6 +37,7 @@ type Client struct {
temperature float64
provider Provider
http *http.Client
aicore *AICoreClient // Only set when provider is aicore
}
// NewClient creates a new LLM client. Default provider is OpenAI-compatible.
@@ -49,8 +52,12 @@ func NewClient(baseURL, apiKey, model string) *Client {
}
// WithTimeout sets the HTTP request timeout for LLM calls (default 5 minutes).
// When using AI Core, this also sets the timeout on the AI Core client.
func (c *Client) WithTimeout(d time.Duration) *Client {
c.http.Timeout = d
if c.aicore != nil {
c.aicore.WithTimeout(d)
}
return c
}
@@ -60,12 +67,21 @@ func (c *Client) WithTemperature(t float64) *Client {
return c
}
// WithProvider sets the API provider format (openai or anthropic).
// WithProvider sets the API provider format (openai, anthropic, or aicore).
func (c *Client) WithProvider(p Provider) *Client {
c.provider = p
return c
}
// WithAICore configures the client to use SAP AI Core for authentication.
// This sets the provider to aicore automatically.
// The AI Core client inherits the current HTTP timeout from this client.
func (c *Client) WithAICore(cfg AICoreConfig) *Client {
c.provider = ProviderAICore
c.aicore = NewAICoreClient(cfg).WithTimeout(c.http.Timeout)
return c
}
// Message represents a chat message.
type Message struct {
Role string `json:"role"`
@@ -82,6 +98,8 @@ func (c *Client) Complete(ctx context.Context, messages []Message) (string, erro
switch c.provider {
case ProviderAnthropic:
result, err = c.completeAnthropic(ctx, messages)
case ProviderAICore:
result, err = c.completeAICore(ctx, messages)
default:
result, err = c.completeOpenAI(ctx, messages)
}
@@ -106,6 +124,18 @@ func (c *Client) Complete(ctx context.Context, messages []Message) (string, erro
return "", err
}
// completeAICore routes to AI Core using the appropriate endpoint based on model type.
func (c *Client) completeAICore(ctx context.Context, messages []Message) (string, error) {
if c.aicore == nil {
return "", fmt.Errorf("AI Core client not configured")
}
if IsAnthropicModel(c.model) {
return c.aicore.CompleteAnthropic(ctx, c.model, messages, 8192, c.temperature)
}
return c.aicore.CompleteOpenAI(ctx, c.model, messages, c.temperature)
}
// isRetryableError returns true for transient errors worth retrying.
func isRetryableError(err error) bool {
if err == nil {
@@ -176,11 +206,12 @@ func (c *Client) completeOpenAI(ctx context.Context, messages []Message) (string
// --- Anthropic Messages API implementation ---
Review

[NIT] The anthropicRequest struct reformatting (alignment change) is a cosmetic-only change unrelated to the feature. Not a problem, just worth noting it's pure noise in the diff.

**[NIT]** The `anthropicRequest` struct reformatting (alignment change) is a cosmetic-only change unrelated to the feature. Not a problem, just worth noting it's pure noise in the diff.
type anthropicRequest struct {
Model string `json:"model"`
MaxTokens int `json:"max_tokens"`
System string `json:"system,omitempty"`
Messages []anthropicMsg `json:"messages"`
Temperature float64 `json:"temperature,omitempty"`
AnthropicVersion string `json:"anthropic_version,omitempty"`
Outdated
Review

[NIT] The anthropicRequest struct fields have inconsistent alignment after the addition of AnthropicVersion - the field names are left-aligned but the types are misaligned compared to the original struct. gofmt handles this automatically, so this is only relevant if gofmt wasn't run, but worth noting.

**[NIT]** The `anthropicRequest` struct fields have inconsistent alignment after the addition of `AnthropicVersion` - the field names are left-aligned but the types are misaligned compared to the original struct. `gofmt` handles this automatically, so this is only relevant if `gofmt` wasn't run, but worth noting.
Model string `json:"model,omitempty"`
MaxTokens int `json:"max_tokens"`
System string `json:"system,omitempty"`
Messages []anthropicMsg `json:"messages"`
Outdated
Review

[NIT] The anthropicRequest struct reformatting (removing alignment of struct fields) is a minor style inconsistency — the original had aligned columns and the new version doesn't. gofmt doesn't enforce column alignment so either is valid, but the change is cosmetically noisy in the diff for a struct that's otherwise unchanged.

**[NIT]** The `anthropicRequest` struct reformatting (removing alignment of struct fields) is a minor style inconsistency — the original had aligned columns and the new version doesn't. `gofmt` doesn't enforce column alignment so either is valid, but the change is cosmetically noisy in the diff for a struct that's otherwise unchanged.
Outdated
Review

[NIT] The struct literal alignment for anthropicRequest fields is inconsistent in the diff — AnthropicVersion and Model are right-aligned differently from the remaining fields. A gofmt run would normalize this (though it may already be correct after formatting; hard to tell from the diff context).

**[NIT]** The struct literal alignment for `anthropicRequest` fields is inconsistent in the diff — `AnthropicVersion` and `Model` are right-aligned differently from the remaining fields. A `gofmt` run would normalize this (though it may already be correct after formatting; hard to tell from the diff context).
Temperature float64 `json:"temperature,omitempty"`
Outdated
Review

[NIT] The anthropicRequest struct alignment was changed: AnthropicVersion was added and Model changed from json:"model" to json:"model,omitempty". This is correct for the AI Core path (model is omitted because the deployment already specifies it), but the direct Anthropic path in completeAnthropic relies on Model being present. Since completeAnthropic always sets reqBody.Model = c.model, the omitempty will never trigger for a non-empty model — so it's functionally correct, but slightly confusing that the same struct now has different semantics for the two code paths.

**[NIT]** The `anthropicRequest` struct alignment was changed: `AnthropicVersion` was added and `Model` changed from `json:"model"` to `json:"model,omitempty"`. This is correct for the AI Core path (model is omitted because the deployment already specifies it), but the direct Anthropic path in `completeAnthropic` relies on `Model` being present. Since `completeAnthropic` always sets `reqBody.Model = c.model`, the `omitempty` will never trigger for a non-empty model — so it's functionally correct, but slightly confusing that the same struct now has different semantics for the two code paths.
}
type anthropicMsg struct {