feat: add native SAP AI Core support #50
@@ -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'
|
||||
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
|
||||
@@ -155,6 +177,11 @@ runs:
|
||||
LLM_PROVIDER: ${{ inputs.llm-provider }}
|
||||
UPDATE_EXISTING: ${{ inputs.update-existing }}
|
||||
SYSTEM_PROMPT_FILE: ${{ inputs.system-prompt-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,7 +18,8 @@ jobs:
|
||||
- run: go vet ./...
|
||||
- run: go build -o review-bot ./cmd/review-bot
|
||||
|
||||
# Self-review: builds from source since we're pre-release
|
||||
# Self-review using native SAP AI Core provider
|
||||
# Models must match SAP AI Core deployments (use 'anthropic--' prefix for Claude)
|
||||
review:
|
||||
runs-on: ubuntu-24.04
|
||||
if: github.event_name == 'pull_request'
|
||||
@@ -28,33 +29,12 @@ jobs:
|
||||
include:
|
||||
- name: sonnet
|
||||
token_secret: SONNET_REVIEW_TOKEN
|
||||
provider: anthropic
|
||||
llm_path: /anthropic/v1
|
||||
model: claude-sonnet-4-6
|
||||
model: anthropic--claude-4.6-sonnet
|
||||
- name: gpt
|
||||
token_secret: GPT_REVIEW_TOKEN
|
||||
provider: openai
|
||||
llm_path: /openai/v1
|
||||
model: gpt-5
|
||||
- name: gpt41
|
||||
token_secret: GPT_REVIEW_TOKEN
|
||||
provider: openai
|
||||
llm_path: /openai/v1
|
||||
model: gpt-4.1
|
||||
- name: gpt5-mini
|
||||
token_secret: GPT_REVIEW_TOKEN
|
||||
provider: openai
|
||||
llm_path: /openai/v1
|
||||
model: gpt-5-mini
|
||||
- name: gpt41-mini
|
||||
token_secret: GPT_REVIEW_TOKEN
|
||||
provider: openai
|
||||
llm_path: /openai/v1
|
||||
model: gpt-4.1-mini
|
||||
- name: security
|
||||
token_secret: SECURITY_REVIEW_TOKEN
|
||||
provider: openai
|
||||
llm_path: /openai/v1
|
||||
model: gpt-5
|
||||
system_prompt_file: SECURITY_REVIEW.md
|
||||
steps:
|
||||
@@ -70,10 +50,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 |
|
||||
@@ -188,6 +213,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:
|
||||
|
||||
@@ -0,0 +1,19 @@
|
||||
## Self-Review: feat/aicore-provider — 2026-05-09
|
||||
|
gpt-review-bot
commented
[NIT] Self-review notes (staticcheck findings, rationale) are committed to the repository. Consider moving these to an issue or PR description to avoid shipping internal review notes in the codebase. **[NIT]** Self-review notes (staticcheck findings, rationale) are committed to the repository. Consider moving these to an issue or PR description to avoid shipping internal review notes in the codebase.
sonnet-review-bot
commented
[NIT] Committing a **[NIT]** Committing a `TODO.md` containing a self-review into the main branch adds noise to the repository. This file is transient review scaffolding and would be better kept out of version control (e.g., in `.gitignore`) or removed before merge.
|
||||
|
||||
### Verdict: PASS
|
||||
|
||||
No blocking issues found — ready for human review.
|
||||
|
||||
#### Notes (informational, not blocking)
|
||||
|
||||
**[fit]** `staticcheck` reports:
|
||||
- `llm/aicore.go:237` and `llm/client.go:231`: struct literal conversion style (S1016) — minor style nit, existing in both old and new code
|
||||
- `gitea/diff.go:78`: HasPrefix return ignored (SA4017) — pre-existing, not introduced by this PR
|
||||
- `cmd/review-bot/main_test.go:347`: nil Context (SA1012) — pre-existing, not introduced by this PR
|
||||
|
||||
**[fit]** Body length validation: `aicore.go` does not include the Content-Length vs body length validation that `doRequest` has in `client.go`. This is acceptable because:
|
||||
1. AI Core uses OAuth tokens which are short-lived, so truncation is less likely
|
||||
2. The retry logic still applies via "read response" error pattern
|
||||
3. Adding complexity to aicore.go for an edge case that hasn't manifested is premature
|
||||
|
||||
**[completeness]** Tests pass (go test ./...), go vet clean, no uncommitted changes.
|
||||
@@ -69,7 +69,13 @@ func main() {
|
||||
dryRun := flag.Bool("dry-run", false, "Print review to stdout instead of posting")
|
||||
|
gpt-review-bot
commented
[NIT] When provider=aicore, a Client is constructed with empty base URL and API key, which is harmless but slightly confusing. A small comment or constructing the Client after provider validation could improve clarity. **[NIT]** When provider=aicore, a Client is constructed with empty base URL and API key, which is harmless but slightly confusing. A small comment or constructing the Client after provider validation could improve clarity.
|
||||
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")
|
||||
// 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)")
|
||||
aicoreResourceGroup := flag.String("aicore-resource-group", envOrDefault("AICORE_RESOURCE_GROUP", "default"), "SAP AI Core resource group (for provider=aicore)")
|
||||
|
||||
flag.Parse()
|
||||
|
||||
@@ -84,10 +90,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
|
||||
if *giteaURL == "" || *repo == "" || *prNum == "" || *reviewerToken == "" || *llmModel == "" {
|
||||
fmt.Fprintf(os.Stderr, "Error: missing required flags or environment variables\n\n")
|
||||
|
[NIT] When using provider=aicore, AuthURL and APIURL are accepted as provided. Consider warning or rejecting non-HTTPS URLs to avoid sending credentials or tokens over insecure transport. **[NIT]** When using provider=aicore, AuthURL and APIURL are accepted as provided. Consider warning or rejecting non-HTTPS URLs to avoid sending credentials or tokens over insecure transport.
|
||||
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 == "") {
|
||||
|
[MINOR] No validation that --aicore-auth-url and --aicore-api-url use HTTPS. A misconfiguration could transmit client credentials over plain HTTP. Enforce or warn on non-https schemes for these URLs. **[MINOR]** No validation that --aicore-auth-url and --aicore-api-url use HTTPS. A misconfiguration could transmit client credentials over plain HTTP. Enforce or warn on non-https schemes for these URLs.
|
||||
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")
|
||||
|
[MINOR] AI Core URLs (AICORE_AUTH_URL, AICORE_API_URL) are accepted as-is without enforcing or warning on non-HTTPS schemes. Requiring or warning on non-HTTPS would reduce risk of credential/token interception in misconfigurations. **[MINOR]** AI Core URLs (AICORE_AUTH_URL, AICORE_API_URL) are accepted as-is without enforcing or warning on non-HTTPS schemes. Requiring or warning on non-HTTPS would reduce risk of credential/token interception in misconfigurations.
|
||||
os.Exit(1)
|
||||
}
|
||||
|
||||
@@ -125,8 +141,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,381 @@
|
||||
package llm
|
||||
|
[MINOR] Unbounded response body reads (io.ReadAll) in token fetch, deployment listing, and completion requests can enable resource exhaustion if the remote endpoint misbehaves or is misconfigured. Consider wrapping resp.Body with an io.LimitedReader and capping the maximum read size, especially for error paths. **[MINOR]** Unbounded response body reads (io.ReadAll) in token fetch, deployment listing, and completion requests can enable resource exhaustion if the remote endpoint misbehaves or is misconfigured. Consider wrapping resp.Body with an io.LimitedReader and capping the maximum read size, especially for error paths.
[MINOR] Error messages include full upstream response bodies (e.g., on non-2xx for token/deployment/completion calls). This can leak provider or configuration details into logs. Consider truncating error bodies (e.g., first 1–2KB) and/or redacting sensitive fields before logging/returning. **[MINOR]** Error messages include full upstream response bodies (e.g., on non-2xx for token/deployment/completion calls). This can leak provider or configuration details into logs. Consider truncating error bodies (e.g., first 1–2KB) and/or redacting sensitive fields before logging/returning.
[MINOR] No validation that AICore AuthURL/APIURL use HTTPS before sending client credentials and tokens. Misconfiguration to an http:// URL could expose client_secret and tokens over cleartext. Enforce https scheme by default and allow overrides only via an explicit 'insecure' flag for controlled environments. **[MINOR]** No validation that AICore AuthURL/APIURL use HTTPS before sending client credentials and tokens. Misconfiguration to an http:// URL could expose client_secret and tokens over cleartext. Enforce https scheme by default and allow overrides only via an explicit 'insecure' flag for controlled environments.
gpt-review-bot
commented
[MINOR] Deployment cache never invalidates entries; if a previously RUNNING deployment stops, the cached URL may persist and cause repeated request failures. Consider invalidating on specific AI Core error responses (e.g., 404/410) or adding a periodic/TTL refresh. **[MINOR]** Deployment cache never invalidates entries; if a previously RUNNING deployment stops, the cached URL may persist and cause repeated request failures. Consider invalidating on specific AI Core error responses (e.g., 404/410) or adding a periodic/TTL refresh.
gpt-review-bot
commented
[MINOR] Deployment URL caching never expires or refreshes once populated; if a deployment is replaced or its URL changes, the cached mapping may become stale and subsequent requests will keep failing without a retry path or re-discovery. Consider a TTL or a retry-on-4xx/5xx to refresh deployments. **[MINOR]** Deployment URL caching never expires or refreshes once populated; if a deployment is replaced or its URL changes, the cached mapping may become stale and subsequent requests will keep failing without a retry path or re-discovery. Consider a TTL or a retry-on-4xx/5xx to refresh deployments.
gpt-review-bot
commented
[NIT] The AICoreOpenAIAPIVersion constant is set to a preview API version. Consider making this configurable or documenting the update process to reduce maintenance friction when versions change. **[NIT]** The AICoreOpenAIAPIVersion constant is set to a preview API version. Consider making this configurable or documenting the update process to reduce maintenance friction when versions change.
gpt-review-bot
commented
[MINOR] Network I/O is performed while holding the write mutex in getToken (fetchToken) and getDeploymentURL (fetchDeployments), which can block other callers for the duration of HTTP calls. Consider performing HTTP requests outside the lock and updating shared state under lock (or using singleflight) to reduce contention. **[MINOR]** Network I/O is performed while holding the write mutex in getToken (fetchToken) and getDeploymentURL (fetchDeployments), which can block other callers for the duration of HTTP calls. Consider performing HTTP requests outside the lock and updating shared state under lock (or using singleflight) to reduce contention.
gpt-review-bot
commented
[MINOR] Responses are read without validating Content-Length vs received body length, unlike doRequest in client.go. A truncated body would likely fail JSON unmarshal and be treated as a non-retryable parse error by Client.Complete, reducing robustness. Consider adding a Content-Length mismatch check (and returning a specific error) to preserve retry semantics for transient truncations. **[MINOR]** Responses are read without validating Content-Length vs received body length, unlike doRequest in client.go. A truncated body would likely fail JSON unmarshal and be treated as a non-retryable parse error by Client.Complete, reducing robustness. Consider adding a Content-Length mismatch check (and returning a specific error) to preserve retry semantics for transient truncations.
|
||||
|
||||
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"
|
||||
|
||||
// AICoreConfig holds SAP AI Core authentication and connection settings.
|
||||
type AICoreConfig struct {
|
||||
ClientID string
|
||||
ClientSecret string
|
||||
AuthURL string
|
||||
APIURL string
|
||||
ResourceGroup string
|
||||
}
|
||||
|
||||
// AICoreClient wraps AI Core authentication and deployment discovery.
|
||||
// Thread-safe for concurrent use after construction.
|
||||
type AICoreClient struct {
|
||||
config AICoreConfig
|
||||
http *http.Client
|
||||
|
||||
|
sonnet-review-bot
commented
[MINOR] The field **[MINOR]** The field `http *http.Client` shadows the `net/http` package name within the struct's method bodies. This is valid Go but can cause confusion — a reader seeing `c.http.Do(req)` has to remember it's a field, not the package. The existing `Client` in `client.go` has the same pattern, so this is consistent with the codebase, but renaming to `httpClient` would be cleaner.
|
||||
mu sync.RWMutex
|
||||
token string
|
||||
tokenExpiry time.Time
|
||||
deployments map[string]deployment // model name -> deployment info
|
||||
}
|
||||
|
||||
type deployment struct {
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `deployment.ID` field is stored but never read — it's populated in fetchDeployments but `getDeploymentURL` only returns `d.URL`. If the ID is truly unused, consider removing it to avoid dead code. If it's intended for future use (e.g., logging), add a comment.
|
||||
ID string
|
||||
URL string
|
||||
}
|
||||
|
||||
|
[NIT] AICoreClient uses an internal http.Client with a fixed 30s Timeout irrespective of the overall LLM timeout. While requests also honor context deadlines, the shorter client timeout may cause premature cancellations and inconsistent behavior. Consider aligning with the caller-provided context/timeout or making this configurable. **[NIT]** AICoreClient uses an internal http.Client with a fixed 30s Timeout irrespective of the overall LLM timeout. While requests also honor context deadlines, the shorter client timeout may cause premature cancellations and inconsistent behavior. Consider aligning with the caller-provided context/timeout or making this configurable.
gpt-review-bot
commented
[MAJOR] AICoreClient constructs an internal http.Client with a hard-coded 30s timeout and does not share or inherit the timeout set on llm.Client via WithTimeout. This creates inconsistent behavior: the top-level LLM timeout may be minutes, but AI Core calls will fail after 30s. Propagate the parent client's timeout or expose a way to configure AICoreClient's http timeout and set it from llm.Client. **[MAJOR]** AICoreClient constructs an internal http.Client with a hard-coded 30s timeout and does not share or inherit the timeout set on llm.Client via WithTimeout. This creates inconsistent behavior: the top-level LLM timeout may be minutes, but AI Core calls will fail after 30s. Propagate the parent client's timeout or expose a way to configure AICoreClient's http timeout and set it from llm.Client.
|
||||
// NewAICoreClient creates a new AI Core client with the given configuration.
|
||||
|
gpt-review-bot
commented
[MAJOR] AICoreClient initializes its own http.Client with a hard-coded 30s timeout, ignoring the user-configured LLM timeout. This causes AI Core requests to time out at 30s even when --llm-timeout is larger, diverging from documented behavior. **[MAJOR]** AICoreClient initializes its own http.Client with a hard-coded 30s timeout, ignoring the user-configured LLM timeout. This causes AI Core requests to time out at 30s even when --llm-timeout is larger, diverging from documented behavior.
|
||||
// The client uses a default 5-minute timeout; use WithTimeout to customize.
|
||||
func NewAICoreClient(cfg AICoreConfig) *AICoreClient {
|
||||
return &AICoreClient{
|
||||
config: cfg,
|
||||
http: &http.Client{Timeout: 5 * time.Minute},
|
||||
deployments: make(map[string]deployment),
|
||||
}
|
||||
|
sonnet-review-bot
commented
[NIT] The **[NIT]** The `deployment` struct has an exported `ID` and `URL` field but the type is unexported. The `ID` field is stored in the map but never read back — only `URL` is used. The `ID` field could be removed unless planned for future use (e.g., logging, metrics).
|
||||
}
|
||||
|
||||
// WithTimeout sets the HTTP request timeout for AI Core calls.
|
||||
// This should be called during construction, before concurrent use.
|
||||
|
sonnet-review-bot
commented
[NIT] The field name **[NIT]** The field name `http` shadows the imported `net/http` package name within the struct. While Go allows this (the package is still accessible by its full import path), it's a subtle naming collision. The existing `llm/client.go` already uses the same pattern so this is consistent with the codebase, but worth noting as a readability concern.
|
||||
func (c *AICoreClient) WithTimeout(d time.Duration) *AICoreClient {
|
||||
c.http.Timeout = d
|
||||
return c
|
||||
}
|
||||
|
||||
// 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] AuthURL/APIURL scheme is not validated. If misconfigured to use http://, the client ID/secret and tokens would be sent in cleartext. Consider enforcing https or at least logging a warning when non-HTTPS endpoints are configured to reduce risk of credential exposure. **[MINOR]** AuthURL/APIURL scheme is not validated. If misconfigured to use http://, the client ID/secret and tokens would be sent in cleartext. Consider enforcing https or at least logging a warning when non-HTTPS endpoints are configured to reduce risk of credential exposure.
|
||||
defer c.mu.Unlock()
|
||||
|
||||
// Double-check after acquiring write lock
|
||||
if c.token != "" && time.Now().Add(5*time.Minute).Before(c.tokenExpiry) {
|
||||
return c.token, nil
|
||||
|
[MINOR] AuthURL/APIURL are accepted as-is and used to construct requests without verifying the scheme. If misconfigured to use plain HTTP, client credentials could be sent in cleartext. Consider validating that AuthURL and APIURL use https:// (or at least warning/refusing non-HTTPS) to prevent accidental credential exposure. **[MINOR]** AuthURL/APIURL are accepted as-is and used to construct requests without verifying the scheme. If misconfigured to use plain HTTP, client credentials could be sent in cleartext. Consider validating that AuthURL and APIURL use https:// (or at least warning/refusing non-HTTPS) to prevent accidental credential exposure.
|
||||
}
|
||||
|
||||
token, expiry, err := c.fetchToken(ctx)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
c.token = token
|
||||
c.tokenExpiry = expiry
|
||||
|
[MINOR] fetchToken constructs the token endpoint from AICoreConfig.AuthURL without enforcing HTTPS. If misconfigured to use http, client credentials would be sent in cleartext. Consider validating that AuthURL uses the https scheme (and fail or warn otherwise). **[MINOR]** fetchToken constructs the token endpoint from AICoreConfig.AuthURL without enforcing HTTPS. If misconfigured to use http, client credentials would be sent in cleartext. Consider validating that AuthURL uses the https scheme (and fail or warn otherwise).
|
||||
return token, nil
|
||||
}
|
||||
|
||||
func (c *AICoreClient) fetchToken(ctx context.Context) (string, time.Time, error) {
|
||||
tokenURL := strings.TrimRight(c.config.AuthURL, "/") + "/oauth/token"
|
||||
|
||||
data := url.Values{}
|
||||
data.Set("grant_type", "client_credentials")
|
||||
data.Set("client_id", c.config.ClientID)
|
||||
data.Set("client_secret", c.config.ClientSecret)
|
||||
|
||||
|
[MINOR] On token fetch failure, the error includes the full response body ("token request failed ...: %s"). While the auth server typically won't echo secrets, returning raw bodies can leak potentially sensitive diagnostic details into logs. Consider redacting or truncating the body in errors. **[MINOR]** On token fetch failure, the error includes the full response body ("token request failed ...: %s"). While the auth server typically won't echo secrets, returning raw bodies can leak potentially sensitive diagnostic details into logs. Consider redacting or truncating the body in errors.
|
||||
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)
|
||||
}
|
||||
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
|
||||
|
||||
|
[MINOR] On authentication and API failures, the code includes the full upstream response body in returned errors (e.g., token and deployments requests, and AI Core invoke errors). While convenient for debugging, this can leak detailed service information into logs. Consider redacting or truncating bodies on errors, or limiting to status code and a short message. **[MINOR]** On authentication and API failures, the code includes the full upstream response body in returned errors (e.g., token and deployments requests, and AI Core invoke errors). While convenient for debugging, this can leak detailed service information into logs. Consider redacting or truncating bodies on errors, or limiting to status code and a short message.
|
||||
resp, err := c.http.Do(req)
|
||||
if err != nil {
|
||||
|
[MINOR] On non-2xx responses from the OAuth or AI Core APIs, the code includes the full response body in returned errors (e.g., "token request failed ...: %s"). Upstream callers log these errors, which can inadvertently leak request/response details to CI logs. Consider truncating or redacting error bodies before returning/logging. **[MINOR]** On non-2xx responses from the OAuth or AI Core APIs, the code includes the full response body in returned errors (e.g., "token request failed ...: %s"). Upstream callers log these errors, which can inadvertently leak request/response details to CI logs. Consider truncating or redacting error bodies before returning/logging.
|
||||
return "", time.Time{}, fmt.Errorf("token request: %w", err)
|
||||
|
[MINOR] Unbounded response body read from the token endpoint (io.ReadAll on resp.Body). If the AuthURL is misconfigured or points to a hostile server, this can lead to excessive memory usage. Consider adding a reasonable limit via io.LimitReader or validating Content-Length and capping size. **[MINOR]** Unbounded response body read from the token endpoint (io.ReadAll on resp.Body). If the AuthURL is misconfigured or points to a hostile server, this can lead to excessive memory usage. Consider adding a reasonable limit via io.LimitReader or validating Content-Length and capping size.
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
|
||||
body, err := io.ReadAll(resp.Body)
|
||||
if err != nil {
|
||||
|
[MINOR] Error message includes full response body from the token endpoint (fmt.Errorf("token request failed ...: %s", body)). This can leak sensitive details from upstream error payloads into logs. Consider truncating or redacting bodies in errors, especially for auth endpoints. **[MINOR]** Error message includes full response body from the token endpoint (fmt.Errorf("token request failed ...: %s", body)). This can leak sensitive details from upstream error payloads into logs. Consider truncating or redacting bodies in errors, especially for auth endpoints.
|
||||
return "", time.Time{}, fmt.Errorf("read token response: %w", err)
|
||||
|
[MINOR] Error paths include the full response body in the returned error (token request and AI Core API errors). This can leak potentially sensitive server responses into logs. Consider truncating bodies in errors or logging them at a debug level only. **[MINOR]** Error paths include the full response body in the returned error (token request and AI Core API errors). This can leak potentially sensitive server responses into logs. Consider truncating bodies in errors or logging them at a debug level only.
|
||||
}
|
||||
|
||||
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
||||
return "", time.Time{}, fmt.Errorf("token request failed (status %d): %s", resp.StatusCode, string(body))
|
||||
|
[MINOR] Unbounded response reads with io.ReadAll can lead to memory exhaustion if the endpoint returns very large bodies. Consider wrapping resp.Body with io.LimitReader or streaming decode (json.Decoder). This pattern occurs in multiple places (fetchToken, fetchDeployments, CompleteAnthropic, CompleteOpenAI). **[MINOR]** Unbounded response reads with io.ReadAll can lead to memory exhaustion if the endpoint returns very large bodies. Consider wrapping resp.Body with io.LimitReader or streaming decode (json.Decoder). This pattern occurs in multiple places (fetchToken, fetchDeployments, CompleteAnthropic, CompleteOpenAI).
|
||||
}
|
||||
|
||||
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)
|
||||
}
|
||||
|
||||
if tokenResp.AccessToken == "" {
|
||||
|
[MINOR] Error returns include the full response body on token request failure (fmt.Errorf("token request failed ...: %s", body)). This can lead to information leakage in logs if upstream returns verbose error details. Consider truncating or redacting bodies in errors. **[MINOR]** Error returns include the full response body on token request failure (fmt.Errorf("token request failed ...: %s", body)). This can lead to information leakage in logs if upstream returns verbose error details. Consider truncating or redacting bodies in errors.
|
||||
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
|
||||
|
[MINOR] The OAuth token request uses the default http.Client redirect behavior, which will resend the POST body on 307/308 redirects. If the AuthURL endpoint (or a misconfigured proxy) issues a cross-host 307/308, client_id/client_secret could be sent to a different host. Consider setting a CheckRedirect function to prevent cross-host redirects or disallow redirects for the token endpoint. **[MINOR]** The OAuth token request uses the default http.Client redirect behavior, which will resend the POST body on 307/308 redirects. If the AuthURL endpoint (or a misconfigured proxy) issues a cross-host 307/308, client_id/client_secret could be sent to a different host. Consider setting a CheckRedirect function to prevent cross-host redirects or disallow redirects for the token endpoint.
|
||||
}
|
||||
|
||||
// getDeploymentURL returns the deployment URL for a model, fetching deployments if needed.
|
||||
func (c *AICoreClient) getDeploymentURL(ctx context.Context, model string) (string, error) {
|
||||
c.mu.RLock()
|
||||
if d, ok := c.deployments[model]; ok {
|
||||
c.mu.RUnlock()
|
||||
return d.URL, nil
|
||||
}
|
||||
c.mu.RUnlock()
|
||||
|
||||
// Fetch token first (before acquiring write lock to avoid deadlock)
|
||||
token, err := c.getToken(ctx)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("get token for deployments: %w", err)
|
||||
}
|
||||
|
gpt-review-bot
commented
[MINOR] Deployment URL cache never invalidates for a model once populated. If a deployment stops or changes, the client may keep using a stale URL. Consider a TTL on entries or refreshing on specific HTTP errors (e.g., 404/410) to keep mappings current. **[MINOR]** Deployment URL cache never invalidates for a model once populated. If a deployment stops or changes, the client may keep using a stale URL. Consider a TTL on entries or refreshing on specific HTTP errors (e.g., 404/410) to keep mappings current.
|
||||
|
||||
c.mu.Lock()
|
||||
|
[MINOR] For non-2xx responses from AI Core requests (both /invoke and /chat/completions), errors include the full response body. Depending on backend behavior, this may reflect parts of the prompt or internal details into logs. Prefer redacting or truncating bodies in error messages. **[MINOR]** For non-2xx responses from AI Core requests (both /invoke and /chat/completions), errors include the full response body. Depending on backend behavior, this may reflect parts of the prompt or internal details into logs. Prefer redacting or truncating bodies in error messages.
|
||||
defer c.mu.Unlock()
|
||||
|
||||
|
gpt-review-bot
commented
[MINOR] getDeploymentURL holds the write mutex while calling fetchDeployments, which performs network I/O. This can block all concurrent callers unnecessarily; consider releasing the lock before the request and re-acquiring it only to update the cache (double-check pattern), or use a singleflight to dedupe concurrent fetches. **[MINOR]** getDeploymentURL holds the write mutex while calling fetchDeployments, which performs network I/O. This can block all concurrent callers unnecessarily; consider releasing the lock before the request and re-acquiring it only to update the cache (double-check pattern), or use a singleflight to dedupe concurrent fetches.
|
||||
// Double-check after acquiring write lock
|
||||
|
gpt-review-bot
commented
[MINOR] getDeploymentURL holds the write lock while performing the network request in fetchDeployments, which can block other callers under load. Consider releasing the lock before the HTTP call and re-acquiring it to update the cache (double-checking first) to avoid long lock holds. **[MINOR]** getDeploymentURL holds the write lock while performing the network request in fetchDeployments, which can block other callers under load. Consider releasing the lock before the HTTP call and re-acquiring it to update the cache (double-checking first) to avoid long lock holds.
|
||||
if d, ok := c.deployments[model]; ok {
|
||||
return d.URL, nil
|
||||
}
|
||||
|
[MINOR] The code trusts and directly uses the deploymentUrl returned by the AI Core API to construct subsequent requests (appending /invoke or /chat/completions). While appropriate in normal operation, this creates a potential SSRF or token exfiltration vector if the AI Core API or configuration is compromised. As a defense-in-depth measure, consider validating that deploymentUrl shares the expected host/scheme with the configured APIURL. **[MINOR]** The code trusts and directly uses the deploymentUrl returned by the AI Core API to construct subsequent requests (appending /invoke or /chat/completions). While appropriate in normal operation, this creates a potential SSRF or token exfiltration vector if the AI Core API or configuration is compromised. As a defense-in-depth measure, consider validating that deploymentUrl shares the expected host/scheme with the configured APIURL.
|
||||
|
||||
|
gpt-review-bot
commented
[MINOR] The deployment cache never refreshes once populated. If a deployment URL changes or a model becomes unavailable, the client may hold stale entries. Consider refreshing on 404/5xx from the invoke endpoint or adding a TTL/refresh strategy. **[MINOR]** The deployment cache never refreshes once populated. If a deployment URL changes or a model becomes unavailable, the client may hold stale entries. Consider refreshing on 404/5xx from the invoke endpoint or adding a TTL/refresh strategy.
|
||||
if err := c.fetchDeployments(ctx, token); err != nil {
|
||||
|
gpt-review-bot
commented
[MINOR] getDeploymentURL calls fetchDeployments while holding the write mutex, performing network I/O under lock. This can cause unnecessary contention and head-of-line blocking. Consider releasing the lock before the HTTP call and re-acquiring it only to update the shared map. **[MINOR]** getDeploymentURL calls fetchDeployments while holding the write mutex, performing network I/O under lock. This can cause unnecessary contention and head-of-line blocking. Consider releasing the lock before the HTTP call and re-acquiring it only to update the shared map.
gpt-review-bot
commented
[MINOR] getDeploymentURL performs a blocking network request (fetchDeployments) while holding c.mu.Lock. This can block concurrent readers/writers longer than necessary. Consider releasing the lock before the HTTP call and reacquiring just to update c.deployments, using a double-check to avoid redundant fetches. **[MINOR]** getDeploymentURL performs a blocking network request (fetchDeployments) while holding c.mu.Lock. This can block concurrent readers/writers longer than necessary. Consider releasing the lock before the HTTP call and reacquiring just to update c.deployments, using a double-check to avoid redundant fetches.
|
||||
return "", err
|
||||
}
|
||||
|
||||
if d, ok := c.deployments[model]; ok {
|
||||
|
sonnet-review-bot
commented
[MINOR] In **[MINOR]** In `getDeploymentURL`, the token is fetched *before* acquiring the write lock (correct to avoid deadlock with getToken's own lock), but then `fetchDeployments` is called while holding the write lock AND passes the token in. If `fetchDeployments` takes a long time (slow network), all readers are blocked for the duration. This is acceptable for a CLI tool but worth noting — in a high-concurrency server scenario a different approach would be needed.
|
||||
return d.URL, nil
|
||||
|
sonnet-review-bot
commented
[MINOR] Deployment cache has no TTL or invalidation mechanism. If a deployment goes from RUNNING to STOPPED (or a new deployment is added), the cached entry will be used until the process restarts. Consider adding a deployment cache TTL (e.g., 5-10 minutes) similar to the token refresh guard, or at least document this limitation. This mirrors the concern raised about the deployment URL being cached indefinitely once fetched. **[MINOR]** Deployment cache has no TTL or invalidation mechanism. If a deployment goes from RUNNING to STOPPED (or a new deployment is added), the cached entry will be used until the process restarts. Consider adding a deployment cache TTL (e.g., 5-10 minutes) similar to the token refresh guard, or at least document this limitation. This mirrors the concern raised about the deployment URL being cached indefinitely once fetched.
|
||||
}
|
||||
return "", fmt.Errorf("no deployment found for model %q", model)
|
||||
}
|
||||
|
||||
func (c *AICoreClient) fetchDeployments(ctx context.Context, token string) error {
|
||||
|
sonnet-review-bot
commented
[MINOR] Deployments are fetched once and cached forever with no TTL or invalidation mechanism. If a deployment goes from RUNNING to STOPPED (or a new deployment is added) between reviews, the cached URL will remain stale until the process restarts. Given this tool runs as a short-lived CI process this is acceptable, but worth a comment explaining the intentional lack of cache expiry. **[MINOR]** Deployments are fetched once and cached forever with no TTL or invalidation mechanism. If a deployment goes from RUNNING to STOPPED (or a new deployment is added) between reviews, the cached URL will remain stale until the process restarts. Given this tool runs as a short-lived CI process this is acceptable, but worth a comment explaining the intentional lack of cache expiry.
|
||||
deployURL := strings.TrimRight(c.config.APIURL, "/") + "/v2/lm/deployments"
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodGet, deployURL, nil)
|
||||
if err != nil {
|
||||
|
[MINOR] Header value for AI-Resource-Group is taken from unvalidated input and inserted into an HTTP header. While Go's net/http prevents CRLF injection in headers, consider validating this value (e.g., allow only [A-Za-z0-9-_]) to reduce risk and fail fast on invalid configuration. **[MINOR]** Header value for AI-Resource-Group is taken from unvalidated input and inserted into an HTTP header. While Go's net/http prevents CRLF injection in headers, consider validating this value (e.g., allow only [A-Za-z0-9-_]) to reduce risk and fail fast on invalid configuration.
|
||||
return fmt.Errorf("create deployments request: %w", err)
|
||||
}
|
||||
req.Header.Set("Authorization", "Bearer "+token)
|
||||
req.Header.Set("AI-Resource-Group", c.config.ResourceGroup)
|
||||
|
||||
resp, err := c.http.Do(req)
|
||||
if err != nil {
|
||||
return fmt.Errorf("deployments request: %w", err)
|
||||
|
[MINOR] Unbounded response body read for deployments discovery (io.ReadAll). Similar DoS risk as above if APIURL is misconfigured or untrusted. Add a body size cap or Content-Length validation. **[MINOR]** Unbounded response body read for deployments discovery (io.ReadAll). Similar DoS risk as above if APIURL is misconfigured or untrusted. Add a body size cap or Content-Length validation.
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
|
||||
body, err := io.ReadAll(resp.Body)
|
||||
if err != nil {
|
||||
|
[MINOR] Error path for deployments includes the entire response body in the error message. Consider truncating or redacting to avoid leaking potentially sensitive upstream details. **[MINOR]** Error path for deployments includes the entire response body in the error message. Consider truncating or redacting to avoid leaking potentially sensitive upstream details.
|
||||
return fmt.Errorf("read deployments response: %w", err)
|
||||
}
|
||||
|
||||
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
||||
return fmt.Errorf("deployments request failed (status %d): %s", resp.StatusCode, string(body))
|
||||
}
|
||||
|
||||
var deployResp struct {
|
||||
Resources []struct {
|
||||
ID string `json:"id"`
|
||||
DeploymentURL string `json:"deploymentUrl"`
|
||||
|
sonnet-review-bot
commented
[MINOR] fetchDeployments has an unnecessary blank line after the function signature ( **[MINOR]** fetchDeployments has an unnecessary blank line after the function signature (`func (c *AICoreClient) fetchDeployments(...) error {\n\n`). Cosmetic but inconsistent with the rest of the file style.
|
||||
Status string `json:"status"`
|
||||
Details struct {
|
||||
|
sonnet-review-bot
commented
[MINOR] Deployments are cached indefinitely after first fetch. If a deployment is restarted, updated, or a new one is added for a model, the client will keep using the stale cached URL until the process restarts. Consider adding a TTL or a mechanism to refresh on 4xx/5xx errors from the deployment URL, similar to how **[MINOR]** Deployments are cached indefinitely after first fetch. If a deployment is restarted, updated, or a new one is added for a model, the client will keep using the stale cached URL until the process restarts. Consider adding a TTL or a mechanism to refresh on 4xx/5xx errors from the deployment URL, similar to how `tokenExpiry` handles token refresh.
|
||||
Resources struct {
|
||||
BackendDetails struct {
|
||||
|
[MINOR] Error returns include the full response body on deployments request failure (fmt.Errorf("deployments request failed ...: %s", body)). This may leak internal details to logs. Prefer limiting or redacting response bodies in error messages. **[MINOR]** Error returns include the full response body on deployments request failure (fmt.Errorf("deployments request failed ...: %s", body)). This may leak internal details to logs. Prefer limiting or redacting response bodies in error messages.
|
||||
Model struct {
|
||||
Name string `json:"name"`
|
||||
} `json:"model"`
|
||||
} `json:"backend_details"`
|
||||
} `json:"resources"`
|
||||
|
sonnet-review-bot
commented
[MINOR] If a model has multiple RUNNING deployments (as the test demonstrates for 'gpt-5' with deploy-456 STOPPED and deploy-789 RUNNING), the final write wins in the loop since the map is overwritten. In the test, deploy-456 is STOPPED so it is filtered out and deploy-789 wins correctly. However, if two deployments for the same model name are both RUNNING, the result is nondeterministic (last one parsed wins). This edge case should either be documented or handled explicitly (e.g., pick the first RUNNING, or error). **[MINOR]** If a model has multiple RUNNING deployments (as the test demonstrates for 'gpt-5' with deploy-456 STOPPED and deploy-789 RUNNING), the final write wins in the loop since the map is overwritten. In the test, deploy-456 is STOPPED so it is filtered out and deploy-789 wins correctly. However, if two deployments for the same model name are both RUNNING, the result is nondeterministic (last one parsed wins). This edge case should either be documented or handled explicitly (e.g., pick the first RUNNING, or error).
|
||||
} `json:"details"`
|
||||
} `json:"resources"`
|
||||
|
[MINOR] Unbounded io.ReadAll in fetchDeployments; a malicious or misconfigured APIURL could cause large response bodies to be read into memory. Apply a reasonable size cap or stream the JSON. **[MINOR]** Unbounded io.ReadAll in fetchDeployments; a malicious or misconfigured APIURL could cause large response bodies to be read into memory. Apply a reasonable size cap or stream the JSON.
|
||||
}
|
||||
if err := json.Unmarshal(body, &deployResp); err != nil {
|
||||
return fmt.Errorf("parse deployments response: %w", err)
|
||||
}
|
||||
|
||||
for _, r := range deployResp.Resources {
|
||||
if r.Status != "RUNNING" {
|
||||
continue
|
||||
|
gpt-review-bot
commented
[MINOR] The deployments cache is only ever appended to and never invalidated or refreshed (e.g., if a deployment transitions from RUNNING to STOPPED). Consider replacing the map on each fetch or introducing a TTL/refresh strategy to avoid serving stale URLs. **[MINOR]** The deployments cache is only ever appended to and never invalidated or refreshed (e.g., if a deployment transitions from RUNNING to STOPPED). Consider replacing the map on each fetch or introducing a TTL/refresh strategy to avoid serving stale URLs.
|
||||
}
|
||||
modelName := r.Details.Resources.BackendDetails.Model.Name
|
||||
if modelName == "" {
|
||||
continue
|
||||
}
|
||||
c.deployments[modelName] = deployment{
|
||||
ID: r.ID,
|
||||
URL: r.DeploymentURL,
|
||||
}
|
||||
}
|
||||
|
||||
return nil
|
||||
}
|
||||
|
||||
// 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) {
|
||||
deployURL, err := c.getDeploymentURL(ctx, model)
|
||||
if err != nil {
|
||||
|
[NIT] fetchDeployments trusts the deploymentUrl from the AI Core control plane and later sends Bearer tokens to that URL. As a defense-in-depth measure, consider validating that the deploymentUrl host matches (or is a subdomain of) the expected API domain to reduce risk of token exfiltration if the control plane is misconfigured. **[NIT]** fetchDeployments trusts the deploymentUrl from the AI Core control plane and later sends Bearer tokens to that URL. As a defense-in-depth measure, consider validating that the deploymentUrl host matches (or is a subdomain of) the expected API domain to reduce risk of token exfiltration if the control plane is misconfigured.
|
||||
return "", err
|
||||
}
|
||||
|
||||
token, err := c.getToken(ctx)
|
||||
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
[MINOR] The OpenAI-style endpoint appends a hard-coded api-version (2024-12-01-preview). Consider making this configurable (or at least a package constant) to avoid breaking changes when AI Core updates versions. **[MINOR]** The OpenAI-style endpoint appends a hard-coded api-version (2024-12-01-preview). Consider making this configurable (or at least a package constant) to avoid breaking changes when AI Core updates versions.
|
||||
system = m.Content
|
||||
} else {
|
||||
userMessages = append(userMessages, anthropicMsg{
|
||||
Role: m.Role,
|
||||
Content: m.Content,
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
reqBody := anthropicRequest{
|
||||
AnthropicVersion: "bedrock-2023-05-31", // SAP AI Core uses Bedrock format
|
||||
// Model omitted - AI Core deployment already specifies model
|
||||
MaxTokens: maxTokens,
|
||||
System: system,
|
||||
Messages: userMessages,
|
||||
}
|
||||
|
sonnet-review-bot
commented
[NIT] In CompleteAnthropic, **[NIT]** In CompleteAnthropic, `getDeploymentURL` and then `getToken` are called sequentially. `getDeploymentURL` internally already calls `getToken` (to fetch deployments), so the second explicit `getToken` call will typically be a cache hit. This is correct and safe, but a comment explaining the double-call or refactoring to return the token from `getDeploymentURL` would improve readability.
|
||||
if temperature > 0 {
|
||||
reqBody.Temperature = temperature
|
||||
}
|
||||
|
||||
data, err := json.Marshal(reqBody)
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("marshal request: %w", err)
|
||||
|
[MINOR] The code trusts the deploymentUrl returned by the AI Core API and directly constructs the invoke endpoint without validating the URL's scheme/host. An attacker controlling the API endpoint (or misconfiguration) could return a malicious URL, leading to SSRF attempts from the runner. Consider parsing and validating that the URL uses https and matches an expected domain or host pattern. **[MINOR]** The code trusts the deploymentUrl returned by the AI Core API and directly constructs the invoke endpoint without validating the URL's scheme/host. An attacker controlling the API endpoint (or misconfiguration) could return a malicious URL, leading to SSRF attempts from the runner. Consider parsing and validating that the URL uses https and matches an expected domain or host pattern.
|
||||
}
|
||||
|
||||
// AI Core uses /invoke for Anthropic models
|
||||
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")
|
||||
|
||||
|
sonnet-review-bot
commented
[MINOR] The **[MINOR]** The `TODO.md` self-review correctly notes that `CompleteAnthropic` and `CompleteOpenAI` lack the Content-Length body validation that `doRequest` has in `client.go`. Given these methods duplicate the HTTP execution pattern rather than going through `doRequest`, any future fix to the retry/validation logic in `doRequest` won't automatically apply here. Consider extracting a shared HTTP execution helper for AI Core, or routing through `doRequest` via an adapter.
|
||||
resp, err := c.http.Do(req)
|
||||
if err != nil {
|
||||
|
[MINOR] Unbounded response body read for Anthropic invoke responses (io.ReadAll). To defend against large or malformed payloads from AI Core or misconfiguration, consider adding a read limit or Content-Length checks like in llm.Client.doRequest. **[MINOR]** Unbounded response body read for Anthropic invoke responses (io.ReadAll). To defend against large or malformed payloads from AI Core or misconfiguration, consider adding a read limit or Content-Length checks like in llm.Client.doRequest.
gpt-review-bot
commented
[MINOR] AI Core request paths do not validate response body length against Content-Length like client.doRequest does. Adding a similar check (and treating mismatches as retryable) would harden against truncated responses. This applies to both CompleteAnthropic and CompleteOpenAI. **[MINOR]** AI Core request paths do not validate response body length against Content-Length like client.doRequest does. Adding a similar check (and treating mismatches as retryable) would harden against truncated responses. This applies to both CompleteAnthropic and CompleteOpenAI.
|
||||
return "", fmt.Errorf("AI Core request: %w", err)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
|
||||
body, err := io.ReadAll(resp.Body)
|
||||
|
[MINOR] Anthropic error path includes full response body in error. Truncate or sanitize to reduce risk of leaking sensitive content into logs. **[MINOR]** Anthropic error path includes full response body in error. Truncate or sanitize to reduce risk of leaking sensitive content into logs.
gpt-review-bot
commented
[MINOR] CompleteAnthropic reads the response body without validating against Content-Length. For parity with the OpenAI path and better detection of truncated responses, consider checking resp.ContentLength and returning a specific error when len(body) < Content-Length. **[MINOR]** CompleteAnthropic reads the response body without validating against Content-Length. For parity with the OpenAI path and better detection of truncated responses, consider checking resp.ContentLength and returning a specific error when len(body) < Content-Length.
|
||||
if err != nil {
|
||||
|
[MINOR] Unbounded io.ReadAll in CompleteAnthropic; add a maximum body size to mitigate potential memory exhaustion if the server returns an unexpectedly large payload. **[MINOR]** Unbounded io.ReadAll in CompleteAnthropic; add a maximum body size to mitigate potential memory exhaustion if the server returns an unexpectedly large payload.
|
||||
return "", fmt.Errorf("read response: %w", err)
|
||||
}
|
||||
|
gpt-review-bot
commented
[MINOR] Response handling in CompleteAnthropic does not validate the received body length against Content-Length like llm.Client.doRequest does. Consider adding a similar check to detect truncated responses. **[MINOR]** Response handling in CompleteAnthropic does not validate the received body length against Content-Length like llm.Client.doRequest does. Consider adding a similar check to detect truncated responses.
|
||||
|
||||
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
||||
return "", fmt.Errorf("AI Core API error (status %d): %s", resp.StatusCode, string(body))
|
||||
}
|
||||
|
||||
var anthropicResp anthropicResponse
|
||||
if err := json.Unmarshal(body, &anthropicResp); err != nil {
|
||||
return "", fmt.Errorf("parse response: %w", err)
|
||||
}
|
||||
|
[NIT] Error messages include the full response body on non-2xx (e.g., in CompleteAnthropic/OpenAI). This can leak potentially sensitive prompt or model output to logs on failures. Consider truncating/redacting bodies in errors. **[NIT]** Error messages include the full response body on non-2xx (e.g., in CompleteAnthropic/OpenAI). This can leak potentially sensitive prompt or model output to logs on failures. Consider truncating/redacting bodies in errors.
|
||||
|
||||
if len(anthropicResp.Content) == 0 {
|
||||
return "", fmt.Errorf("no content in response")
|
||||
|
[MINOR] CompleteAnthropic reads the response body without validating it against Content-Length, so a truncated response could be parsed if it remains valid JSON. Consider adding the same Content-Length vs body-length validation used in llm/client.go#doRequest to detect truncation before attempting to parse. **[MINOR]** CompleteAnthropic reads the response body without validating it against Content-Length, so a truncated response could be parsed if it remains valid JSON. Consider adding the same Content-Length vs body-length validation used in llm/client.go#doRequest to detect truncation before attempting to parse.
|
||||
}
|
||||
|
||||
var sb strings.Builder
|
||||
for _, block := range anthropicResp.Content {
|
||||
if block.Type == "text" {
|
||||
sb.WriteString(block.Text)
|
||||
}
|
||||
}
|
||||
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, err := c.getDeploymentURL(ctx, model)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
|
||||
token, err := c.getToken(ctx)
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
|
||||
|
gpt-review-bot
commented
[NIT] The OpenAI chat endpoint includes a hard-coded API version string ("2024-12-01-preview"). Consider defining this as a named constant for readability and easier future updates. **[NIT]** The OpenAI chat endpoint includes a hard-coded API version string ("2024-12-01-preview"). Consider defining this as a named constant for readability and easier future updates.
|
||||
reqBody := ChatRequest{
|
||||
Model: model,
|
||||
Temperature: temperature,
|
||||
Messages: messages,
|
||||
}
|
||||
|
||||
data, err := json.Marshal(reqBody)
|
||||
|
sonnet-review-bot
commented
[NIT] CompleteAnthropic and CompleteOpenAI are public methods on AICoreClient. Since the intended consumer is only **[NIT]** CompleteAnthropic and CompleteOpenAI are public methods on AICoreClient. Since the intended consumer is only `completeAICore` in client.go (internal routing), exporting these gives callers the ability to bypass the routing logic (calling CompleteOpenAI with an Anthropic model name, for example). Consider making them unexported (`completeAnthropic`, `completeOpenAI`) since they're not part of any documented public API.
|
||||
if err != nil {
|
||||
return "", fmt.Errorf("marshal request: %w", err)
|
||||
|
[MINOR] Same SSRF concern for OpenAI-style models: the chat/completions URL is derived directly from deploymentUrl without scheme/host validation. Add URL parsing and enforce https and allowed host/domain. **[MINOR]** Same SSRF concern for OpenAI-style models: the chat/completions URL is derived directly from deploymentUrl without scheme/host validation. Add URL parsing and enforce https and allowed host/domain.
|
||||
}
|
||||
|
||||
// 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))
|
||||
|
gpt-review-bot
commented
[MINOR] Response handling in CompleteOpenAI does not validate body length against Content-Length. Align with the OpenAI/Anthropic client path by adding a Content-Length vs received length check to detect truncation. **[MINOR]** Response handling in CompleteOpenAI does not validate body length against Content-Length. Align with the OpenAI/Anthropic client path by adding a Content-Length vs received length check to detect truncation.
|
||||
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")
|
||||
|
||||
|
gpt-review-bot
commented
[MINOR] The OpenAI-through-AI-Core endpoint hard-codes the api-version ("2024-12-01-preview"). Consider making this a constant or configurable to avoid future breakage when AI Core updates versions. **[MINOR]** The OpenAI-through-AI-Core endpoint hard-codes the api-version ("2024-12-01-preview"). Consider making this a constant or configurable to avoid future breakage when AI Core updates versions.
|
||||
resp, err := c.http.Do(req)
|
||||
if err != nil {
|
||||
|
[MINOR] Unbounded response body read for OpenAI-compatible chat responses via AI Core. Consider adding a maximum body size or Content-Length validation for defense-in-depth. **[MINOR]** Unbounded response body read for OpenAI-compatible chat responses via AI Core. Consider adding a maximum body size or Content-Length validation for defense-in-depth.
[MINOR] CompleteOpenAI also reads the response body without Content-Length validation. Add a length check similar to llm/client.go#doRequest to guard against truncated responses. **[MINOR]** CompleteOpenAI also reads the response body without Content-Length validation. Add a length check similar to llm/client.go#doRequest to guard against truncated responses.
|
||||
return "", fmt.Errorf("AI Core request: %w", err)
|
||||
}
|
||||
defer resp.Body.Close()
|
||||
|
||||
body, err := io.ReadAll(resp.Body)
|
||||
|
gpt-review-bot
commented
[NIT] IsAnthropicModel uses a broad contains("claude") check, which may misclassify non-Anthropic models containing 'claude' in their name. Consider restricting detection further (e.g., prefix match or explicit allowlist). **[NIT]** IsAnthropicModel uses a broad contains("claude") check, which may misclassify non-Anthropic models containing 'claude' in their name. Consider restricting detection further (e.g., prefix match or explicit allowlist).
[MINOR] OpenAI-compatible error path includes full response body in error. Recommend truncating or redacting the body in error messages to avoid potential sensitive data exposure. **[MINOR]** OpenAI-compatible error path includes full response body in error. Recommend truncating or redacting the body in error messages to avoid potential sensitive data exposure.
gpt-review-bot
commented
[MINOR] CompleteOpenAI also omits Content-Length vs body length validation. Adding the same check here improves robustness against truncated responses and enables the retry logic to trigger on body length mismatch. **[MINOR]** CompleteOpenAI also omits Content-Length vs body length validation. Adding the same check here improves robustness against truncated responses and enables the retry logic to trigger on body length mismatch.
|
||||
if err != nil {
|
||||
|
[MINOR] Unbounded io.ReadAll in CompleteOpenAI; consider io.LimitReader or streaming decode to limit memory use. **[MINOR]** Unbounded io.ReadAll in CompleteOpenAI; consider io.LimitReader or streaming decode to limit memory use.
|
||||
return "", fmt.Errorf("read response: %w", err)
|
||||
|
[MINOR] Same body-echoing concern on AI Core chat/completions error path; consider truncation or lower log level to avoid leaking large or sensitive content into logs. **[MINOR]** Same body-echoing concern on AI Core chat/completions error path; consider truncation or lower log level to avoid leaking large or sensitive content into logs.
|
||||
}
|
||||
|
||||
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
||||
return "", fmt.Errorf("AI Core API error (status %d): %s", resp.StatusCode, string(body))
|
||||
}
|
||||
|
||||
var openaiResp ChatResponse
|
||||
|
gpt-review-bot
commented
[NIT] IsAnthropicModel uses a broad heuristic (contains "claude"). This may misclassify edge cases. Consider a stricter prefix check (e.g., anthropic--*) or a configurable mapping if model naming varies. **[NIT]** IsAnthropicModel uses a broad heuristic (contains "claude"). This may misclassify edge cases. Consider a stricter prefix check (e.g., anthropic--*) or a configurable mapping if model naming varies.
|
||||
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
|
||||
|
gpt-review-bot
commented
[NIT] Test name 'TestClient_WithTimeout_PropagatestoAICore' has inconsistent capitalization ('Propagatesto'); consider renaming to 'TestClient_WithTimeout_PropagatesToAICore' for readability consistency. **[NIT]** Test name 'TestClient_WithTimeout_PropagatestoAICore' has inconsistent capitalization ('Propagatesto'); consider renaming to 'TestClient_WithTimeout_PropagatesToAICore' for readability consistency.
gpt-review-bot
commented
[NIT] Test name 'TestClient_WithTimeout_PropagatestoAICore' has a minor typo/casing; consider renaming to 'TestClient_WithTimeout_PropagatesToAICore' for readability and consistency. **[NIT]** Test name 'TestClient_WithTimeout_PropagatestoAICore' has a minor typo/casing; consider renaming to 'TestClient_WithTimeout_PropagatesToAICore' for readability and consistency.
gpt-review-bot
commented
[NIT] Test name 'TestClient_WithTimeout_PropagatestoAICore' has a typo ('Propagatesto'). Consider renaming to 'PropagatesToAICore' for clarity and consistency. **[NIT]** Test name 'TestClient_WithTimeout_PropagatestoAICore' has a typo ('Propagatesto'). Consider renaming to 'PropagatesToAICore' for clarity and consistency.
|
||||
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"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,
|
||||
})
|
||||
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",
|
||||
})
|
||||
|
||||
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)
|
||||
}
|
||||
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 {
|
||||
t.Error("expected error for unknown model")
|
||||
}
|
||||
}
|
||||
|
||||
func TestAICoreClient_CompleteAnthropic(t *testing.T) {
|
||||
// Use a pointer to capture the server URL for use in the handler
|
||||
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",
|
||||
|
sonnet-review-bot
commented
[NIT] The test comment **[NIT]** The test comment `// Use a pointer to capture the server URL for use in the handler` is slightly misleading — `baseURL` is a plain `string` variable captured by closure, not a pointer.
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
},
|
||||
})
|
||||
})
|
||||
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{}{
|
||||
{
|
||||
"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 {
|
||||
|
sonnet-review-bot
commented
[NIT] TestAICoreClient_TokenExpiry generates token strings with **[NIT]** TestAICoreClient_TokenExpiry generates token strings with `string(rune('0'+call))` which works for single-digit call counts (1, 2) but would produce incorrect results for call >= 10 (non-digit runes). Since the test only makes 2 calls this is safe, but `fmt.Sprintf("token-%d", call)` would be clearer and more robust.
|
||||
t.Fatalf("second getToken: %v", err)
|
||||
}
|
||||
|
sonnet-review-bot
commented
[NIT] TestAICoreClient_TokenExpiry ignores the error return from **[NIT]** TestAICoreClient_TokenExpiry ignores the error return from `getToken` on lines 390 and 396 (`token1, _ :=` and `token2, _ :=`). Tests should check errors or use t.Fatal to avoid masking failures.
|
||||
|
||||
if token1 == token2 {
|
||||
t.Error("expected different tokens after expiry")
|
||||
}
|
||||
if atomic.LoadInt32(&tokenCalls) != 2 {
|
||||
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 {
|
||||
t.Errorf("expected default timeout 5m, got %v", client.http.Timeout)
|
||||
}
|
||||
|
gpt-review-bot
commented
[NIT] Token string generation uses rune arithmetic ("token-" + string(rune('0'+call))). This is fragile beyond single-digit calls. Prefer fmt.Sprintf("token-%d", call) for clarity and robustness. **[NIT]** Token string generation uses rune arithmetic ("token-" + string(rune('0'+call))). This is fragile beyond single-digit calls. Prefer fmt.Sprintf("token-%d", call) for clarity and robustness.
|
||||
|
||||
// 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{
|
||||
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 ---
|
||||
|
||||
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"`
|
||||
Model string `json:"model,omitempty"`
|
||||
MaxTokens int `json:"max_tokens"`
|
||||
System string `json:"system,omitempty"`
|
||||
Messages []anthropicMsg `json:"messages"`
|
||||
Temperature float64 `json:"temperature,omitempty"`
|
||||
}
|
||||
|
||||
type anthropicMsg struct {
|
||||
|
||||
[NIT] Minor trailing whitespace in the default value for aicore-resource-group ('default '). This is harmless but can be cleaned up for consistency.