feat: native SAP AI Core support #54
@@ -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'
|
||||
|
|
||||
conventions-file:
|
||||
description: 'Path to conventions file in the repo (e.g. CLAUDE.md)'
|
||||
required: false
|
||||
|
sonnet-review-bot
commented
[NIT] Trailing space on the **[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
|
||||
|
||||
@@ -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'
|
||||
@@ -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/"
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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)")
|
||||
|
gpt-review-bot
commented
[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 {
|
||||
@@ -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
|
||||
|
gpt-review-bot
commented
[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)
|
||||
}
|
||||
|
||||
@@ -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 {
|
||||
|
||||
@@ -0,0 +1,391 @@
|
||||
package llm
|
||||
|
[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.
sonnet-review-bot
commented
[NIT] The **[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
|
||||
|
[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
|
||||
|
sonnet-review-bot
commented
[NIT] The field name **[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.
|
||||
|
sonnet-review-bot
commented
[NIT] The field name **[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.
|
||||
|
sonnet-review-bot
commented
[NIT] The struct field **[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.
|
||||
//
|
||||
|
sonnet-review-bot
commented
[NIT] The struct field **[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
|
||||
|
sonnet-review-bot
commented
[NIT] The **[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
|
||||
}
|
||||
|
sonnet-review-bot
commented
[NIT] The **[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{
|
||||
|
sonnet-review-bot
commented
[NIT] The field name **[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),
|
||||
}
|
||||
}
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[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.
sonnet-review-bot
commented
[NIT] The field name **[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`).
sonnet-review-bot
commented
[NIT] The field name **[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 {
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[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()
|
||||
|
[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()
|
||||
|
||||
|
[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)
|
||||
|
sonnet-review-bot
commented
[NIT] The token refresh threshold **[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".
[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
|
||||
}
|
||||
|
[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.
[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
|
||||
|
sonnet-review-bot
commented
[NIT] The token refresh buffer of 5 minutes ( **[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"
|
||||
|
[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)
|
||||
|
[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")
|
||||
|
||||
|
[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)
|
||||
|
[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 {
|
||||
|
[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)
|
||||
|
[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.
|
||||
}
|
||||
|
sonnet-review-bot
commented
[MINOR] In **[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()
|
||||
|
[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)
|
||||
}
|
||||
|
sonnet-review-bot
commented
[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
|
||||
|
sonnet-review-bot
commented
[MINOR] Duplicate doc comment on **[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.
|
||||
}
|
||||
|
gpt-review-bot
commented
[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.
|
||||
|
sonnet-review-bot
commented
[MINOR] Duplicate doc comment. The **[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.
|
||||
//
|
||||
|
gpt-review-bot
commented
[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.
[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.
gpt-review-bot
commented
[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
|
||||
|
sonnet-review-bot
commented
[MINOR] Duplicate doc comment on **[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) {
|
||||
|
sonnet-review-bot
commented
[NIT] Duplicate doc comment: **[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.
sonnet-review-bot
commented
[MINOR] Duplicate doc comment on **[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
|
||||
|
sonnet-review-bot
commented
[NIT] The variable **[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)
|
||||
|
sonnet-review-bot
commented
[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).
sonnet-review-bot
commented
[MINOR] Duplicate doc comment block: **[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.
|
||||
}
|
||||
|
sonnet-review-bot
commented
[MINOR] Duplicate doc comment: the **[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)
|
||||
|
sonnet-review-bot
commented
[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 {
|
||||
|
[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.
gpt-review-bot
commented
[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.
gpt-review-bot
commented
[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()
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[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] 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 {
|
||||
|
sonnet-review-bot
commented
[MINOR] When the deployment cache lookup succeeds on the RLock path, **[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
|
||||
}
|
||||
|
sonnet-review-bot
commented
[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.
|
||||
|
||||
|
[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
|
||||
}
|
||||
|
sonnet-review-bot
commented
[MINOR] The client_secret is sent as a URL-encoded form parameter ( **[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
|
||||
}
|
||||
|
sonnet-review-bot
commented
[NIT] In **[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)
|
||||
}
|
||||
|
sonnet-review-bot
commented
[MINOR] When multiple models exist with the same name and both are RUNNING, the last one encountered wins (the loop overwrites **[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.
sonnet-review-bot
commented
[MINOR] When multiple goroutines concurrently call **[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.
gpt-review-bot
commented
[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)
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[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)
|
||||
|
sonnet-review-bot
commented
[MINOR] When multiple models map to the same name (e.g. two RUNNING gpt-5 deployments in **[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] 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)
|
||||
|
sonnet-review-bot
commented
[MINOR] When **[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] 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)
|
||||
}
|
||||
|
[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)
|
||||
}
|
||||
|
gpt-review-bot
commented
[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 {
|
||||
|
sonnet-review-bot
commented
[MINOR] In **[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"`
|
||||
|
sonnet-review-bot
commented
[NIT] The **[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"`
|
||||
|
sonnet-review-bot
commented
[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"`
|
||||
}
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[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)
|
||||
}
|
||||
|
sonnet-review-bot
commented
[MINOR] **[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
|
||||
}
|
||||
|
sonnet-review-bot
commented
[MINOR] If two concurrent goroutines both miss the read-lock cache check for **[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) {
|
||||
|
[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" {
|
||||
|
gpt-review-bot
commented
[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.
gpt-review-bot
commented
[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,
|
||||
|
sonnet-review-bot
commented
[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.
sonnet-review-bot
commented
[MINOR] In **[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.
|
||||
})
|
||||
}
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[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,
|
||||
|
gpt-review-bot
commented
[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 {
|
||||
|
sonnet-review-bot
commented
[MINOR] **[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
|
||||
}
|
||||
|
||||
|
sonnet-review-bot
commented
[MINOR] Same double **[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)
|
||||
|
sonnet-review-bot
commented
[NIT] The hardcoded string **[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
|
||||
|
[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)
|
||||
|
gpt-review-bot
commented
[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 {
|
||||
|
sonnet-review-bot
commented
[NIT] The **[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)
|
||||
|
[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{
|
||||
|
gpt-review-bot
commented
[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,
|
||||
}
|
||||
|
||||
|
gpt-review-bot
commented
[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--")
|
||||
}
|
||||
@@ -0,0 +1,535 @@
|
||||
package llm
|
||||
|
sonnet-review-bot
commented
[NIT] The **[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"
|
||||
|
sonnet-review-bot
commented
[NIT] The **[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,
|
||||
|
sonnet-review-bot
commented
[NIT] The test **[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.
sonnet-review-bot
commented
[NIT] Test uses **[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()
|
||||
|
sonnet-review-bot
commented
[NIT] The test uses **[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)
|
||||
|
sonnet-review-bot
commented
[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 {
|
||||
|
sonnet-review-bot
commented
[NIT] The comment **[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")
|
||||
}
|
||||
}
|
||||
|
||||
|
sonnet-review-bot
commented
[NIT] The comment **[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{}{
|
||||
{
|
||||
|
sonnet-review-bot
commented
[NIT] In **[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)
|
||||
}
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[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 {
|
||||
|
sonnet-review-bot
commented
[NIT] **[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 {
|
||||
|
gpt-review-bot
commented
[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{
|
||||
|
gpt-review-bot
commented
[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)
|
||||
}
|
||||
}
|
||||
@@ -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 ---
|
||||
|
sonnet-review-bot
commented
[NIT] The **[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"`
|
||||
|
sonnet-review-bot
commented
[NIT] The **[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"`
|
||||
|
sonnet-review-bot
commented
[NIT] The **[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.
sonnet-review-bot
commented
[NIT] The struct literal alignment for **[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"`
|
||||
|
sonnet-review-bot
commented
[NIT] The **[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 {
|
||||
|
||||
[NIT] Trailing whitespace on the
default: 'default'line foraicore-resource-group. Minor but inconsistent with the rest of the file.