From a24dc19d49263a0fe9b1a8d82e354ef051e7217c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Banaszewski?= Date: Wed, 29 Apr 2026 18:28:37 +0200 Subject: [PATCH] chore: clean up env var usage in aibridge (#24783) > AI tools where used when creating this PR This PR removes environment variable parsing from `/aibridge` directory. Added env variables/flags for dump dir as coder options. Only added to new indexed provider options (`CODER_AIBRIDGE_PROVIDER__*`) not to deprecated legacy env variables (`CODER_AIBRIDGE_ANTHROPIC_*` and `CODER_AIBRIDGE_OPENAI_KEY_*`). Reverted adding `MaxRetries` option as it will be removed soon due to key failover work: https://github.com/coder/coder/pull/24783#discussion_r3155544808 --- aibridge/config/config.go | 12 ----------- aibridge/intercept/chatcompletions/base.go | 3 --- aibridge/intercept/messages/base.go | 3 --- aibridge/intercept/responses/base.go | 3 --- .../integrationtest/responses_test.go | 6 ------ aibridge/provider/anthropic.go | 15 -------------- aibridge/provider/copilot.go | 13 ------------ aibridge/provider/openai.go | 15 -------------- cli/server.go | 2 ++ cli/server_aibridge_internal_test.go | 2 ++ coderd/apidoc/docs.go | 4 ++++ coderd/apidoc/swagger.json | 4 ++++ codersdk/deployment.go | 2 ++ docs/ai-coder/ai-gateway/setup.md | 20 +++++++++++++------ docs/reference/api/general.md | 1 + docs/reference/api/schemas.md | 6 ++++++ enterprise/cli/aibridged.go | 3 +++ enterprise/cli/aibridged_internal_test.go | 20 ++++++++++++++++--- site/src/api/typesGenerated.ts | 4 ++++ 19 files changed, 59 insertions(+), 79 deletions(-) diff --git a/aibridge/config/config.go b/aibridge/config/config.go index 1af2407b8a..48f29bb3f5 100644 --- a/aibridge/config/config.go +++ b/aibridge/config/config.go @@ -21,10 +21,6 @@ type Anthropic struct { // with a access token. When set, the access token is used for upstream // LLM requests instead of the API key. BYOKBearerToken string - // MaxRetries controls the number of automatic retries the SDK will perform - // on transient errors. If nil, the SDK default (2) is used. - // Set to 0 to disable retries entirely. - MaxRetries *int } type AWSBedrock struct { @@ -46,10 +42,6 @@ type OpenAI struct { CircuitBreaker *CircuitBreaker SendActorHeaders bool ExtraHeaders map[string]string - // MaxRetries controls the number of automatic retries the SDK will perform - // on transient errors. If nil, the SDK default (2) is used. - // Set to 0 to disable retries entirely. - MaxRetries *int } type Copilot struct { @@ -58,10 +50,6 @@ type Copilot struct { BaseURL string APIDumpDir string CircuitBreaker *CircuitBreaker - // MaxRetries controls the number of automatic retries the SDK will perform - // on transient errors. If nil, the SDK default (2) is used. - // Set to 0 to disable retries entirely. - MaxRetries *int } // CircuitBreaker holds configuration for circuit breakers. diff --git a/aibridge/intercept/chatcompletions/base.go b/aibridge/intercept/chatcompletions/base.go index 6f6ca35261..aa84e7dead 100644 --- a/aibridge/intercept/chatcompletions/base.go +++ b/aibridge/intercept/chatcompletions/base.go @@ -46,9 +46,6 @@ type interceptionBase struct { func (i *interceptionBase) newCompletionsService() openai.ChatCompletionService { opts := []option.RequestOption{option.WithAPIKey(i.cfg.Key), option.WithBaseURL(i.cfg.BaseURL)} - if i.cfg.MaxRetries != nil { - opts = append(opts, option.WithMaxRetries(*i.cfg.MaxRetries)) - } // Add extra headers if configured. // Some providers require additional headers that are not added by the SDK. diff --git a/aibridge/intercept/messages/base.go b/aibridge/intercept/messages/base.go index 9853f7da5e..c5d053768e 100644 --- a/aibridge/intercept/messages/base.go +++ b/aibridge/intercept/messages/base.go @@ -217,9 +217,6 @@ func (i *interceptionBase) newMessagesService(ctx context.Context, opts ...optio opts = append(opts, option.WithAPIKey(i.cfg.Key)) } opts = append(opts, option.WithBaseURL(i.cfg.BaseURL)) - if i.cfg.MaxRetries != nil { - opts = append(opts, option.WithMaxRetries(*i.cfg.MaxRetries)) - } // Add extra headers if configured. // Some providers require additional headers that are not added by the SDK. diff --git a/aibridge/intercept/responses/base.go b/aibridge/intercept/responses/base.go index 0107d2328d..9affc7d3ea 100644 --- a/aibridge/intercept/responses/base.go +++ b/aibridge/intercept/responses/base.go @@ -55,9 +55,6 @@ type responsesInterceptionBase struct { func (i *responsesInterceptionBase) newResponsesService() responses.ResponseService { opts := []option.RequestOption{option.WithBaseURL(i.cfg.BaseURL), option.WithAPIKey(i.cfg.Key)} - if i.cfg.MaxRetries != nil { - opts = append(opts, option.WithMaxRetries(*i.cfg.MaxRetries)) - } // Add extra headers if configured. // Some providers require additional headers that are not added by the SDK. diff --git a/aibridge/internal/integrationtest/responses_test.go b/aibridge/internal/integrationtest/responses_test.go index 5d675c6db2..82f0774b3a 100644 --- a/aibridge/internal/integrationtest/responses_test.go +++ b/aibridge/internal/integrationtest/responses_test.go @@ -639,10 +639,7 @@ func TestClientAndConnectionError(t *testing.T) { t.Cleanup(cancel) // tc.addr may be an intentionally invalid URL; use withCustomProvider. - // MaxRetries is set to 0 to disable SDK retries and speed up the test. cfg := openAICfg(tc.addr, apiKey) - maxRetries := 0 - cfg.MaxRetries = &maxRetries bridgeServer := newBridgeTestServer(ctx, t, tc.addr, withCustomProvider(provider.NewOpenAI(cfg))) reqBytes := responsesRequestBytes(t, tc.streaming) @@ -719,10 +716,7 @@ func TestUpstreamError(t *testing.T) { })) t.Cleanup(upstream.Close) - // MaxRetries is set to 0 to disable SDK retries and speed up the test. cfg := openAICfg(upstream.URL, apiKey) - maxRetries := 0 - cfg.MaxRetries = &maxRetries bridgeServer := newBridgeTestServer(ctx, t, upstream.URL, withCustomProvider(provider.NewOpenAI(cfg))) reqBytes := responsesRequestBytes(t, tc.streaming) diff --git a/aibridge/provider/anthropic.go b/aibridge/provider/anthropic.go index 88d94a78ca..269c669a16 100644 --- a/aibridge/provider/anthropic.go +++ b/aibridge/provider/anthropic.go @@ -4,8 +4,6 @@ import ( "fmt" "io" "net/http" - "os" - "strconv" "strings" "github.com/google/uuid" @@ -57,19 +55,6 @@ func NewAnthropic(cfg config.Anthropic, bedrockCfg *config.AWSBedrock) *Anthropi if cfg.BaseURL == "" { cfg.BaseURL = "https://api.anthropic.com/" } - if cfg.Key == "" { - cfg.Key = os.Getenv("ANTHROPIC_API_KEY") - } - if cfg.APIDumpDir == "" { - cfg.APIDumpDir = os.Getenv("BRIDGE_DUMP_DIR") - } - if cfg.MaxRetries == nil { - if v := os.Getenv("ANTHROPIC_MAX_RETRIES"); v != "" { - if n, err := strconv.Atoi(v); err == nil { - cfg.MaxRetries = &n - } - } - } if cfg.CircuitBreaker != nil { cfg.CircuitBreaker.IsFailure = anthropicIsFailure cfg.CircuitBreaker.OpenErrorResponse = anthropicOpenErrorResponse diff --git a/aibridge/provider/copilot.go b/aibridge/provider/copilot.go index 153514e979..9557b05d44 100644 --- a/aibridge/provider/copilot.go +++ b/aibridge/provider/copilot.go @@ -5,8 +5,6 @@ import ( "fmt" "io" "net/http" - "os" - "strconv" "strings" "github.com/google/uuid" @@ -61,16 +59,6 @@ func NewCopilot(cfg config.Copilot) *Copilot { if cfg.BaseURL == "" { cfg.BaseURL = copilotBaseURL } - if cfg.APIDumpDir == "" { - cfg.APIDumpDir = os.Getenv("BRIDGE_DUMP_DIR") - } - if cfg.MaxRetries == nil { - if v := os.Getenv("COPILOT_MAX_RETRIES"); v != "" { - if n, err := strconv.Atoi(v); err == nil { - cfg.MaxRetries = &n - } - } - } if cfg.CircuitBreaker != nil { cfg.CircuitBreaker.OpenErrorResponse = copilotOpenErrorResponse } @@ -153,7 +141,6 @@ func (p *Copilot) CreateInterceptor(_ http.ResponseWriter, r *http.Request, trac APIDumpDir: p.cfg.APIDumpDir, CircuitBreaker: p.cfg.CircuitBreaker, ExtraHeaders: extractCopilotHeaders(r), - MaxRetries: p.cfg.MaxRetries, } cred := intercept.NewCredentialInfo(intercept.CredentialKindBYOK, key) diff --git a/aibridge/provider/openai.go b/aibridge/provider/openai.go index 2e37da5a1b..80d3eb5eef 100644 --- a/aibridge/provider/openai.go +++ b/aibridge/provider/openai.go @@ -5,8 +5,6 @@ import ( "fmt" "io" "net/http" - "os" - "strconv" "strings" "github.com/google/uuid" @@ -46,19 +44,6 @@ func NewOpenAI(cfg config.OpenAI) *OpenAI { if cfg.BaseURL == "" { cfg.BaseURL = "https://api.openai.com/v1/" } - if cfg.Key == "" { - cfg.Key = os.Getenv("OPENAI_API_KEY") - } - if cfg.APIDumpDir == "" { - cfg.APIDumpDir = os.Getenv("BRIDGE_DUMP_DIR") - } - if cfg.MaxRetries == nil { - if v := os.Getenv("OPENAI_MAX_RETRIES"); v != "" { - if n, err := strconv.Atoi(v); err == nil { - cfg.MaxRetries = &n - } - } - } if cfg.CircuitBreaker != nil { cfg.CircuitBreaker.OpenErrorResponse = openAIOpenErrorResponse } diff --git a/cli/server.go b/cli/server.go index 3b3d8b5a4a..35eec130b4 100644 --- a/cli/server.go +++ b/cli/server.go @@ -2981,6 +2981,8 @@ func ReadAIBridgeProvidersFromEnv(logger slog.Logger, environ []string) ([]coder provider.Key = v.Value case "BASE_URL": provider.BaseURL = v.Value + case "DUMP_DIR": + provider.DumpDir = v.Value case "BEDROCK_BASE_URL": provider.BedrockBaseURL = v.Value case "BEDROCK_REGION": diff --git a/cli/server_aibridge_internal_test.go b/cli/server_aibridge_internal_test.go index d0833070ef..49789f1a83 100644 --- a/cli/server_aibridge_internal_test.go +++ b/cli/server_aibridge_internal_test.go @@ -34,6 +34,7 @@ func TestReadAIBridgeProvidersFromEnv(t *testing.T) { "CODER_AIBRIDGE_PROVIDER_0_NAME=anthropic-zdr", "CODER_AIBRIDGE_PROVIDER_0_KEY=sk-ant-xxx", "CODER_AIBRIDGE_PROVIDER_0_BASE_URL=https://api.anthropic.com/", + "CODER_AIBRIDGE_PROVIDER_0_DUMP_DIR=/tmp/aibridge-dump", }, expected: []codersdk.AIBridgeProviderConfig{ { @@ -41,6 +42,7 @@ func TestReadAIBridgeProvidersFromEnv(t *testing.T) { Name: "anthropic-zdr", Key: "sk-ant-xxx", BaseURL: "https://api.anthropic.com/", + DumpDir: "/tmp/aibridge-dump", }, }, }, diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go index 85f4586105..f486b71104 100644 --- a/coderd/apidoc/docs.go +++ b/coderd/apidoc/docs.go @@ -13312,6 +13312,10 @@ const docTemplate = `{ "bedrock_small_fast_model": { "type": "string" }, + "dump_dir": { + "description": "DumpDir is the directory path for dumping API requests and responses.", + "type": "string" + }, "name": { "description": "Name is the unique instance identifier used for routing.\nDefaults to Type if not provided.", "type": "string" diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json index 323440a5e0..5e72e5638d 100644 --- a/coderd/apidoc/swagger.json +++ b/coderd/apidoc/swagger.json @@ -11860,6 +11860,10 @@ "bedrock_small_fast_model": { "type": "string" }, + "dump_dir": { + "description": "DumpDir is the directory path for dumping API requests and responses.", + "type": "string" + }, "name": { "description": "Name is the unique instance identifier used for routing.\nDefaults to Type if not provided.", "type": "string" diff --git a/codersdk/deployment.go b/codersdk/deployment.go index 69cfc0d84b..8dca6ab102 100644 --- a/codersdk/deployment.go +++ b/codersdk/deployment.go @@ -4114,6 +4114,8 @@ type AIBridgeProviderConfig struct { Key string `json:"-"` // BaseURL is the base URL of the upstream provider API. BaseURL string `json:"base_url"` + // DumpDir is the directory path for dumping API requests and responses. + DumpDir string `json:"dump_dir,omitempty"` // Bedrock fields (only applicable when Type == "anthropic"). BedrockBaseURL string `json:"-"` diff --git a/docs/ai-coder/ai-gateway/setup.md b/docs/ai-coder/ai-gateway/setup.md index 0df93376d6..de7b301c3c 100644 --- a/docs/ai-coder/ai-gateway/setup.md +++ b/docs/ai-coder/ai-gateway/setup.md @@ -213,12 +213,20 @@ requests to `/api/v2/aibridge//` to target a specific instance: **Supported keys per provider:** -| Key | Required | Description | -|------------|----------|------------------------------------------------------| -| `TYPE` | Yes | Provider type: `openai`, `anthropic`, or `copilot` | -| `NAME` | No | Unique instance name for routing. Defaults to `TYPE` | -| `KEY` | No | API key for upstream authentication (alias: `KEYS`) | -| `BASE_URL` | No | Base URL of the upstream API | +| Key | Required | Description | +|------------|----------|-------------------------------------------------------| +| `TYPE` | Yes | Provider type: `openai`, `anthropic`, or `copilot` | +| `NAME` | No | Unique instance name for routing. Defaults to `TYPE` | +| `KEY` | No | API key for upstream authentication (alias: `KEYS`) | +| `BASE_URL` | No | Base URL of the upstream API | +| `DUMP_DIR` | No | Directory for provider API request and response dumps | + +> [!WARNING] +> `DUMP_DIR` is not intended for regular use. Setting this option +> results in a high number of writes. Dump files contain raw request and +> response data, which may include proprietary or sensitive information +> (prompts, completions, tool inputs). Enable only briefly for diagnostic +> purposes and protect the target directory. For `anthropic` providers using AWS Bedrock, the following keys are also available: `BEDROCK_BASE_URL`, `BEDROCK_REGION`, diff --git a/docs/reference/api/general.md b/docs/reference/api/general.md index 48ae6f8cf0..857f900c8f 100644 --- a/docs/reference/api/general.md +++ b/docs/reference/api/general.md @@ -210,6 +210,7 @@ curl -X GET http://coder-server:8080/api/v2/deployment/config \ "bedrock_model": "string", "bedrock_region": "string", "bedrock_small_fast_model": "string", + "dump_dir": "string", "name": "string", "type": "string" } diff --git a/docs/reference/api/schemas.md b/docs/reference/api/schemas.md index 3182eb676f..e6b8851dc9 100644 --- a/docs/reference/api/schemas.md +++ b/docs/reference/api/schemas.md @@ -468,6 +468,7 @@ "bedrock_model": "string", "bedrock_region": "string", "bedrock_small_fast_model": "string", + "dump_dir": "string", "name": "string", "type": "string" } @@ -760,6 +761,7 @@ "bedrock_model": "string", "bedrock_region": "string", "bedrock_small_fast_model": "string", + "dump_dir": "string", "name": "string", "type": "string" } @@ -773,6 +775,7 @@ | `bedrock_model` | string | false | | | | `bedrock_region` | string | false | | | | `bedrock_small_fast_model` | string | false | | | +| `dump_dir` | string | false | | Dump dir is the directory path for dumping API requests and responses. | | `name` | string | false | | Name is the unique instance identifier used for routing. Defaults to Type if not provided. | | `type` | string | false | | Type is the provider type: "openai", "anthropic", or "copilot". | @@ -1287,6 +1290,7 @@ "bedrock_model": "string", "bedrock_region": "string", "bedrock_small_fast_model": "string", + "dump_dir": "string", "name": "string", "type": "string" } @@ -3326,6 +3330,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "bedrock_model": "string", "bedrock_region": "string", "bedrock_small_fast_model": "string", + "dump_dir": "string", "name": "string", "type": "string" } @@ -3916,6 +3921,7 @@ CreateWorkspaceRequest provides options for creating a new workspace. Only one o "bedrock_model": "string", "bedrock_region": "string", "bedrock_small_fast_model": "string", + "dump_dir": "string", "name": "string", "type": "string" } diff --git a/enterprise/cli/aibridged.go b/enterprise/cli/aibridged.go index 4c2d1d0687..a51e0e01a7 100644 --- a/enterprise/cli/aibridged.go +++ b/enterprise/cli/aibridged.go @@ -116,6 +116,7 @@ func buildProviders(cfg codersdk.AIBridgeConfig) ([]aibridge.Provider, error) { Name: name, BaseURL: p.BaseURL, Key: p.Key, + APIDumpDir: p.DumpDir, CircuitBreaker: cbConfig, SendActorHeaders: cfg.SendActorHeaders.Value(), })) @@ -124,6 +125,7 @@ func buildProviders(cfg codersdk.AIBridgeConfig) ([]aibridge.Provider, error) { Name: name, BaseURL: p.BaseURL, Key: p.Key, + APIDumpDir: p.DumpDir, CircuitBreaker: cbConfig, SendActorHeaders: cfg.SendActorHeaders.Value(), }, bedrockConfigFromProvider(p))) @@ -131,6 +133,7 @@ func buildProviders(cfg codersdk.AIBridgeConfig) ([]aibridge.Provider, error) { providers = append(providers, aibridge.NewCopilotProvider(aibridge.CopilotConfig{ Name: name, BaseURL: p.BaseURL, + APIDumpDir: p.DumpDir, CircuitBreaker: cbConfig, })) default: diff --git a/enterprise/cli/aibridged_internal_test.go b/enterprise/cli/aibridged_internal_test.go index 4f446b6616..e923c106af 100644 --- a/enterprise/cli/aibridged_internal_test.go +++ b/enterprise/cli/aibridged_internal_test.go @@ -43,8 +43,19 @@ func TestBuildProviders(t *testing.T) { t.Parallel() cfg := codersdk.AIBridgeConfig{ Providers: []codersdk.AIBridgeProviderConfig{ - {Type: aibridge.ProviderAnthropic, Name: "anthropic-zdr", Key: "sk-zdr"}, - {Type: aibridge.ProviderOpenAI, Name: "openai-azure", Key: "sk-azure", BaseURL: "https://azure.openai.com"}, + { + Type: aibridge.ProviderAnthropic, + Name: "anthropic-zdr", + Key: "sk-zdr", + DumpDir: "/tmp/anthropic-dump", + }, + { + Type: aibridge.ProviderOpenAI, + Name: "openai-azure", + Key: "sk-azure", + BaseURL: "https://azure.openai.com", + DumpDir: "/tmp/openai-dump", + }, }, } @@ -53,6 +64,8 @@ func TestBuildProviders(t *testing.T) { names := providerNames(providers) assert.Equal(t, []string{"anthropic-zdr", "openai-azure"}, names) + assert.Equal(t, "/tmp/anthropic-dump", providers[0].APIDumpDir()) + assert.Equal(t, "/tmp/openai-dump", providers[1].APIDumpDir()) }) t.Run("LegacyOpenAIConflictsWithIndexed", func(t *testing.T) { @@ -154,7 +167,7 @@ func TestBuildProviders(t *testing.T) { // Copilot API hosts via an explicit BASE_URL. cfg := codersdk.AIBridgeConfig{ Providers: []codersdk.AIBridgeProviderConfig{ - {Type: aibridge.ProviderCopilot, Name: aibridge.ProviderCopilot}, + {Type: aibridge.ProviderCopilot, Name: aibridge.ProviderCopilot, DumpDir: "/tmp/copilot-dump"}, {Type: aibridge.ProviderCopilot, Name: agplaibridge.ProviderCopilotBusiness, BaseURL: "https://" + agplaibridge.HostCopilotBusiness}, {Type: aibridge.ProviderCopilot, Name: agplaibridge.ProviderCopilotEnterprise, BaseURL: "https://" + agplaibridge.HostCopilotEnterprise}, }, @@ -165,6 +178,7 @@ func TestBuildProviders(t *testing.T) { require.Len(t, providers, 3) assert.Equal(t, aibridge.ProviderCopilot, providers[0].Name()) + assert.Equal(t, "/tmp/copilot-dump", providers[0].APIDumpDir()) assert.Equal(t, agplaibridge.ProviderCopilotBusiness, providers[1].Name()) assert.Equal(t, "https://"+agplaibridge.HostCopilotBusiness, providers[1].BaseURL()) assert.Equal(t, agplaibridge.ProviderCopilotEnterprise, providers[2].Name()) diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 5bee48151b..4e59633a6e 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -144,6 +144,10 @@ export interface AIBridgeProviderConfig { * BaseURL is the base URL of the upstream provider API. */ readonly base_url: string; + /** + * DumpDir is the directory path for dumping API requests and responses. + */ + readonly dump_dir?: string; readonly bedrock_region?: string; readonly bedrock_model?: string; readonly bedrock_small_fast_model?: string;