diff --git a/cmd/opencodereview/apply_provider_field_test.go b/cmd/opencodereview/apply_provider_field_test.go index 1b9a6e34d..d266e88c3 100644 --- a/cmd/opencodereview/apply_provider_field_test.go +++ b/cmd/opencodereview/apply_provider_field_test.go @@ -23,7 +23,7 @@ func TestApplyProviderField(t *testing.T) { {"extra_body", `{"k":1}`, func(e ProviderEntry) bool { return e.ExtraBody["k"] != nil }}, } for _, c := range cases { - if err := applyProviderField(&e, c.field, "providers.p."+c.field, c.value); err != nil { + if err := applyProviderField("p", &e, c.field, "providers.p."+c.field, c.value); err != nil { t.Fatalf("field %q: %v", c.field, err) } if !c.check(e) { @@ -34,20 +34,20 @@ func TestApplyProviderField(t *testing.T) { t.Run("protocol validated and normalized", func(t *testing.T) { var e ProviderEntry - if err := applyProviderField(&e, "protocol", "providers.p.protocol", "openai"); err != nil { + if err := applyProviderField("p", &e, "protocol", "providers.p.protocol", "openai"); err != nil { t.Fatalf("valid protocol: %v", err) } if e.Protocol == "" { t.Error("protocol not set") } - if err := applyProviderField(&e, "protocol", "providers.p.protocol", "not-a-protocol"); err == nil { + if err := applyProviderField("p", &e, "protocol", "providers.p.protocol", "not-a-protocol"); err == nil { t.Error("expected error for invalid protocol") } }) t.Run("auth_header normalized", func(t *testing.T) { var e ProviderEntry - if err := applyProviderField(&e, "auth_header", "providers.p.auth_header", "x-api-key"); err != nil { + if err := applyProviderField("p", &e, "auth_header", "providers.p.auth_header", "x-api-key"); err != nil { t.Fatalf("valid auth header: %v", err) } if e.AuthHeader == "" { @@ -57,21 +57,21 @@ func TestApplyProviderField(t *testing.T) { t.Run("auth_header rejects unsupported value", func(t *testing.T) { var e ProviderEntry - if err := applyProviderField(&e, "auth_header", "providers.p.auth_header", "cookie"); err == nil { + if err := applyProviderField("p", &e, "auth_header", "providers.p.auth_header", "cookie"); err == nil { t.Error("expected error for unsupported auth header") } }) t.Run("extra_body rejects invalid JSON", func(t *testing.T) { var e ProviderEntry - if err := applyProviderField(&e, "extra_body", "providers.p.extra_body", "{bad"); err == nil { + if err := applyProviderField("p", &e, "extra_body", "providers.p.extra_body", "{bad"); err == nil { t.Error("expected JSON error") } }) t.Run("extra_headers parsed", func(t *testing.T) { var e ProviderEntry - if err := applyProviderField(&e, "extra_headers", "providers.p.extra_headers", "X-A=1"); err != nil { + if err := applyProviderField("p", &e, "extra_headers", "providers.p.extra_headers", "X-A=1"); err != nil { t.Fatalf("valid extra headers: %v", err) } if len(e.ExtraHeaders) == 0 { @@ -81,7 +81,7 @@ func TestApplyProviderField(t *testing.T) { t.Run("unknown field returns error", func(t *testing.T) { var e ProviderEntry - if err := applyProviderField(&e, "bogus", "providers.p.bogus", "x"); err == nil { + if err := applyProviderField("p", &e, "bogus", "providers.p.bogus", "x"); err == nil { t.Error("expected error for unknown field") } }) diff --git a/cmd/opencodereview/bedrock_config_test.go b/cmd/opencodereview/bedrock_config_test.go new file mode 100644 index 000000000..8d2bb7d08 --- /dev/null +++ b/cmd/opencodereview/bedrock_config_test.go @@ -0,0 +1,342 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "encoding/json" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/alibaba/open-code-review/internal/llm" +) + +// TestConfigRoundTripKeepsAWSSettings is the regression test for a silent loss: +// config is unmarshalled into Config and marshalled back on every write, so +// before aws_profile / aws_region existed on ProviderEntry, the first run of any +// config command deleted them from a hand-written file — with no error, and no +// way for the user to tell why Bedrock suddenly used the wrong region. +func TestConfigRoundTripKeepsAWSSettings(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + original := `{ + "provider": "bedrock", + "model": "us.anthropic.claude-sonnet-4-6", + "providers": { + "bedrock": { "aws_region": "us-west-2", "aws_profile": "example-profile" } + } +}` + if err := os.WriteFile(path, []byte(original), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + + cfg, err := loadOrCreateConfig(path) + if err != nil { + t.Fatalf("loadOrCreateConfig: %v", err) + } + if err := saveConfig(path, cfg); err != nil { + t.Fatalf("saveConfig: %v", err) + } + + reloaded, err := loadOrCreateConfig(path) + if err != nil { + t.Fatalf("reload: %v", err) + } + entry := reloaded.Providers["bedrock"] + if entry.AWSRegion != "us-west-2" { + t.Errorf("AWSRegion = %q after round trip, want us-west-2", entry.AWSRegion) + } + if entry.AWSProfile != "example-profile" { + t.Errorf("AWSProfile = %q after round trip, want example-profile", entry.AWSProfile) + } + + // The resolver reads the same file independently; assert the written JSON + // still carries the keys it looks for, not just that our struct held them. + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read back: %v", err) + } + var raw map[string]any + if err := json.Unmarshal(data, &raw); err != nil { + t.Fatalf("unmarshal written config: %v", err) + } + providers, _ := raw["providers"].(map[string]any) + bedrockEntry, _ := providers["bedrock"].(map[string]any) + if bedrockEntry["aws_region"] != "us-west-2" || bedrockEntry["aws_profile"] != "example-profile" { + t.Errorf("written JSON = %v, want aws_region and aws_profile preserved", bedrockEntry) + } +} + +func TestSetProviderValueAWSSettings(t *testing.T) { + tests := []struct { + name string + key string + value string + wantErr string + check func(*testing.T, *Config) + }{ + { + name: "region on an ambient provider", + key: "providers.bedrock.aws_region", + value: "us-west-2", + check: func(t *testing.T, cfg *Config) { + if got := cfg.Providers["bedrock"].AWSRegion; got != "us-west-2" { + t.Errorf("AWSRegion = %q, want us-west-2", got) + } + }, + }, + { + name: "profile is trimmed", + key: "providers.bedrock.aws_profile", + value: " example-profile ", + check: func(t *testing.T, cfg *Config) { + if got := cfg.Providers["bedrock"].AWSProfile; got != "example-profile" { + t.Errorf("AWSProfile = %q, want example-profile", got) + } + }, + }, + { + name: "empty value hands the decision back to the AWS chain", + key: "providers.bedrock.aws_profile", + value: "", + check: func(t *testing.T, cfg *Config) { + if got := cfg.Providers["bedrock"].AWSProfile; got != "" { + t.Errorf("AWSProfile = %q, want empty", got) + } + }, + }, + { + // Storing it would be dead config that reads as applied. + name: "rejected on a key-based provider", + key: "providers.anthropic.aws_region", + value: "us-west-2", + wantErr: "does not apply to provider", + }, + { + name: "whitespace inside the value is rejected", + key: "providers.bedrock.aws_region", + value: "us west 2", + wantErr: "contains whitespace", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + cfg := &Config{} + err := setProviderValue(cfg, tc.key, tc.value) + if tc.wantErr != "" { + if err == nil { + t.Fatalf("setProviderValue(%q, %q) = nil, want error containing %q", tc.key, tc.value, tc.wantErr) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("error = %q, want it to contain %q", err, tc.wantErr) + } + return + } + if err != nil { + t.Fatalf("setProviderValue(%q, %q): %v", tc.key, tc.value, err) + } + tc.check(t, cfg) + }) + } +} + +// TestSetCustomProviderAWSSettingsFollowProtocol covers the custom-provider +// path: aws_* is meaningful there only once the entry speaks the Bedrock +// protocol, so the order of the two set commands matters and the error has to +// say why. +func TestSetCustomProviderAWSSettingsFollowProtocol(t *testing.T) { + cfg := &Config{} + if err := setCustomProviderValue(cfg, "custom_providers.mine.aws_region", "us-west-2"); err == nil { + t.Fatal("aws_region accepted before a protocol was set; want an error") + } + + if err := setCustomProviderValue(cfg, "custom_providers.mine.protocol", llm.ProtocolAnthropicBedrock); err != nil { + t.Fatalf("set protocol: %v", err) + } + if err := setCustomProviderValue(cfg, "custom_providers.mine.aws_region", "us-west-2"); err != nil { + t.Fatalf("set aws_region after protocol: %v", err) + } + if got := cfg.CustomProviders["mine"].AWSRegion; got != "us-west-2" { + t.Errorf("AWSRegion = %q, want us-west-2", got) + } +} + +// TestAWSSettingsRejectedWhenEntryOverridesProtocol covers the same +// entry-level protocol override the resolver honours: a preset's protocol can be +// overridden per entry, so `protocol: openai` on the bedrock preset must stop +// accepting AWS settings that nothing would read. +func TestAWSSettingsRejectedWhenEntryOverridesProtocol(t *testing.T) { + cfg := &Config{} + if err := setProviderValue(cfg, "providers.bedrock.protocol", "openai"); err != nil { + t.Fatalf("set protocol: %v", err) + } + err := setProviderValue(cfg, "providers.bedrock.aws_region", "us-west-2") + if err == nil { + t.Fatal("aws_region accepted on a bedrock entry overridden to protocol openai; want an error") + } + if !strings.Contains(err.Error(), "does not apply to provider") { + t.Errorf("error = %q, want it to explain the field does not apply", err) + } + + // Overriding back to the bedrock protocol makes them meaningful again. + if err := setProviderValue(cfg, "providers.bedrock.protocol", llm.ProtocolAnthropicBedrock); err != nil { + t.Fatalf("set protocol back: %v", err) + } + if err := setProviderValue(cfg, "providers.bedrock.aws_region", "us-west-2"); err != nil { + t.Errorf("aws_region rejected for an explicit bedrock protocol: %v", err) + } +} + +// TestSetProtocolClearsStaleAWSSettings covers the reverse order from +// TestAWSSettingsRejectedWhenEntryOverridesProtocol: aws_region/aws_profile set +// first while the entry is still ambient, then the entry switched to a protocol +// that does not read them. Without this, the fields survive the switch as dead +// config that reads as applied but has no effect — exactly what +// providerAcceptsAWSSettings exists to prevent on the other ordering. +func TestSetProtocolClearsStaleAWSSettings(t *testing.T) { + cfg := &Config{} + if err := setProviderValue(cfg, "providers.bedrock.aws_region", "us-west-2"); err != nil { + t.Fatalf("set aws_region: %v", err) + } + if err := setProviderValue(cfg, "providers.bedrock.aws_profile", "example-profile"); err != nil { + t.Fatalf("set aws_profile: %v", err) + } + + stderr := captureStderr(t, func() { + if err := setProviderValue(cfg, "providers.bedrock.protocol", "openai"); err != nil { + t.Fatalf("set protocol: %v", err) + } + }) + + entry := cfg.Providers["bedrock"] + if entry.AWSRegion != "" || entry.AWSProfile != "" { + t.Errorf("AWSRegion/AWSProfile = %q/%q after switching to openai, want both cleared", entry.AWSRegion, entry.AWSProfile) + } + if !strings.Contains(stderr, "WARNING") || !strings.Contains(stderr, "aws_region") { + t.Errorf("stderr = %q, want a WARNING naming aws_region", stderr) + } + + // Switching back to bedrock does not resurrect the cleared values, and + // setting them again still works. + if err := setProviderValue(cfg, "providers.bedrock.protocol", llm.ProtocolAnthropicBedrock); err != nil { + t.Fatalf("set protocol back: %v", err) + } + if entry := cfg.Providers["bedrock"]; entry.AWSRegion != "" { + t.Errorf("AWSRegion = %q after switching back to bedrock, want it to stay cleared", entry.AWSRegion) + } + if err := setProviderValue(cfg, "providers.bedrock.aws_region", "eu-west-1"); err != nil { + t.Errorf("aws_region rejected after switching back to bedrock: %v", err) + } +} + +// TestSetProtocolLeavesAWSSettingsWhenNoneSet is the no-op guard: switching +// protocol on an entry with no aws_region/aws_profile must not print a WARNING +// about clearing a value that was never there. +func TestSetProtocolLeavesAWSSettingsWhenNoneSet(t *testing.T) { + cfg := &Config{} + stderr := captureStderr(t, func() { + if err := setProviderValue(cfg, "providers.bedrock.protocol", "openai"); err != nil { + t.Fatalf("set protocol: %v", err) + } + }) + if strings.Contains(stderr, "WARNING") { + t.Errorf("stderr = %q, want no WARNING when nothing was cleared", stderr) + } +} + +// TestSetLlmProtocolRejectsBedrock pins the other half of the contract enforced +// in the resolver: the llm block is one url plus one token, with nowhere to put +// a region or a profile, so the value is refused where it is typed rather than +// stored and ignored until the next review run. +func TestSetLlmProtocolRejectsBedrock(t *testing.T) { + cfg := &Config{} + err := setConfigValue(cfg, "llm.protocol", llm.ProtocolAnthropicBedrock) + if err == nil { + t.Fatal("llm.protocol accepted anthropic-bedrock; want an error") + } + for _, want := range []string{"aws_region", "provider bedrock"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } + if cfg.Llm.Protocol != "" { + t.Errorf("Llm.Protocol = %q, want it left unset after the rejection", cfg.Llm.Protocol) + } + + // The neighbouring values still work — the rejection is one protocol, not + // the whole key. + if err := setConfigValue(cfg, "llm.protocol", llm.ProtocolOpenAIResponses); err != nil { + t.Fatalf("set llm.protocol=openai-responses: %v", err) + } +} + +func TestCheckAPIKeyRequirement(t *testing.T) { + bedrock, ok := llm.LookupProvider("bedrock") + if !ok { + t.Fatal("bedrock preset not registered") + } + anthropic, ok := llm.LookupProvider("anthropic") + if !ok { + t.Fatal("anthropic preset not registered") + } + + if err := checkAPIKeyRequirement("bedrock", "", "", bedrock, true); err != nil { + t.Errorf("ambient provider with no api_key = %v, want nil", err) + } + + t.Setenv(anthropic.EnvVar, "") + if err := checkAPIKeyRequirement("anthropic", "", "", anthropic, true); err == nil { + t.Error("key-based provider with no api_key and no env var = nil, want an error") + } + if err := checkAPIKeyRequirement("anthropic", "", "op read op://vault/key", anthropic, true); err != nil { + t.Errorf("key-based provider with api_key_cmd = %v, want nil", err) + } +} + +// TestProviderTUIAmbientProviderSkipsAPIKeyStep pins the wizard flow: the model +// step is the last one for a provider with no key to collect. An API-key prompt +// that must be left blank reads as a step the user failed to complete. +func TestProviderTUIAmbientProviderSkipsAPIKeyStep(t *testing.T) { + m := newProviderTUI(&Config{}, "") + idx := -1 + for i, p := range m.providers { + if p.Name == "bedrock" { + idx = i + break + } + } + if idx < 0 { + t.Fatal("bedrock not offered in the official provider list") + } + m.officialIdx = idx + + result, _ := m.Update(enterKey()) + atModel := result.(providerTUIModel) + if atModel.step != stepModel { + t.Fatalf("after Enter on provider, step = %d, want %d (stepModel)", atModel.step, stepModel) + } + + result, cmd := atModel.Update(enterKey()) + done := result.(providerTUIModel) + if done.step == stepAPIKey { + t.Error("ambient provider advanced to stepAPIKey; want the model step to be final") + } + if !done.confirmed { + t.Error("confirmed = false; want the selection confirmed from the model step") + } + if cmd == nil { + t.Error("no command returned; want tea.Quit") + } + res := done.result() + if res.provider != "bedrock" { + t.Errorf("result provider = %q, want bedrock", res.provider) + } + if res.apiKey != "" { + t.Errorf("result apiKey = %q, want empty for an ambient provider", res.apiKey) + } + if got := res.resolvedModel(); got == "" { + t.Error("resolvedModel is empty; want the model selected on the model step") + } +} diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 58b0dd476..5700314be 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -303,6 +303,16 @@ type ProviderEntry struct { ExtraBody map[string]any `json:"extra_body,omitempty"` ExtraHeaders map[string]string `json:"extra_headers,omitempty"` RetryCodes []int `json:"retry_codes,omitempty"` + + // AWSProfile and AWSRegion pin the credentials and region for providers that + // authenticate from the AWS chain (bedrock). Both are optional — without + // them the standard chain decides, as with any other AWS tool. They must + // exist here as well as in the resolver's own view of the file: config is + // unmarshalled into this struct and marshalled back on every write, so a + // field missing from it is silently dropped from a hand-written config the + // first time any config command runs. + AWSProfile string `json:"aws_profile,omitempty"` + AWSRegion string `json:"aws_region,omitempty"` } // MCPServerConfig holds configuration for a single MCP server. @@ -495,6 +505,12 @@ func setConfigValue(cfg *Config, key, value string) error { if err := llm.ValidateProtocol(normalized); err != nil { return err } + // The llm block is a single url + token endpoint. Bedrock needs neither + // and has nowhere here to put a region or a profile, so it is refused at + // the point of setting rather than accepted and ignored at resolve time. + if normalized == llm.ProtocolAnthropicBedrock { + return fmt.Errorf("llm.protocol cannot be %q: bedrock derives its host from aws_region and signs with the AWS credential chain, so it has no use for llm.url or llm.auth_token; run `ocr config set provider bedrock` instead", normalized) + } cfg.Llm.Protocol = normalized // Mirror use_anthropic so older binaries that predate llm.protocol // still pick the right protocol family: anthropic -> true, the OpenAI @@ -559,12 +575,12 @@ func setConfigValue(cfg *Config, key, value string) error { } cfg.Llm.RetryCodes = codes default: - return fmt.Errorf("unknown config key: %s\nSupported keys: %s\nProvider fields: api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers, retry_codes\nProtocol values: anthropic, openai, openai-responses\nMCP server fields: type, command, args, env, url, headers, tools, setup", key, strings.Join(supportedConfigKeys, ", ")) + return fmt.Errorf("unknown config key: %s\nSupported keys: %s\nProvider fields: api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers, retry_codes, aws_region, aws_profile\nProtocol values: anthropic, anthropic-bedrock, openai, openai-responses\nMCP server fields: type, command, args, env, url, headers, tools, setup", key, strings.Join(supportedConfigKeys, ", ")) } return nil } -func applyProviderField(entry *ProviderEntry, field, key, value string) error { +func applyProviderField(providerName string, entry *ProviderEntry, field, key, value string) error { switch field { case "api_key": entry.APIKey = value @@ -584,6 +600,15 @@ func applyProviderField(entry *ProviderEntry, field, key, value string) error { return err } entry.Protocol = normalized + // Switching away from bedrock leaves aws_region/aws_profile as dead + // config that reads as applied but nothing reads it — clear both, the + // same way the TUI drops url/api_key/auth_header when switching onto + // bedrock (see cpAmbientProtocol in provider_tui.go). + if normalized != llm.ProtocolAnthropicBedrock && (entry.AWSRegion != "" || entry.AWSProfile != "") { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: clearing aws_region/aws_profile on %q: protocol %q does not use the AWS credential chain\n", providerName, normalized) + entry.AWSRegion = "" + entry.AWSProfile = "" + } case "model": entry.Model = value case "models": @@ -619,12 +644,56 @@ func applyProviderField(entry *ProviderEntry, field, key, value string) error { fmt.Fprintf(os.Stderr, "[ocr] WARNING: %s\n", w) } entry.RetryCodes = codes + case "aws_region", "aws_profile": + normalized, err := normalizeAWSSetting(field, key, value) + if err != nil { + return err + } + if !providerAcceptsAWSSettings(providerName, entry) { + return fmt.Errorf("%s does not apply to provider %q: aws_region and aws_profile are only used by providers that authenticate from the AWS credential chain (protocol %s)", field, providerName, llm.ProtocolAnthropicBedrock) + } + if field == "aws_region" { + entry.AWSRegion = normalized + } else { + entry.AWSProfile = normalized + } default: - return fmt.Errorf("unknown provider field %q: supported fields are api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers, retry_codes", field) + return fmt.Errorf("unknown provider field %q: supported fields are api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers, retry_codes, aws_region, aws_profile", field) } return nil } +// providerAcceptsAWSSettings reports whether aws_region / aws_profile mean +// anything for this provider. Storing them anywhere else would be dead config +// that reads as applied, so it is rejected instead. +// +// The entry's own protocol decides whenever it sets one: a preset's protocol can +// be overridden per entry (see tryProviderConfig), so `protocol: openai` on the +// bedrock preset would otherwise still accept AWS settings that nothing reads. +// Only when the entry is silent does the preset's own AmbientAuth flag answer. +func providerAcceptsAWSSettings(providerName string, entry *ProviderEntry) bool { + if entry.Protocol != "" { + return llm.NormalizeProtocol(entry.Protocol) == llm.ProtocolAnthropicBedrock + } + preset, isPreset := llm.LookupProvider(providerName) + return isPreset && preset.AmbientAuth +} + +// normalizeAWSSetting trims the value and rejects the shapes AWS itself will +// not accept. Region names are deliberately not checked against a fixed list: +// AWS adds regions faster than any embedded list stays correct, and a wrong one +// already surfaces at request time. +func normalizeAWSSetting(field, key, value string) (string, error) { + trimmed := strings.TrimSpace(value) + if trimmed == "" { + return "", nil // clearing the field hands the decision back to the AWS chain + } + if strings.ContainsAny(trimmed, " \t\n") { + return "", fmt.Errorf("invalid %s for %s: %q contains whitespace", field, key, value) + } + return trimmed, nil +} + func parseModelListValue(value string) ([]string, error) { value = strings.TrimSpace(value) if value == "" { @@ -702,7 +771,7 @@ func setProviderValue(cfg *Config, key, value string) error { cfg.Providers = make(map[string]ProviderEntry) } entry := cfg.Providers[parts[1]] - if err := applyProviderField(&entry, parts[2], key, value); err != nil { + if err := applyProviderField(parts[1], &entry, parts[2], key, value); err != nil { return err } cfg.Providers[parts[1]] = entry @@ -722,7 +791,7 @@ func setCustomProviderField(cfg *Config, name, field, key, value string) error { cfg.CustomProviders = make(map[string]ProviderEntry) } entry := cfg.CustomProviders[name] - if err := applyProviderField(&entry, field, key, value); err != nil { + if err := applyProviderField(name, &entry, field, key, value); err != nil { return err } cfg.CustomProviders[name] = entry diff --git a/cmd/opencodereview/config_cmd_test.go b/cmd/opencodereview/config_cmd_test.go index 54f9dc082..e7cf91007 100644 --- a/cmd/opencodereview/config_cmd_test.go +++ b/cmd/opencodereview/config_cmd_test.go @@ -1069,8 +1069,8 @@ func TestSetConfigValueUnknownKeyMessage(t *testing.T) { } want := "unknown config key: bogus.key\n" + "Supported keys: provider, model, max_tokens, providers.., custom_providers.., mcp_servers.., llm.url, llm.auth_token, llm.auth_token_cmd, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, llm.retry_codes, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging\n" + - "Provider fields: api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers, retry_codes\n" + - "Protocol values: anthropic, openai, openai-responses\n" + + "Provider fields: api_key, api_key_cmd, url, protocol, model, models, auth_header, extra_body, extra_headers, retry_codes, aws_region, aws_profile\n" + + "Protocol values: anthropic, anthropic-bedrock, openai, openai-responses\n" + "MCP server fields: type, command, args, env, url, headers, tools, setup" if err.Error() != want { t.Errorf("unknown-key message drifted:\n got: %q\nwant: %q", err.Error(), want) diff --git a/cmd/opencodereview/llm_cmd.go b/cmd/opencodereview/llm_cmd.go index 65adce6e6..c145ec9e5 100644 --- a/cmd/opencodereview/llm_cmd.go +++ b/cmd/opencodereview/llm_cmd.go @@ -108,7 +108,19 @@ func runLLMTest() error { model = resp.Model } fmt.Printf("Source: %s\n", ep.Source) - fmt.Printf("URL: %s\n", ep.URL) + if region, profile, ok := bedrockContext(llmClient); ok { + // Bedrock has no configured URL — the region decides the host — so + // report what was resolved instead. A request that reached the wrong + // region otherwise fails in a way that looks like a bad model ID. + fmt.Printf("Region: %s\n", region) + if profile != "" { + fmt.Printf("Profile: %s\n", profile) + } else { + fmt.Printf("Profile: (from the ambient AWS chain)\n") + } + } else { + fmt.Printf("URL: %s\n", ep.URL) + } fmt.Printf("Model: %s\n", model) content := resp.Content() @@ -120,6 +132,17 @@ func runLLMTest() error { return nil } +// bedrockContext reports the region and profile a Bedrock client resolved. +// ok is false for every other client, which keeps the test output unchanged for +// URL-based providers. +func bedrockContext(client llm.LLMClient) (region, profile string, ok bool) { + c, isAnthropic := client.(*llm.AnthropicClient) + if !isAnthropic { + return "", "", false + } + return c.BedrockContext() +} + func runLLMProviders() { providers := llm.ListProviders() fmt.Println("\nBuilt-in providers:") diff --git a/cmd/opencodereview/provider_cmd.go b/cmd/opencodereview/provider_cmd.go index 3fa20b516..e32338eea 100644 --- a/cmd/opencodereview/provider_cmd.go +++ b/cmd/opencodereview/provider_cmd.go @@ -229,6 +229,34 @@ func applyCustomProviderConfig(configPath string, cfg *Config, result providerTU return nil } +// checkAPIKeyRequirement decides whether a provider selection may be saved with +// no api_key. It mirrors the resolver's precedence (static api_key -> +// api_key_cmd -> env var), so an already-configured command satisfies the +// requirement and picking a model for such a provider does not fail and abandon +// the save. apiKeyCmd is trimmed because the resolver treats a whitespace-only +// command as unset, so without this a command of " " would satisfy the check +// here and then fail resolution with "no api_key or api_key_cmd configured". +// +// An ambient-auth provider has no credential to save at all: demanding one would +// make it impossible to configure, since the credentials live in the AWS chain +// rather than the config file. +func checkAPIKeyRequirement(providerName, apiKey, apiKeyCmd string, preset llm.Provider, isPreset bool) error { + if apiKey != "" || strings.TrimSpace(apiKeyCmd) != "" { + return nil + } + switch { + case isPreset && preset.AmbientAuth: + return nil + case isPreset && preset.EnvVar != "": + if os.Getenv(preset.EnvVar) == "" { + return fmt.Errorf("API key is required for provider %s (configure it, set providers.%s.api_key_cmd, or set $%s)", providerName, providerName, preset.EnvVar) + } + return nil + default: + return fmt.Errorf("API key is required for provider %s (configure it or set providers.%s.api_key_cmd)", providerName, providerName) + } +} + func applyOfficialProviderConfig(configPath string, cfg *Config, result providerTUIResult) error { if result.provider == "" { return fmt.Errorf("provider and model are required") @@ -240,20 +268,8 @@ func applyOfficialProviderConfig(configPath string, cfg *Config, result provider preset, isPreset := llm.LookupProvider(result.provider) - // Mirror the resolver's precedence (static api_key -> api_key_cmd -> env var): - // an already-configured api_key_cmd satisfies the requirement, so picking a - // model for such a provider must not fail and abandon the save. Trimmed - // because the resolver treats a whitespace-only command as unset, so without - // this a command of " " would satisfy the check here and then fail - // resolution with "no api_key or api_key_cmd configured". - if result.apiKey == "" && strings.TrimSpace(cfg.Providers[result.provider].APIKeyCmd) == "" { - if isPreset && preset.EnvVar != "" { - if os.Getenv(preset.EnvVar) == "" { - return fmt.Errorf("API key is required for provider %s (configure it, set providers.%s.api_key_cmd, or set $%s)", result.provider, result.provider, preset.EnvVar) - } - } else { - return fmt.Errorf("API key is required for provider %s (configure it or set providers.%s.api_key_cmd)", result.provider, result.provider) - } + if err := checkAPIKeyRequirement(result.provider, result.apiKey, cfg.Providers[result.provider].APIKeyCmd, preset, isPreset); err != nil { + return err } if cfg.Providers == nil { diff --git a/cmd/opencodereview/provider_tui.go b/cmd/opencodereview/provider_tui.go index 2acc0286a..fdd98013a 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -53,14 +53,25 @@ const ( manualStepAuthHeader ) -// cpProtocols lists the protocol options offered in the Custom and Manual -// provider forms. Using the canonical names from protocol.go means whatever the -// user picks flows through resolver normalization unchanged and is written to -// config verbatim. +// cpProtocols lists the protocol options offered in the Custom provider form. +// Using the canonical names from protocol.go means whatever the user picks +// flows through resolver normalization unchanged and is written to config +// verbatim. var cpProtocols = []string{ llm.ProtocolAnthropic, llm.ProtocolOpenAIChatCompletions, llm.ProtocolOpenAIResponses, + llm.ProtocolAnthropicBedrock, +} + +// manualProtocols lists the protocol options offered in the Manual form, which +// writes llm.url and llm.auth_token. Bedrock is deliberately absent: that block +// holds no region or profile, and bedrock uses neither the url nor the token it +// does hold, so the resolver rejects the combination outright. +var manualProtocols = []string{ + llm.ProtocolAnthropic, + llm.ProtocolOpenAIChatCompletions, + llm.ProtocolOpenAIResponses, } type customProviderListItem struct { @@ -181,9 +192,29 @@ type providerTUIModel struct { // its index in cpProtocols. Unknown / empty values default to the OpenAI Chat // Completions entry (index 1) to preserve legacy behavior where any non-anthropic // protocol was treated as OpenAI. +// cpAmbientProtocol reports whether the protocol selected in the Custom form +// authenticates from the environment rather than from a stored credential. Such +// a provider has no url, no api key and no auth header to collect, so the form +// ends at the protocol step instead of walking three fields that would be +// written as dead config. +func (m providerTUIModel) cpAmbientProtocol() bool { + return cpProtocols[m.cpProtocolIdx] == llm.ProtocolAnthropicBedrock +} + func cpProtocolIndex(protocol string) int { + return protocolIndexIn(cpProtocols, protocol) +} + +// manualProtocolIndex is cpProtocolIndex for the Manual form's shorter list. A +// config that names bedrock in llm.protocol is unusable there and lands on the +// default rather than an out-of-range index; the resolver reports why. +func manualProtocolIndex(protocol string) int { + return protocolIndexIn(manualProtocols, protocol) +} + +func protocolIndexIn(list []string, protocol string) int { normalized := llm.NormalizeProtocol(protocol) - for i, p := range cpProtocols { + for i, p := range list { if p == normalized { return i } @@ -365,7 +396,7 @@ func newProviderTUI(cfg *Config, configPath string) providerTUIModel { // protocols including openai-responses); fall back to use_anthropic for // configs written before llm.protocol existed. if cfg.Llm.Protocol != "" { - m.manualProtocolIdx = cpProtocolIndex(cfg.Llm.Protocol) + m.manualProtocolIdx = manualProtocolIndex(cfg.Llm.Protocol) } else if cfg.Llm.UseAnthropic == nil || *cfg.Llm.UseAnthropic { m.manualProtocolIdx = 0 // anthropic } else { @@ -966,6 +997,11 @@ func (m providerTUIModel) apiKeyStepCanConfirm() (ok bool, errMsg string) { } if m.activeTab == tabOfficial { p := m.currentProvider() + if p.AmbientAuth { + // Reachable when an existing config is edited: an empty key is the + // correct state for a provider that signs from the AWS chain. + return true, "" + } if officialProviderEnvKeySet(p) { return true, "" } @@ -1124,6 +1160,9 @@ func (m providerTUIModel) handleCustomFormEnter() (tea.Model, tea.Cmd) { m.cpStep = cpStepProtocol return m, nil case cpStepProtocol: + if m.cpAmbientProtocol() { + return m.finishCustomForm() + } m.cpStep = cpStepBaseURL return m, m.cpURLInput.Focus() case cpStepBaseURL: @@ -1148,31 +1187,38 @@ func (m providerTUIModel) handleCustomFormEnter() (tea.Model, tea.Cmd) { return m, nil } m.cpAuthInput.Blur() - if m.editingCustom { - r := m.result() - if err := m.applyEditCustomProviderSave(); err != nil { - return m, nil - } - // Edit succeeded — drop the user into the model list for this provider. - m.editingCustom = false - m.editTargetName = "" - m.apiKeyInput.SetValue("") - m.apiKeyMasked = false - m.apiKeyOriginal = "" - if idx := m.findCustomIdx(r.provider); idx >= 0 { - m.customIdx = idx - } - m.step = stepModel - m.prepareModelSelection(r.provider, m.customProviderEntry(r.provider, ProviderEntry{}).Model) + return m.finishCustomForm() + } + return m, nil +} + +// finishCustomForm saves the Custom provider form. It runs from the auth-header +// step for a token-based protocol and from the protocol step for an ambient one, +// which has nothing further to collect. +func (m providerTUIModel) finishCustomForm() (tea.Model, tea.Cmd) { + if m.editingCustom { + r := m.result() + if err := m.applyEditCustomProviderSave(); err != nil { return m, nil } - if m.creatingCustom { - return m.applyCreateCustomProvider() + // Edit succeeded — drop the user into the model list for this provider. + m.editingCustom = false + m.editTargetName = "" + m.apiKeyInput.SetValue("") + m.apiKeyMasked = false + m.apiKeyOriginal = "" + if idx := m.findCustomIdx(r.provider); idx >= 0 { + m.customIdx = idx } - m.confirmed = true - return m, tea.Quit + m.step = stepModel + m.prepareModelSelection(r.provider, m.customProviderEntry(r.provider, ProviderEntry{}).Model) + return m, nil } - return m, nil + if m.creatingCustom { + return m.applyCreateCustomProvider() + } + m.confirmed = true + return m, tea.Quit } func (m providerTUIModel) applyCreateCustomProvider() (tea.Model, tea.Cmd) { @@ -1206,6 +1252,9 @@ func (m providerTUIModel) applyCreateCustomProvider() (tea.Model, tea.Cmd) { AuthHeader: r.authHeader, APIKey: strings.TrimSpace(m.apiKeyInput.Value()), } + if r.protocol == llm.ProtocolAnthropicBedrock { + entry.APIKey = "" + } m.existingCfg.CustomProviders[r.provider] = entry if err := saveConfig(m.configPath, m.existingCfg); err != nil { @@ -1248,6 +1297,8 @@ func cloneProviderEntry(v ProviderEntry) ProviderEntry { AuthHeader: v.AuthHeader, TimeoutSec: v.TimeoutSec, RetryCodes: append([]int(nil), v.RetryCodes...), + AWSProfile: v.AWSProfile, + AWSRegion: v.AWSRegion, } if v.ExtraBody != nil { out.ExtraBody = make(map[string]any, len(v.ExtraBody)) @@ -1318,6 +1369,11 @@ func (m *providerTUIModel) applyEditCustomProviderSave() error { if key, edited := m.customAPIKeyForSave(); edited { entry.APIKey = key } + // Switching an entry to an ambient protocol drops the key it no longer uses, + // rather than leaving a live credential in a file nothing reads it from. + if entry.Protocol == llm.ProtocolAnthropicBedrock { + entry.APIKey = "" + } // If name changed, delete old key if r.editTargetName != "" && r.editTargetName != r.provider { if _, exists := m.existingCfg.CustomProviders[r.provider]; exists { @@ -1458,7 +1514,7 @@ func (m providerTUIModel) updateManualForm(key string, msg tea.KeyPressMsg) (tea } return m, nil case "down", "j": - if m.manualProtocolIdx < len(cpProtocols)-1 { + if m.manualProtocolIdx < len(manualProtocols)-1 { m.manualProtocolIdx++ } return m, nil @@ -1800,6 +1856,14 @@ func (m providerTUIModel) handleEnter() (tea.Model, tea.Cmd) { m.formError = err.Error() return m, nil } + if m.activeTab == tabOfficial && m.currentProvider().AmbientAuth { + // An ambient-auth provider has no key to collect, so the model step + // is the last one. Showing an API-key prompt that must be left blank + // would read as a step the user failed to complete. + m.formError = "" + m.confirmed = true + return m, tea.Quit + } m.step = stepAPIKey m.formError = "" m.loadExistingAPIKey() @@ -1927,13 +1991,21 @@ func (m providerTUIModel) result() providerTUIResult { apiKey = m.apiKeyOriginal } authHeader, _ := llm.NormalizeAuthHeader(m.cpAuthInput.Value()) + url := m.cpURLInput.Value() + // An ambient protocol collects none of these. Clearing them also + // covers switching an existing entry over to one: the url the + // previous protocol needed is dead config under bedrock, and leaving + // it behind is how a stale host outlives the change that removed it. + if m.cpAmbientProtocol() { + url, apiKey, authHeader = "", "", "" + } r := providerTUIResult{ provider: m.cpNameInput.Value(), apiKey: apiKey, isCustom: true, isEdit: m.editingCustom, editTargetName: m.editTargetName, - url: m.cpURLInput.Value(), + url: url, protocol: protocol, authHeader: authHeader, } @@ -1990,7 +2062,7 @@ func (m providerTUIModel) result() providerTUIResult { url: m.manualURLInput.Value(), model: m.manualModelInput.Value(), apiKey: apiKey, - protocol: cpProtocols[m.manualProtocolIdx], + protocol: manualProtocols[m.manualProtocolIdx], authHeader: authHeader, } } @@ -2177,9 +2249,13 @@ func (m providerTUIModel) viewCustomProviderForm(s *strings.Builder) { fields := []field{ {"Provider name", m.cpNameInput.Value(), m.cpStep == cpStepName}, {"Protocol", cpProtocols[m.cpProtocolIdx], m.cpStep == cpStepProtocol}, - {"Base URL", m.cpURLInput.Value(), m.cpStep == cpStepBaseURL}, - {"API Key", strings.Repeat("*", len(m.apiKeyInput.Value())), m.cpStep == cpStepAPIKey}, - {"Auth Header", m.cpAuthInput.Value(), m.cpStep == cpStepAuthHeader}, + } + if !m.cpAmbientProtocol() { + fields = append(fields, + field{"Base URL", m.cpURLInput.Value(), m.cpStep == cpStepBaseURL}, + field{"API Key", strings.Repeat("*", len(m.apiKeyInput.Value())), m.cpStep == cpStepAPIKey}, + field{"Auth Header", m.cpAuthInput.Value(), m.cpStep == cpStepAuthHeader}, + ) } for _, f := range fields { @@ -2198,6 +2274,9 @@ func (m providerTUIModel) viewCustomProviderForm(s *strings.Builder) { s.WriteString(cur + tuiItemStyle.Render(proto) + "\n") } } + if m.cpAmbientProtocol() { + s.WriteString(tuiDimStyle.Render(" credentials come from the AWS chain; pin a region or profile with `ocr config set custom_providers."+m.cpNameInput.Value()+".aws_region `") + "\n") + } case cpStepBaseURL: s.WriteString(" " + m.cpURLInput.View() + "\n") case cpStepAPIKey: @@ -2256,7 +2335,7 @@ func (m providerTUIModel) viewManualTab(s *strings.Builder) { fields := []field{ {"URL", m.manualURLInput.Value(), m.manualStep == manualStepURL}, - {"Protocol", cpProtocols[m.manualProtocolIdx], m.manualStep == manualStepProtocol}, + {"Protocol", manualProtocols[m.manualProtocolIdx], m.manualStep == manualStepProtocol}, {"Model", m.manualModelInput.Value(), m.manualStep == manualStepModel}, {"Auth Token", strings.Repeat("*", len(m.manualTokenInput.Value())), m.manualStep == manualStepAuthToken}, {"Auth Header", m.manualAuthHeaderInput.Value(), m.manualStep == manualStepAuthHeader}, @@ -2269,7 +2348,7 @@ func (m providerTUIModel) viewManualTab(s *strings.Builder) { case manualStepURL: s.WriteString(" " + m.manualURLInput.View() + "\n") case manualStepProtocol: - for i, proto := range cpProtocols { + for i, proto := range manualProtocols { if i == m.manualProtocolIdx { cur := " " + tuiCursorStyle.Render(tuiCursor) + " " s.WriteString(cur + tuiSelectedItemStyle.Render(proto) + "\n") diff --git a/cmd/opencodereview/provider_tui_customform_test.go b/cmd/opencodereview/provider_tui_customform_test.go index 4ee9f1def..a05cd7b52 100644 --- a/cmd/opencodereview/provider_tui_customform_test.go +++ b/cmd/opencodereview/provider_tui_customform_test.go @@ -8,6 +8,8 @@ import ( "testing" tea "charm.land/bubbletea/v2" + + "github.com/alibaba/open-code-review/internal/llm" ) // TestHandleCustomFormEnter_Steps drives handleCustomFormEnter through the create @@ -193,6 +195,64 @@ func TestUpdateCustomProviderForm_Esc(t *testing.T) { }) } +// TestCustomFormEndsAtProtocolForBedrock covers the Custom form's ambient +// branch. Bedrock has no url, no api key and no auth header to collect, so +// walking those three steps would ask for values the client never reads and +// store them as dead config. The form finishes at the protocol step instead. +func TestCustomFormEndsAtProtocolForBedrock(t *testing.T) { + setup := func(t *testing.T) providerTUIModel { + t.Helper() + cfg := &Config{} + m := newProviderTUI(cfg, filepath.Join(t.TempDir(), "c.json")) + m.activeTab = tabCustom + m.creatingCustom = true + m.cpStep = cpStepProtocol + m.cpProtocolIdx = cpProtocolIndex(llm.ProtocolAnthropicBedrock) + m.cpNameInput.SetValue("bedrock-eu") + return m + } + + t.Run("protocol step saves instead of advancing", func(t *testing.T) { + m := setup(t) + out, _ := m.handleCustomFormEnter() + got := out.(providerTUIModel) + if got.creatingCustom { + t.Error("still creating after the protocol step; the form did not finish") + } + entry, ok := got.existingCfg.CustomProviders["bedrock-eu"] + if !ok { + t.Fatal("provider was not written to config") + } + if entry.Protocol != llm.ProtocolAnthropicBedrock { + t.Errorf("Protocol = %q, want %q", entry.Protocol, llm.ProtocolAnthropicBedrock) + } + if entry.URL != "" || entry.APIKey != "" || entry.AuthHeader != "" { + t.Errorf("entry carries token-protocol fields: url=%q api_key=%q auth_header=%q", entry.URL, entry.APIKey, entry.AuthHeader) + } + }) + + t.Run("a token protocol still walks to the url step", func(t *testing.T) { + m := setup(t) + m.cpProtocolIdx = cpProtocolIndex(llm.ProtocolOpenAIChatCompletions) + out, _ := m.handleCustomFormEnter() + got := out.(providerTUIModel) + if got.cpStep != cpStepBaseURL { + t.Errorf("cpStep = %d, want cpStepBaseURL", got.cpStep) + } + }) + + t.Run("values left by a previous protocol are dropped", func(t *testing.T) { + m := setup(t) + m.cpURLInput.SetValue("https://stale.invalid/v1") + m.cpAuthInput.SetValue("X-Api-Key") + m.apiKeyInput.SetValue("sk-stale") + r := m.result() + if r.url != "" || r.apiKey != "" || r.authHeader != "" { + t.Errorf("result kept stale fields: url=%q apiKey=%q authHeader=%q", r.url, r.apiKey, r.authHeader) + } + }) +} + // TestUpdateCustomProviderForm_ProtocolNav covers up/down protocol selection. func TestUpdateCustomProviderForm_ProtocolNav(t *testing.T) { m := newProviderTUI(&Config{}, "") diff --git a/cmd/opencodereview/provider_tui_funcs_test.go b/cmd/opencodereview/provider_tui_funcs_test.go index 6b2ca9820..a961d43a9 100644 --- a/cmd/opencodereview/provider_tui_funcs_test.go +++ b/cmd/opencodereview/provider_tui_funcs_test.go @@ -292,6 +292,8 @@ func TestCloneProviderEntry_CopiesEveryField(t *testing.T) { RetryCodes: []int{403}, ExtraBody: map[string]any{"temperature": 0.7}, ExtraHeaders: map[string]string{"X-Trace": "on"}, + AWSRegion: "us-west-2", + AWSProfile: "example-profile", } rv := reflect.ValueOf(orig) diff --git a/cmd/opencodereview/provider_tui_test.go b/cmd/opencodereview/provider_tui_test.go index 5e108b892..5edf3e9b3 100644 --- a/cmd/opencodereview/provider_tui_test.go +++ b/cmd/opencodereview/provider_tui_test.go @@ -2762,13 +2762,15 @@ func TestApplyCustomProviderConfigNormalizesAuthHeader(t *testing.T) { // --- protocol normalization / openai-responses support --- func TestCpProtocols_ContainsAllCanonicalNames(t *testing.T) { - // The slice drives both Custom and Manual forms — every canonical protocol - // must appear, in canonical order, so the TUI result() picks up the right - // string for each index. + // The Custom form offers every canonical protocol, in canonical order, so + // result() picks up the right string for each index. The Manual form writes + // llm.url + llm.auth_token and so omits bedrock, which uses neither; the two + // lists share their prefix, which is what keeps a single index helper honest. want := []string{ llm.ProtocolAnthropic, llm.ProtocolOpenAIChatCompletions, llm.ProtocolOpenAIResponses, + llm.ProtocolAnthropicBedrock, } if len(cpProtocols) != len(want) { t.Fatalf("cpProtocols has %d entries, want %d", len(cpProtocols), len(want)) @@ -2778,6 +2780,21 @@ func TestCpProtocols_ContainsAllCanonicalNames(t *testing.T) { t.Errorf("cpProtocols[%d] = %q, want %q", i, cpProtocols[i], p) } } + + wantManual := want[:len(want)-1] + if len(manualProtocols) != len(wantManual) { + t.Fatalf("manualProtocols has %d entries, want %d", len(manualProtocols), len(wantManual)) + } + for i, p := range wantManual { + if manualProtocols[i] != p { + t.Errorf("manualProtocols[%d] = %q, want %q", i, manualProtocols[i], p) + } + } + for _, p := range manualProtocols { + if p == llm.ProtocolAnthropicBedrock { + t.Error("manualProtocols offers bedrock; the llm block has no region, profile or use for its url and token") + } + } } func TestCpProtocolIndex(t *testing.T) { @@ -2792,6 +2809,7 @@ func TestCpProtocolIndex(t *testing.T) { {"alias openai normalizes to chat-completions", "openai", 1}, {"alias OPENAI case-insensitive", "OPENAI", 1}, {"empty defaults to chat-completions", "", 1}, + {"canonical bedrock", llm.ProtocolAnthropicBedrock, 3}, {"unknown defaults to chat-completions", "grpc", 1}, } for _, tt := range tests { @@ -2969,7 +2987,7 @@ func TestApplyManualConfig_DoubleWritesProtocolAndUseAnthropic(t *testing.T) { } // TestProviderTUIResult_ManualProtocolIsCanonical makes sure result() for the -// Manual tab returns the canonical protocol name picked from cpProtocols. +// Manual tab returns the canonical protocol name picked from manualProtocols. func TestProviderTUIResult_ManualProtocolIsCanonical(t *testing.T) { cfg := &Config{} m := newProviderTUI(cfg, "") @@ -2979,7 +2997,7 @@ func TestProviderTUIResult_ManualProtocolIsCanonical(t *testing.T) { m.manualModelInput.SetValue("m") m.manualTokenInput.SetValue("t") - for i, want := range cpProtocols { + for i, want := range manualProtocols { m.manualProtocolIdx = i r := m.result() if r.protocol != want { diff --git a/go.mod b/go.mod index 32c78d6d3..918636ec2 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( charm.land/bubbletea/v2 v2.0.8 charm.land/lipgloss/v2 v2.0.6 github.com/anthropics/anthropic-sdk-go v1.63.1 + github.com/aws/aws-sdk-go-v2/config v1.32.35 github.com/bmatcuk/doublestar/v4 v4.10.0 github.com/google/uuid v1.6.0 github.com/modelcontextprotocol/go-sdk v1.7.0 @@ -29,6 +30,20 @@ require ( require ( github.com/atotto/clipboard v0.1.4 // indirect + github.com/aws/aws-sdk-go-v2 v1.43.4 // indirect + github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.7.16 // indirect + github.com/aws/aws-sdk-go-v2/credentials v1.19.34 // indirect + github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.18.35 // indirect + github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.35 // indirect + github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.35 // indirect + github.com/aws/aws-sdk-go-v2/internal/v4a v1.4.36 // indirect + github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.13.15 // indirect + github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.13.35 // indirect + github.com/aws/aws-sdk-go-v2/service/signin v1.5.4 // indirect + github.com/aws/aws-sdk-go-v2/service/sso v1.33.4 // indirect + github.com/aws/aws-sdk-go-v2/service/ssooidc v1.38.4 // indirect + github.com/aws/aws-sdk-go-v2/service/sts v1.45.4 // indirect + github.com/aws/smithy-go v1.27.6 // indirect github.com/bahlo/generic-list-go v0.2.0 // indirect github.com/buger/jsonparser v1.1.2 // indirect github.com/cenkalti/backoff/v5 v5.0.3 // indirect diff --git a/go.sum b/go.sum index ec97b9805..20f379570 100644 --- a/go.sum +++ b/go.sum @@ -8,6 +8,36 @@ github.com/anthropics/anthropic-sdk-go v1.63.1 h1:M9dIoZzUWB453ulVpBkQp3FX0769ud github.com/anthropics/anthropic-sdk-go v1.63.1/go.mod h1:3EfIfmFqxH6rbiLcIP4tPFyXL/IHakx2wDG4OU+TIEI= github.com/atotto/clipboard v0.1.4 h1:EH0zSVneZPSuFR11BlR9YppQTVDbh5+16AmcJi4g1z4= github.com/atotto/clipboard v0.1.4/go.mod h1:ZY9tmq7sm5xIbd9bOK4onWV4S6X0u6GY7Vn0Yu86PYI= +github.com/aws/aws-sdk-go-v2 v1.43.4 h1:b9FTvbRwy+JCsfp2Wp6wV/KbOx3Aj7nkoFb2cRX0IhE= +github.com/aws/aws-sdk-go-v2 v1.43.4/go.mod h1:70vwSy16txshwG+g55WkpgPKDIByzHI8ccBsOteo3bQ= +github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.7.16 h1:aiuaKlDweRC5qExJondpWjOgyzMHpofpwspGXUtwn4c= +github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.7.16/go.mod h1:nG/LOlmox9BDe9HvQnXWzgcK8uKbgBMZ/Hp5pVt/21I= +github.com/aws/aws-sdk-go-v2/config v1.32.35 h1:UEzXuET8E42lxBPijuACu/tEK7v5lFPlk0Q+GT5WD9E= +github.com/aws/aws-sdk-go-v2/config v1.32.35/go.mod h1:KaMtJpFa2JlL2BStjjHQVwQpzZEmw+ND/EgVrfFoo2g= +github.com/aws/aws-sdk-go-v2/credentials v1.19.34 h1:y6GkSmcv5myd1ngrYbGmiLlwQqB6TQhOuN/tbSSuWDY= +github.com/aws/aws-sdk-go-v2/credentials v1.19.34/go.mod h1:w3dTcnDVoQIewjo7JG45hduAToikiIFLC4FIO7fndvw= +github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.18.35 h1:+S7kbJoLDDQ5tE+lHrUBgMkzC8NLgsaioS2F3dVoFAE= +github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.18.35/go.mod h1:Ak7xXviIARfFdNUJ9Etb0bdVDt/KAvKjMGJVLWXDzik= +github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.35 h1:kzVuGlatQtYinwBJEEyLAbggepCoavosiaHHX9+fD+c= +github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.35/go.mod h1:0yLx0yEI+SfqeJMPvOtIEFoZbiQYXMGszBueiutQyaI= +github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.35 h1:WK6CjihTuLisCjSKKbildJ79sGZZgbBz3iNa7VsKIhU= +github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.35/go.mod h1:KYleN57luLoe97R7vTnx8PMcVrr9gAcRECtOjl91DNg= +github.com/aws/aws-sdk-go-v2/internal/v4a v1.4.36 h1:jbGY4CXLzZElOXgGsexlC3Hi+3YM0rSmk4opFXKqg/k= +github.com/aws/aws-sdk-go-v2/internal/v4a v1.4.36/go.mod h1:uBu/9aKsS/UQGc72RAt3y54kjgYQxmhut8ZD2dXCDNE= +github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.13.15 h1:JJLBQxwY+AFwuPAi5ivGc1ChnTdUt4cXMv7e76m2c/Y= +github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.13.15/go.mod h1:lQknBIe78MVL0cQOQDlag8KGflMbMEVFx9mB6O8ENvk= +github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.13.35 h1:BBEElKh4a+rKshvjrfpajTe9CbpZvrbb4Jkg2PB7RzA= +github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.13.35/go.mod h1:zaZk983w//8beSruBVec/mr4CmDwgZitW/qzGhAAX0g= +github.com/aws/aws-sdk-go-v2/service/signin v1.5.4 h1:cOJELVNrq5Q3Udry2GLuHUM7MhwpeaQRdYaoa6GI/yI= +github.com/aws/aws-sdk-go-v2/service/signin v1.5.4/go.mod h1:f4LxzKBtaTxD7xh3PiVg3CE1tchQemfmghaJr+NbK2c= +github.com/aws/aws-sdk-go-v2/service/sso v1.33.4 h1:AMW7a7S8iQaHjBYZdU3PCq4GKRPijTPRAc7e6XtEThY= +github.com/aws/aws-sdk-go-v2/service/sso v1.33.4/go.mod h1:QQNsFV1DVXoXcZt18FS8lI8rtUrlDyAuWZLQ5shunv4= +github.com/aws/aws-sdk-go-v2/service/ssooidc v1.38.4 h1:AsbZcJAQPRmHDJG8K1N0pof/1zPWjVT8TFlTWuGLSvo= +github.com/aws/aws-sdk-go-v2/service/ssooidc v1.38.4/go.mod h1:6imqztH0//t0mKbl6yWl7swSEl7F/w32oAmqB3vP1ag= +github.com/aws/aws-sdk-go-v2/service/sts v1.45.4 h1:w/AryDYMjSUANSQ2uoZxJovUsMTwWJNTv3IMex30Y+4= +github.com/aws/aws-sdk-go-v2/service/sts v1.45.4/go.mod h1:WeBiAa67azG7Su9Vf+ChGDBLiAozJCXzdjXiPBUwtbc= +github.com/aws/smithy-go v1.27.6 h1:0zjT8jgK3jbrTT7JJ3EE6JsMhX8JTrZ+f1sEndYDXrA= +github.com/aws/smithy-go v1.27.6/go.mod h1:YE2RhdIuDbA5E5bTdciG9KrW3+TiEONeUWCqxX9i1Fc= github.com/aymanbagabas/go-udiff v0.4.1 h1:OEIrQ8maEeDBXQDoGCbbTTXYJMYRCRO1fnodZ12Gv5o= github.com/aymanbagabas/go-udiff v0.4.1/go.mod h1:0L9PGwj20lrtmEMeyw4WKJ/TMyDtvAoK9bf2u/mNo3w= github.com/bahlo/generic-list-go v0.2.0 h1:5sz/EEAK+ls5wF+NeqDpk5+iNdMDXrh3z3nPnH1Wvgk= diff --git a/internal/llm/bedrock_test.go b/internal/llm/bedrock_test.go new file mode 100644 index 000000000..7f1a3e80b --- /dev/null +++ b/internal/llm/bedrock_test.go @@ -0,0 +1,523 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "encoding/json" + "errors" + "os" + "path/filepath" + "strings" + "testing" +) + +// writeConfig writes an OCR config file to a temp dir and returns its path. +func writeConfig(t *testing.T, cfg map[string]any) string { + t.Helper() + path := filepath.Join(t.TempDir(), "config.json") + data, err := json.Marshal(cfg) + if err != nil { + t.Fatalf("marshal config: %v", err) + } + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + return path +} + +// TestBedrockProtocolIsRecognized guards the three-part contract documented in +// protocol.go: a new protocol needs a constant, a NormalizeProtocol case, and a +// ValidateProtocol entry. Missing the last one turns a valid config into +// "unsupported protocol". +func TestBedrockProtocolIsRecognized(t *testing.T) { + for _, raw := range []string{"anthropic-bedrock", "ANTHROPIC-BEDROCK", " Anthropic-Bedrock "} { + if got := NormalizeProtocol(raw); got != ProtocolAnthropicBedrock { + t.Errorf("NormalizeProtocol(%q) = %q, want %q", raw, got, ProtocolAnthropicBedrock) + } + } + if err := ValidateProtocol(ProtocolAnthropicBedrock); err != nil { + t.Errorf("ValidateProtocol(%q) = %v, want nil", ProtocolAnthropicBedrock, err) + } +} + +// TestBedrockProviderIsRegistered pins the preset's shape. An api_key or a +// BaseURL here would be wrong: credentials come from the AWS chain and the host +// is derived from the region. +func TestBedrockProviderIsRegistered(t *testing.T) { + p, ok := LookupProvider("bedrock") + if !ok { + t.Fatal("LookupProvider(\"bedrock\") not found") + } + if p.Protocol != ProtocolAnthropicBedrock { + t.Errorf("Protocol = %q, want %q", p.Protocol, ProtocolAnthropicBedrock) + } + if !p.AmbientAuth { + t.Error("AmbientAuth = false, want true — bedrock signs with SigV4 and has no api_key") + } + if p.BaseURL != "" { + t.Errorf("BaseURL = %q, want empty — the region determines the bedrock-runtime host", p.BaseURL) + } + if p.EnvVar != "" { + t.Errorf("EnvVar = %q, want empty — there is no API key env var to fall back to", p.EnvVar) + } +} + +// TestResolveBedrockWithoutAPIKey is the regression test for the two gates that +// rejected a correct Bedrock config: the api_key requirement in +// tryProviderConfig, and the URL-and-Token completeness check in +// ResolveEndpointWithModelOverride. Either one turns a valid setup into +// "no valid LLM endpoint configured", which reads as "you forgot to configure +// anything". +func TestResolveBedrockWithoutAPIKey(t *testing.T) { + path := writeConfig(t, map[string]any{ + "provider": "bedrock", + "model": "us.anthropic.claude-sonnet-4-6", + "providers": map[string]any{"bedrock": map[string]any{}}, + }) + + ep, err := ResolveEndpoint(path) + if err != nil { + t.Fatalf("ResolveEndpoint: %v", err) + } + if ep.Protocol != ProtocolAnthropicBedrock { + t.Errorf("Protocol = %q, want %q", ep.Protocol, ProtocolAnthropicBedrock) + } + if !ep.AmbientAuth { + t.Error("AmbientAuth = false, want true") + } + if ep.Token != "" { + t.Errorf("Token = %q, want empty", ep.Token) + } + if ep.Model != "us.anthropic.claude-sonnet-4-6" { + t.Errorf("Model = %q, want us.anthropic.claude-sonnet-4-6", ep.Model) + } +} + +// TestBedrockDoesNotRunAPIKeyCmd pins that an ambient-auth provider never +// executes api_key_cmd. A signed request has no use for the output, and the +// command is typically a secret-manager read — running it means a real +// 1Password / Touch ID prompt for a value that is immediately discarded. The +// sentinel file proves non-execution; a nil error would not. +func TestBedrockDoesNotRunAPIKeyCmd(t *testing.T) { + sentinel := filepath.Join(t.TempDir(), "ran") + path := writeConfig(t, map[string]any{ + "provider": "bedrock", + "model": "us.anthropic.claude-sonnet-4-6", + "providers": map[string]any{ + "bedrock": map[string]any{"api_key_cmd": "touch '" + sentinel + "'; echo sk-should-never-be-used"}, + }, + }) + + ep, err := ResolveEndpoint(path) + if err != nil { + t.Fatalf("ResolveEndpoint: %v", err) + } + if _, err := os.Stat(sentinel); !os.IsNotExist(err) { + t.Error("api_key_cmd ran for an ambient-auth provider") + } + if ep.Token != "" { + t.Errorf("Token = %q, want empty", ep.Token) + } +} + +// TestResolveBedrockPassesAWSSettings covers aws_profile / aws_region reaching +// the client, so a review run is reproducible without exporting AWS_PROFILE. +func TestResolveBedrockPassesAWSSettings(t *testing.T) { + // NewLLMClient loads AWS config for the bedrock protocol; point it at empty + // files so the test does not depend on whatever profiles the developer or + // the CI runner happens to have. + t.Setenv("AWS_CONFIG_FILE", filepath.Join(t.TempDir(), "config")) + t.Setenv("AWS_SHARED_CREDENTIALS_FILE", filepath.Join(t.TempDir(), "credentials")) + + path := writeConfig(t, map[string]any{ + "provider": "bedrock", + "model": "us.anthropic.claude-sonnet-4-6", + "providers": map[string]any{ + "bedrock": map[string]any{"aws_profile": "example-profile", "aws_region": "us-west-2"}, + }, + }) + + ep, err := ResolveEndpoint(path) + if err != nil { + t.Fatalf("ResolveEndpoint: %v", err) + } + if ep.AWSProfile != "example-profile" { + t.Errorf("AWSProfile = %q, want example-profile", ep.AWSProfile) + } + if ep.AWSRegion != "us-west-2" { + t.Errorf("AWSRegion = %q, want us-west-2", ep.AWSRegion) + } + + cfg := ClientConfig{} + if c, ok := NewLLMClient(ep, nil).(*AnthropicClient); ok { + cfg = c.cfg + } else { + t.Fatal("NewLLMClient did not return *AnthropicClient for the bedrock protocol") + } + if cfg.AWSProfile != "example-profile" || cfg.AWSRegion != "us-west-2" { + t.Errorf("ClientConfig AWS settings = %q/%q, want example-profile/us-west-2", cfg.AWSProfile, cfg.AWSRegion) + } +} + +// TestAmbientAuthFollowsTheEffectiveProtocol covers the entry-level protocol +// override. An entry may override a preset's protocol, so reading ambient auth +// off the preset alone lets `protocol: openai` on the bedrock preset resolve with +// no token and no URL — an endpoint that cannot work, reported as if configured. +func TestAmbientAuthFollowsTheEffectiveProtocol(t *testing.T) { + t.Run("bedrock preset overridden to a token protocol needs a key again", func(t *testing.T) { + path := writeConfig(t, map[string]any{ + "provider": "bedrock", + "model": "gpt-5.4", + "providers": map[string]any{ + "bedrock": map[string]any{"protocol": "openai", "url": "https://example.invalid/v1"}, + }, + }) + if _, err := ResolveEndpoint(path); err == nil { + t.Error("resolved with no api_key after the protocol was overridden away from bedrock; want an error") + } + }) + + t.Run("entry that selects the bedrock protocol signs without a key", func(t *testing.T) { + path := writeConfig(t, map[string]any{ + "provider": "anthropic", + "model": "us.anthropic.claude-sonnet-4-6", + "providers": map[string]any{ + "anthropic": map[string]any{"protocol": ProtocolAnthropicBedrock}, + }, + }) + t.Setenv("ANTHROPIC_API_KEY", "") + ep, err := ResolveEndpoint(path) + if err != nil { + t.Fatalf("ResolveEndpoint: %v", err) + } + if !ep.AmbientAuth { + t.Error("AmbientAuth = false for an entry whose protocol is anthropic-bedrock") + } + }) +} + +// TestBedrockIsRejectedOnTheURLAndTokenPaths covers the two strategies that +// describe a single HTTP endpoint. Both validated anthropic-bedrock as a +// protocol name and then ignored it — the request would have been signed and +// re-hosted while the url and token the block declares went unused, with no +// region or profile anywhere to state where it went instead. +func TestBedrockIsRejectedOnTheURLAndTokenPaths(t *testing.T) { + t.Run("OCR_LLM_PROTOCOL", func(t *testing.T) { + t.Setenv("OCR_LLM_URL", "https://example.invalid/v1") + t.Setenv("OCR_LLM_TOKEN", "sk-test") + t.Setenv("OCR_LLM_MODEL", "us.anthropic.claude-sonnet-4-6") + t.Setenv("OCR_LLM_PROTOCOL", ProtocolAnthropicBedrock) + + _, err := ResolveEndpoint(writeConfig(t, map[string]any{})) + if err == nil { + t.Fatal("resolved with OCR_LLM_PROTOCOL=anthropic-bedrock; want an error naming the variable") + } + for _, want := range []string{"OCR_LLM_PROTOCOL", "aws_region", `"provider": "bedrock"`} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not mention %q", err, want) + } + } + }) + + t.Run("llm.protocol", func(t *testing.T) { + for _, k := range []string{"OCR_LLM_URL", "OCR_LLM_TOKEN", "OCR_LLM_MODEL", "OCR_LLM_PROTOCOL"} { + t.Setenv(k, "") + } + path := writeConfig(t, map[string]any{ + "llm": map[string]any{ + "url": "https://example.invalid/v1", + "auth_token": "sk-test", + "model": "us.anthropic.claude-sonnet-4-6", + "protocol": ProtocolAnthropicBedrock, + }, + }) + + _, err := ResolveEndpoint(path) + if err == nil { + t.Fatal("resolved with llm.protocol=anthropic-bedrock; want an error naming the key") + } + if !strings.Contains(err.Error(), "llm.protocol") { + t.Errorf("error %q does not mention llm.protocol", err) + } + }) +} + +// TestCustomProviderCanSelectBedrock is the other half of that contract: where +// a provider entry exists there is somewhere to put aws_region and aws_profile, +// so bedrock is configurable — and the url every other protocol requires is not +// demanded, because the region decides the host and the client never reads it. +func TestCustomProviderCanSelectBedrock(t *testing.T) { + t.Run("no url required", func(t *testing.T) { + path := writeConfig(t, map[string]any{ + "provider": "mine", + "model": "us.anthropic.claude-sonnet-4-6", + "custom_providers": map[string]any{ + "mine": map[string]any{ + "protocol": ProtocolAnthropicBedrock, + "aws_region": "eu-west-1", + "aws_profile": "example-profile", + }, + }, + }) + + ep, err := ResolveEndpoint(path) + if err != nil { + t.Fatalf("ResolveEndpoint: %v", err) + } + if !ep.AmbientAuth { + t.Error("AmbientAuth = false for a custom provider on the bedrock protocol") + } + if ep.AWSRegion != "eu-west-1" || ep.AWSProfile != "example-profile" { + t.Errorf("AWSRegion/AWSProfile = %q/%q, want eu-west-1/example-profile", ep.AWSRegion, ep.AWSProfile) + } + if ep.Token != "" { + t.Errorf("Token = %q, want empty", ep.Token) + } + }) + + t.Run("url still required for every other protocol", func(t *testing.T) { + path := writeConfig(t, map[string]any{ + "provider": "mine", + "model": "gpt-5.5", + "custom_providers": map[string]any{ + "mine": map[string]any{"protocol": "openai", "api_key": "sk-test"}, + }, + }) + + _, err := ResolveEndpoint(path) + if err == nil { + t.Fatal("resolved a custom openai provider with no url; want an error") + } + if !strings.Contains(err.Error(), "url") { + t.Errorf("error %q does not mention the missing url", err) + } + }) +} + +// TestBedrockModelOverrideIsNotGatedByThePresetList covers what the preset's +// own documentation promises: any identifier Bedrock will route. A preset's +// Models list otherwise acts as an allowlist for --model, which cannot work for +// identifiers scoped to an account and a region — an application inference +// profile ARN, the value to use when spend has to be attributed, can never +// appear in a list compiled upstream. +func TestBedrockModelOverrideIsNotGatedByThePresetList(t *testing.T) { + path := writeConfig(t, map[string]any{ + "provider": "bedrock", + "model": "us.anthropic.claude-sonnet-4-6", + "providers": map[string]any{"bedrock": map[string]any{"aws_region": "us-west-2"}}, + }) + + for _, model := range []string{ + "arn:aws:bedrock:us-west-2:123456789012:application-inference-profile/abc123", + "us.anthropic.claude-haiku-4-5", // a real ID the preset list does not carry + } { + ep, err := ResolveEndpointWithModelOverride(path, model) + if err != nil { + t.Errorf("ResolveEndpointWithModelOverride(%q) = %v, want it accepted", model, err) + continue + } + if ep.Model != model { + t.Errorf("resolved model = %q, want %q", ep.Model, model) + } + } +} + +// TestModelOverrideStillGatedForKeyBasedProviders keeps the relaxation scoped to +// ambient auth: a typo against a hosted API should still be caught locally. +func TestModelOverrideStillGatedForKeyBasedProviders(t *testing.T) { + path := writeConfig(t, map[string]any{ + "provider": "anthropic", + "model": "claude-sonnet-5", + "providers": map[string]any{"anthropic": map[string]any{"api_key": "sk-test-not-a-real-key"}}, + }) + + if _, err := ResolveEndpointWithModelOverride(path, "claude-sonnet-5-typo"); err == nil { + t.Error("an unlisted model was accepted for a key-based provider; want an error") + } +} + +// TestNonAmbientProviderStillRequiresAPIKey makes sure relaxing the gate for +// ambient auth did not relax it for everyone. +func TestNonAmbientProviderStillRequiresAPIKey(t *testing.T) { + t.Setenv("ANTHROPIC_API_KEY", "") + path := writeConfig(t, map[string]any{ + "provider": "anthropic", + "model": "claude-opus-4-6", + "providers": map[string]any{"anthropic": map[string]any{}}, + }) + + if _, err := ResolveEndpoint(path); err == nil { + t.Fatal("ResolveEndpoint succeeded with no api_key for a non-ambient provider; want an error") + } +} + +// TestExplainErrorClassifiesBedrockFailures covers the diagnosis Bedrock's own +// wording does not give. Two of these are actively misleading: the API-key +// complaint names a credential the user cannot configure, and a model absent +// from the region reads as a malformed identifier. +func TestExplainErrorClassifiesBedrockFailures(t *testing.T) { + // The bearer-token branch consults this variable, so a value in the ambient + // environment flips the expected message on a developer machine that has one + // exported. Pin it empty rather than depending on whoever runs the suite. + t.Setenv("AWS_BEARER_TOKEN_BEDROCK", "") + + client := &AnthropicClient{bedrock: true, awsRegion: "us-west-2", awsProfile: "example-profile"} + + tests := []struct { + name string + err error + wantAll []string + wantNone []string + }{ + { + name: "bearer token reached the request", + err: errors.New(`403 Forbidden {"Message":"Invalid API Key format: Must start with pre-defined prefix"}`), + wantAll: []string{"no api_key applies to bedrock", "region us-west-2", "profile example-profile"}, + }, + { + name: "expired session", + err: errors.New("operation error: get credentials: ExpiredToken: the security token included in the request is expired"), + wantAll: []string{"aws sso login --profile example-profile"}, + }, + { + // Bedrock answers both "IAM forbids this" and "the account has not + // enabled this model" with AccessDeniedException, and the fixes have + // nothing in common. This is the model-access shape, verbatim. + name: "model access not enabled for the account", + err: errors.New(`operation error Bedrock Runtime: InvokeModel, https response error StatusCode: 403, AccessDeniedException: You don't have access to the model with the specified model ID.`), + wantAll: []string{"model access is granted per account and per region", "console"}, + wantNone: []string{"bedrock:InvokeModel"}, + }, + { + name: "IAM gap, not a bad credential", + err: errors.New("operation error Bedrock Runtime: AccessDeniedException: User: arn:aws:sts::x:assumed-role/y is not authorized to perform: bedrock:InvokeModel"), + wantAll: []string{"bedrock:InvokeModel", "authorization gap"}, + wantNone: []string{"sso login"}, + }, + { + // The same IAM fix reached by different wording, and deliberately + // without "AccessDenied" in the text: this pins the phrase itself to + // the authorization branch rather than the exception name. + name: "IAM gap phrased as an unauthorized API operation", + err: errors.New(`operation error Bedrock Runtime: InvokeModel, https response error StatusCode: 403, Your account is not authorized to invoke this API operation.`), + wantAll: []string{"bedrock:InvokeModel", "authorization gap"}, + wantNone: []string{"console"}, + }, + { + name: "model absent from the region", + err: errors.New("operation error Bedrock Runtime: ValidationException: The provided model identifier is invalid."), + wantAll: []string{"aws bedrock list-inference-profiles --region us-west-2", "-v1:0"}, + }, + { + // A request-shape ValidationException is not a model-ID problem, and + // telling the user to go list inference profiles wastes their time. + name: "validation error about the request, not the model", + err: errors.New("operation error Bedrock Runtime: ValidationException: Input is too long for requested model."), + wantAll: []string{"Input is too long", "region us-west-2"}, + wantNone: []string{"list-inference-profiles", "bedrock:InvokeModel"}, + }, + { + // A bare "expired" match would claim this is an SSO session problem. + name: "expired TLS certificate is not an expired session", + err: errors.New(`Post "https://bedrock-runtime.us-west-2.amazonaws.com/v1/messages": tls: failed to verify certificate: x509: certificate has expired or is not yet valid`), + wantAll: []string{"certificate has expired"}, + wantNone: []string{"sso login", "credentials are expired"}, + }, + { + name: "anything else keeps its own wording and gains context", + err: errors.New("connection reset by peer"), + wantAll: []string{"connection reset by peer", "region us-west-2"}, + wantNone: []string{"authorization gap", "sso login"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := client.explainError("us.anthropic.claude-sonnet-4-6", tc.err) + if got == nil { + t.Fatal("explainError returned nil for a non-nil error") + } + if !errors.Is(got, tc.err) { + t.Error("original error is not wrapped; callers lose the service's own message") + } + for _, want := range tc.wantAll { + if !strings.Contains(got.Error(), want) { + t.Errorf("message %q does not contain %q", got, want) + } + } + for _, unwanted := range tc.wantNone { + if strings.Contains(got.Error(), unwanted) { + t.Errorf("message %q should not mention %q", got, unwanted) + } + } + }) + } +} + +// TestExplainErrorNamesTheBearerTokenVariable separates the two ways the same +// 403 arrives: an SSO token the SDK attached on its own, versus a token the user +// set deliberately. The fix differs, so the message has to. +func TestExplainErrorNamesTheBearerTokenVariable(t *testing.T) { + t.Setenv("AWS_BEARER_TOKEN_BEDROCK", "sk-not-a-real-token") + client := &AnthropicClient{bedrock: true, awsRegion: "us-west-2"} + err := client.explainError("m", errors.New(`{"Message":"Invalid API Key format: Must start with pre-defined prefix"}`)) + if !strings.Contains(err.Error(), "AWS_BEARER_TOKEN_BEDROCK") { + t.Errorf("message %q does not name AWS_BEARER_TOKEN_BEDROCK", err) + } +} + +// TestExplainErrorLeavesNonBedrockErrorsAlone keeps the diagnosis scoped: every +// other protocol shares this client type. +func TestExplainErrorLeavesNonBedrockErrorsAlone(t *testing.T) { + client := &AnthropicClient{} + original := errors.New("401 Unauthorized") + if got := client.explainError("m", original); got != original { + t.Errorf("explainError rewrote a non-bedrock error: %q", got) + } + if got := client.explainError("m", nil); got != nil { + t.Errorf("explainError(nil) = %v, want nil", got) + } +} + +// TestBedrockContextReportsResolvedRegion covers what `ocr llm test` prints: +// bedrock has no configured URL, so the resolved region is the only way to see +// where a request went. +func TestBedrockContextReportsResolvedRegion(t *testing.T) { + client := &AnthropicClient{bedrock: true, awsRegion: "us-west-2", awsProfile: "example-profile"} + region, profile, ok := client.BedrockContext() + if !ok { + t.Fatal("ok = false for a bedrock client") + } + if region != "us-west-2" || profile != "example-profile" { + t.Errorf("BedrockContext() = %q/%q, want us-west-2/example-profile", region, profile) + } + + if _, _, ok := (&AnthropicClient{}).BedrockContext(); ok { + t.Error("ok = true for a non-bedrock client") + } +} + +// TestBedrockClientReportsAWSFailureAsError is the guard against the SDK's +// bedrock.WithLoadDefaultConfig, which panics when AWS config cannot be loaded. +// A CLI must not hand a user a stack trace because their session expired, so the +// failure is deferred to the first request instead. +func TestBedrockClientReportsAWSFailureAsError(t *testing.T) { + // An unresolvable profile makes LoadDefaultConfig fail deterministically. + t.Setenv("AWS_CONFIG_FILE", filepath.Join(t.TempDir(), "nonexistent-config")) + t.Setenv("AWS_SHARED_CREDENTIALS_FILE", filepath.Join(t.TempDir(), "nonexistent-creds")) + + client := NewAnthropicBedrockClient(ClientConfig{ + Model: "us.anthropic.claude-sonnet-4-6", + AWSProfile: "definitely-not-a-real-profile", + }) + if client == nil { + t.Fatal("NewAnthropicBedrockClient returned nil; it must always return a client so the error can surface per-request") + } + if client.initErr == nil { + t.Skip("this environment resolved an AWS config for a bogus profile; nothing to assert") + } + if _, err := client.CompletionsWithCtx(t.Context(), ChatRequest{}); err == nil { + t.Error("CompletionsWithCtx returned nil error despite a construction failure") + } +} diff --git a/internal/llm/client.go b/internal/llm/client.go index e3ec71907..2c9a437a6 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -4,6 +4,7 @@ // Package llm provides LLM client interfaces supporting multiple protocols. // Supported protocols (canonical names, see protocol.go): // - "anthropic" — Anthropic Messages API +// - "anthropic-bedrock" — the same API served by AWS Bedrock, SigV4-signed // - "openai" — OpenAI Chat Completions API // - "openai-responses" — OpenAI Responses API package llm @@ -15,12 +16,15 @@ import ( "fmt" "io" "net/http" + "os" "strings" "sync" "time" anthropic "github.com/anthropics/anthropic-sdk-go" + "github.com/anthropics/anthropic-sdk-go/bedrock" "github.com/anthropics/anthropic-sdk-go/option" + awsconfig "github.com/aws/aws-sdk-go-v2/config" openai "github.com/openai/openai-go/v3" openaiopt "github.com/openai/openai-go/v3/option" "github.com/openai/openai-go/v3/shared" @@ -29,6 +33,15 @@ import ( var AppVersion = "dev" +// bedrockConfigLoadTimeout bounds how long NewAnthropicBedrockClient may spend +// in awsconfig.LoadDefaultConfig. Credential resolution itself (SSO refresh, +// AssumeRole, credential_process) is lazy — deferred to the first signed +// request, where cfg.Timeout already applies — but region auto-detection can +// still reach the network, and this keeps that bounded rather than relying +// solely on the AWS SDK's own defaults. Package var, not const, so tests can +// shrink it, same as keyCmdTimeout. +var bedrockConfigLoadTimeout = 60 * time.Second + func userAgent(provider string) string { ua := "open-code-review/" + AppVersion if provider != "" { @@ -223,6 +236,11 @@ type ClientConfig struct { // the request path changes. That is the state for llm test, and for any // caller that builds a client without one. retryCollector *RetryCollector + + // AWSProfile and AWSRegion are used only by SigV4 providers (bedrock). + // Empty means the standard AWS credential chain decides. + AWSProfile string + AWSRegion string } // retryCodesMiddleware returns an HTTP middleware that forces the SDK to retry @@ -276,10 +294,14 @@ func NewLLMClient(ep ResolvedEndpoint, collector *RetryCollector) LLMClient { ExtraHeaders: ep.ExtraHeaders, RetryCodes: ep.RetryCodes, retryCollector: collector, + AWSProfile: ep.AWSProfile, + AWSRegion: ep.AWSRegion, } switch ep.Protocol { case ProtocolAnthropic: return NewAnthropicClient(cfg) + case ProtocolAnthropicBedrock: + return NewAnthropicBedrockClient(cfg) case ProtocolOpenAIResponses: return NewOpenAIResponsesClient(cfg) default: @@ -730,6 +752,21 @@ func (c *OpenAIClient) mapOpenAIResponse(sdkResp *openai.ChatCompletion) *ChatRe type AnthropicClient struct { cfg ClientConfig sdk anthropic.Client + + // initErr defers a construction failure to the first request. The client + // factory returns an LLMClient with no error channel, and the alternative — + // panicking, as the SDK's own bedrock helper does — would surface a Go + // stack trace to someone whose real problem is an expired AWS session. + initErr error + + // bedrock marks a client whose requests are SigV4-signed for Bedrock, along + // with the region and profile that were actually resolved. Bedrock's + // rejections need translating (see explainError) and the resolved region is + // worth showing, because a request sent to the wrong one fails in a way that + // looks like a bad model ID. + bedrock bool + awsRegion string + awsProfile string } // NewAnthropicClient creates a new Anthropic Messages API client. @@ -791,6 +828,210 @@ func NewAnthropicClient(cfg ClientConfig) *AnthropicClient { } } +// NewAnthropicBedrockClient creates a client for Anthropic models served by AWS +// Bedrock. +// +// The wire format is the Messages API, so this reuses AnthropicClient wholesale; +// the bedrock middleware from the official SDK handles what differs — SigV4 +// signing, moving the model from the body into the URL path, injecting +// anthropic_version, and deriving the host from the region. +// +// No api_key is involved. Credentials come from the standard AWS chain +// (AWS_PROFILE, SSO cache, instance role, AWS_ACCESS_KEY_ID…), or from +// AWS_BEARER_TOKEN_BEDROCK if set. Region comes from AWS_REGION or the active +// profile. +func NewAnthropicBedrockClient(cfg ClientConfig) *AnthropicClient { + if cfg.Timeout <= 0 { + cfg.Timeout = 5 * time.Minute + } + if cfg.SessionKey == "" { + cfg.SessionKey = NewSessionKey() + } + + // cfg.URL is deliberately unused: bedrock.WithConfig is appended last and + // installs its own base URL from the resolved region, so anything set here + // would be overwritten rather than honoured. A custom endpoint (a VPC + // endpoint, say) would need to be threaded through the AWS config instead. + opts := []option.RequestOption{ + option.WithMaxRetries(5), + option.WithHeader("User-Agent", userAgent("claude")), + option.WithRequestTimeout(cfg.Timeout), + // Bedrock authenticates by SigV4 signature, added by the middleware + // below at transport time. Any API-key header the SDK would otherwise + // attach — including an empty one — is rejected outright with + // "Invalid API Key format: Must start with pre-defined prefix", so both + // are removed here, before signing. + option.WithHeaderDel("Authorization"), + option.WithHeaderDel("X-Api-Key"), + } + // ExtraHeaders are applied per request in CompletionsWithCtx, where the + // session key template can expand — same as the plain Anthropic client. + if mw := retryCodesMiddleware(cfg.RetryCodes); mw != nil { + opts = append(opts, option.WithMiddleware(mw)) + } + if cfg.retryCollector != nil { + opts = append(opts, option.WithMiddleware(newRetryObserver(cfg.retryCollector))) + } + + // Load the AWS config here rather than calling bedrock.WithLoadDefaultConfig, + // which panics on failure. + var loadOpts []func(*awsconfig.LoadOptions) error + if cfg.AWSProfile != "" { + loadOpts = append(loadOpts, awsconfig.WithSharedConfigProfile(cfg.AWSProfile)) + } + if cfg.AWSRegion != "" { + loadOpts = append(loadOpts, awsconfig.WithRegion(cfg.AWSRegion)) + } + loadCtx, cancel := context.WithTimeout(context.Background(), bedrockConfigLoadTimeout) + defer cancel() + awsCfg, err := awsconfig.LoadDefaultConfig(loadCtx, loadOpts...) + if err != nil { + return &AnthropicClient{ + cfg: cfg, + bedrock: true, + awsProfile: cfg.AWSProfile, + initErr: fmt.Errorf("bedrock: could not load AWS configuration: %w\n"+ + " bedrock uses the standard AWS credential chain — set AWS_PROFILE, or run `aws sso login%s`", err, ssoLoginProfileArg(cfg.AWSProfile)), + } + } + if awsCfg.Region == "" { + return &AnthropicClient{ + cfg: cfg, + bedrock: true, + awsProfile: cfg.AWSProfile, + initErr: fmt.Errorf("bedrock: no AWS region resolved\n" + + " set AWS_REGION, or give the active profile a region — the region decides which bedrock-runtime host is used"), + } + } + + // Drop the credential-chain bearer token, always. + // + // bedrock.WithConfig prefers bearer auth over SigV4 whenever + // cfg.BearerAuthTokenProvider is non-nil, and LoadDefaultConfig populates + // that provider from the SSO token cache — the OIDC access token, which is + // for identity services, not Bedrock. So an SSO-authenticated caller + // (i.e. most enterprise setups) silently sends `Authorization: Bearer + // ` and Bedrock answers 403 "Invalid API Key format: Must start + // with pre-defined prefix". + // + // Clearing it unconditionally is what gives AWS_BEARER_TOKEN_BEDROCK the + // precedence its documentation describes. WithConfig's doc comment says the + // variable wins, but the code only consults it when the provider is nil + // (bedrock.go: `if cfg.BearerAuthTokenProvider == nil`), so leaving an + // SSO-derived provider in place would make a deliberately configured Bedrock + // API key unreachable — the same silent substitution, with the user's real + // token discarded. Cleared here, WithConfig re-reads the variable and builds + // a static provider from it; unset, the SigV4 path runs. + awsCfg.BearerAuthTokenProvider = nil + + // Appended last on purpose, and the order depends on the SDK wrapping + // direction: each option wraps the ones before it, so the last appended + // middleware ends up innermost — signing runs closest to the wire, after any + // header the earlier options set, and a retry re-signs rather than replaying + // a stale signature. Moving this call earlier silently breaks both. + opts = append(opts, bedrock.WithConfig(awsCfg)) + + return &AnthropicClient{ + cfg: cfg, + sdk: anthropic.NewClient(opts...), + bedrock: true, + awsRegion: awsCfg.Region, + awsProfile: cfg.AWSProfile, + } +} + +// BedrockContext reports the AWS region and profile a Bedrock client resolved, +// so callers can show what a request actually used. ok is false for every other +// protocol. An empty profile means the ambient chain chose the credentials. +func (c *AnthropicClient) BedrockContext() (region, profile string, ok bool) { + if !c.bedrock { + return "", "", false + } + return c.awsRegion, c.awsProfile, true +} + +func ssoLoginProfileArg(profile string) string { + if profile == "" { + return "" + } + return " --profile " + profile +} + +// bedrockWhere describes the region and profile in one clause, for error text. +func (c *AnthropicClient) bedrockWhere() string { + region := c.awsRegion + if region == "" { + region = "unknown region" + } + if c.awsProfile == "" { + return fmt.Sprintf("region %s, credentials from the ambient AWS chain", region) + } + return fmt.Sprintf("region %s, profile %s", region, c.awsProfile) +} + +// explainError translates a Bedrock rejection into the action that fixes it. +// Two of these are actively misleading as the service words them: the API-key +// complaint has nothing to do with any api_key the user could configure, and a +// model that is merely absent from the region reads as a malformed identifier. +// Non-Bedrock clients are unaffected — the error is returned untouched. +func (c *AnthropicClient) explainError(model string, err error) error { + if err == nil || !c.bedrock { + return err + } + msg := err.Error() + where := c.bedrockWhere() + + // Order matters here, and the two AccessDenied shapes are why: Bedrock + // answers both "your IAM policy forbids this" and "this account has not + // enabled the model" with AccessDeniedException, and the fixes have nothing + // in common. The specific wording is matched before the generic code. + switch { + // First: the bearer-token path produces this even when credentials are + // otherwise valid, so a later "denied" branch would mislabel it. + case strings.Contains(msg, "Invalid API Key format"): + if os.Getenv("AWS_BEARER_TOKEN_BEDROCK") != "" { + return fmt.Errorf("bedrock rejected the token in AWS_BEARER_TOKEN_BEDROCK (%s): %w\n"+ + " unset that variable to sign requests with SigV4 instead", where, err) + } + return fmt.Errorf("bedrock rejected an API-key header rather than a signature (%s): %w\n"+ + " no api_key applies to bedrock; this means a bearer token reached the request, not that a key is missing", where, err) + case strings.Contains(msg, "don't have access to the model"): + return fmt.Errorf("bedrock has no access enabled for model %q (%s): %w\n"+ + " model access is granted per account and per region in the Bedrock console; an IAM policy alone does not enable it", model, where, err) + case strings.Contains(msg, "model identifier is invalid"), + strings.Contains(msg, "inference profile") && strings.Contains(msg, "not found"): + return fmt.Errorf("bedrock rejected model %q (%s): %w\n"+ + " run `aws bedrock list-inference-profiles%s` to see what this account offers — IDs are account- and region-scoped, and a version suffix such as -v1:0 is invalid for the newer families", + model, where, err, listProfilesRegionArg(c.awsRegion)) + // Specific credential codes only. A bare "expired" would also claim an + // expired TLS certificate is an SSO problem. + case strings.Contains(msg, "ExpiredToken"), strings.Contains(msg, "ExpiredTokenException"), + strings.Contains(msg, "SSOProviderInvalidToken"), strings.Contains(msg, "InvalidGrantException"), + strings.Contains(msg, "NoCredentialProviders"), strings.Contains(msg, "failed to refresh cached credentials"): + return fmt.Errorf("bedrock could not authenticate: AWS credentials are expired or unavailable (%s): %w\n"+ + " run `aws sso login%s`, or refresh whichever credential source this profile uses", where, err, ssoLoginProfileArg(c.awsProfile)) + // "not authorized to invoke this API operation" is IAM's own wording, so it + // belongs here rather than in the model-access branch above: the fix is a + // policy change, not a console toggle. + case strings.Contains(msg, "AccessDenied"), + strings.Contains(msg, "not authorized to invoke this API operation"): + return fmt.Errorf("bedrock denied access to model %q (%s): %w\n"+ + " credentials resolved, so this is an authorization gap: the identity needs bedrock:InvokeModel on this model in this region, and the account needs model access enabled for it", model, where, err) + } + // Everything else — ValidationException on max_tokens, a network reset, a + // throttle — keeps the service's own wording. Guessing at a cause here would + // send people after the wrong problem, which is the failure this function + // exists to prevent. + return fmt.Errorf("bedrock request failed (%s): %w", where, err) +} + +func listProfilesRegionArg(region string) string { + if region == "" { + return "" + } + return " --region " + region +} + // CompletionsWithCtx sends a chat completion request with context support. // // The deferred finalizeRequest is this client's boundary for the retry report; @@ -806,6 +1047,10 @@ func (c *AnthropicClient) CompletionsWithCtx(ctx context.Context, req ChatReques finalizeRequest(ctx, c.cfg.retryCollector, err) }() + if c.initErr != nil { + return nil, c.initErr + } + model := req.Model if model == "" { model = c.cfg.Model @@ -838,7 +1083,7 @@ func (c *AnthropicClient) CompletionsWithCtx(ctx context.Context, req ChatReques sdkResp, err := c.sdk.Messages.New(ctx, params, opts...) if err != nil { - return nil, err + return nil, c.explainError(model, err) } return c.mapAnthropicResponse(sdkResp), nil diff --git a/internal/llm/protocol.go b/internal/llm/protocol.go index 8a56b8619..04250e9cb 100644 --- a/internal/llm/protocol.go +++ b/internal/llm/protocol.go @@ -27,6 +27,14 @@ const ( // ProtocolOpenAIResponses is the OpenAI Responses API (/v1/responses), // used by GPT-5.x / o-series models. ProtocolOpenAIResponses = "openai-responses" + // ProtocolAnthropicBedrock is the Anthropic Messages API served by AWS + // Bedrock. The request body is the same as ProtocolAnthropic — the + // difference is transport: requests are SigV4-signed from the ambient AWS + // credential chain rather than carrying an API key, the model moves from + // the body into the URL path, and the region determines the host. The + // official SDK's bedrock middleware performs that rewriting, so this + // shares the Anthropic client rather than reimplementing the protocol. + ProtocolAnthropicBedrock = "anthropic-bedrock" ) // NormalizeProtocol canonicalizes protocol names. It is case-insensitive and @@ -45,18 +53,20 @@ func NormalizeProtocol(raw string) string { return ProtocolOpenAIChatCompletions case ProtocolOpenAIResponses: return ProtocolOpenAIResponses + case ProtocolAnthropicBedrock: + return ProtocolAnthropicBedrock default: return normalized } } -// ValidateProtocol accepts the three canonical protocol names and rejects +// ValidateProtocol accepts the four canonical protocol names and rejects // everything else. func ValidateProtocol(p string) error { switch p { - case ProtocolAnthropic, ProtocolOpenAIChatCompletions, ProtocolOpenAIResponses: + case ProtocolAnthropic, ProtocolOpenAIChatCompletions, ProtocolOpenAIResponses, ProtocolAnthropicBedrock: return nil default: - return fmt.Errorf("unsupported protocol %q; supported protocols are %q, %q, %q", p, ProtocolAnthropic, ProtocolOpenAIChatCompletions, ProtocolOpenAIResponses) + return fmt.Errorf("unsupported protocol %q; supported protocols are %q, %q, %q, %q", p, ProtocolAnthropic, ProtocolOpenAIChatCompletions, ProtocolOpenAIResponses, ProtocolAnthropicBedrock) } } diff --git a/internal/llm/providers.go b/internal/llm/providers.go index e02d0a276..c8941e1a0 100644 --- a/internal/llm/providers.go +++ b/internal/llm/providers.go @@ -14,6 +14,7 @@ import ( // - ProtocolAnthropic ("anthropic") // - ProtocolOpenAIChatCompletions ("openai") // - ProtocolOpenAIResponses ("openai-responses") +// - ProtocolAnthropicBedrock ("anthropic-bedrock") // // To add a built-in provider that speaks a different protocol, set Protocol // accordingly and ensure NewLLMClient has a matching case. @@ -25,6 +26,13 @@ type Provider struct { AuthHeader string // Anthropic-only; empty for OpenAI-compatible EnvVar string // environment variable name for API key fallback Models []string + + // AmbientAuth marks a provider whose credentials come from the + // environment's own chain rather than an api_key — AWS SigV4, for + // instance. The resolver skips its api_key requirement for these, because + // there is no key to configure and demanding one would make the provider + // impossible to use. + AmbientAuth bool } var registry = []Provider{ @@ -44,6 +52,34 @@ var registry = []Provider{ "claude-sonnet-4-6", }, }, + { + // Bedrock takes no api_key and no base URL: the SDK's bedrock + // middleware derives the host from the resolved AWS region and signs + // each request from the ambient credential chain (profile, SSO, + // instance role, or AWS_* variables). Set AWS_REGION or AWS_PROFILE the + // way any other AWS tool expects. + // + // Model accepts anything Bedrock will route: a foundation model ID, an + // inference profile ID, or the ARN of an application inference profile + // when usage needs to be attributed for cost allocation. Run + // `aws bedrock list-inference-profiles` to see what an account offers — + // IDs differ per account and per region, so the list below is only a + // starting point. + Name: "bedrock", + DisplayName: "AWS Bedrock (Anthropic models)", + Protocol: ProtocolAnthropicBedrock, + AmbientAuth: true, + Models: []string{ + "us.anthropic.claude-opus-5", + "us.anthropic.claude-sonnet-5", + "us.anthropic.claude-opus-4-8", + "us.anthropic.claude-opus-4-7", + "us.anthropic.claude-sonnet-4-6", + "global.anthropic.claude-opus-5", + "global.anthropic.claude-sonnet-5", + "global.anthropic.claude-opus-4-8", + }, + }, { Name: "openai", DisplayName: "OpenAI API", diff --git a/internal/llm/providers_test.go b/internal/llm/providers_test.go index 48389c36c..dd66c1765 100644 --- a/internal/llm/providers_test.go +++ b/internal/llm/providers_test.go @@ -75,7 +75,7 @@ func TestListProviders_Order(t *testing.T) { if len(providers) < 3 { t.Fatalf("expected at least 3 providers, got %d", len(providers)) } - expected := []string{"anthropic", "baidu-qianfan", "dashscope", "dashscope-tokenplan", "deepseek", "edenai", "gemini", "hy-tokenplan", "iflytek", "kimi", "kimi-global", "litellm", "mimo", "minimax", "minimax-cn", "mistral", "novita", "ollama-cloud", "openai", "siliconflow", "siliconflow-cn", "tencent-tokenhub", "volcengine", "xai", "z-ai", "z-ai-coding"} + expected := []string{"anthropic", "baidu-qianfan", "bedrock", "dashscope", "dashscope-tokenplan", "deepseek", "edenai", "gemini", "hy-tokenplan", "iflytek", "kimi", "kimi-global", "litellm", "mimo", "minimax", "minimax-cn", "mistral", "novita", "ollama-cloud", "openai", "siliconflow", "siliconflow-cn", "tencent-tokenhub", "volcengine", "xai", "z-ai", "z-ai-coding"} if len(providers) != len(expected) { t.Fatalf("expected %d providers, got %d", len(expected), len(providers)) } @@ -353,11 +353,14 @@ func TestLookupProvider_XAIDetails(t *testing.T) { // canonical protocol constant — no stale "openai" / "anthropic" literals that // would bypass NormalizeProtocol downstream. func TestProviders_AllProtocolsCanonical(t *testing.T) { + // Delegates to ValidateProtocol rather than re-listing the canonical names, + // so adding a protocol does not silently leave this assertion behind. for _, p := range ListProviders() { - switch p.Protocol { - case ProtocolAnthropic, ProtocolOpenAIChatCompletions, ProtocolOpenAIResponses: - default: - t.Errorf("provider %q has non-canonical Protocol %q", p.Name, p.Protocol) + if NormalizeProtocol(p.Protocol) != p.Protocol { + t.Errorf("provider %q Protocol %q is not in canonical form", p.Name, p.Protocol) + } + if err := ValidateProtocol(p.Protocol); err != nil { + t.Errorf("provider %q has non-canonical Protocol %q: %v", p.Name, p.Protocol, err) } } } diff --git a/internal/llm/resolver.go b/internal/llm/resolver.go index 85b083420..3b0654020 100644 --- a/internal/llm/resolver.go +++ b/internal/llm/resolver.go @@ -32,6 +32,18 @@ type ResolvedEndpoint struct { // knob; users can still override via OCR_LLM_TIMEOUT. Timeout time.Duration RetryCodes []int // additional HTTP status codes that trigger exponential-backoff retry + + // AmbientAuth marks an endpoint that carries no token and needs no base + // URL, because the transport supplies both — AWS SigV4 signing derives the + // host from the region and the credentials from the environment's own + // chain. Completeness checks must treat an empty URL and Token as valid for + // these; requiring either would reject a correctly configured endpoint. + AmbientAuth bool + + // AWSProfile and AWSRegion override the ambient AWS chain for SigV4 + // providers. Empty means "let the AWS SDK decide". + AWSProfile string + AWSRegion string } // Environment variable names for OCR-specific configuration. @@ -126,7 +138,10 @@ func ResolveEndpointWithOptions(configPath string, opts ResolveOptions) (Resolve if err != nil { return ResolvedEndpoint{}, fmt.Errorf("resolve %s: %w", strategy.name, err) } - if ok && ep.URL != "" && ep.Token != "" && ep.Model != "" { + // An ambient-auth endpoint is complete without a URL or token: the + // transport supplies both. Everything else still needs all three. + complete := ep.Model != "" && (ep.AmbientAuth || (ep.URL != "" && ep.Token != "")) + if ok && complete { return finalizeResolvedEndpoint(strategy.name, ep, env), nil } } @@ -218,6 +233,16 @@ func validateTimeoutSec(sec int) (time.Duration, error) { return time.Duration(sec) * time.Second, nil } +// errBedrockNotConfigurable explains why the two url+token strategies reject the +// bedrock protocol. Both describe a single HTTP endpoint and carry no place for +// a region or a profile, and bedrock uses neither the url nor the token they do +// carry. Accepting the value would switch transports and silently ignore the +// rest of the block, so it is refused at the point it is read. +func errBedrockNotConfigurable(key string) error { + return fmt.Errorf("%s cannot be %q: bedrock derives its host from aws_region and signs with the AWS credential chain, so it has no use for a url or a token; configure it as a provider instead (\"provider\": \"bedrock\")", + key, ProtocolAnthropicBedrock) +} + // tryOCREnv reads OCR-specific environment variables. func tryOCREnv(modelOverride string) (ResolvedEndpoint, bool, error) { url := os.Getenv(envOCRLLMURL) @@ -237,6 +262,9 @@ func tryOCREnv(modelOverride string) (ResolvedEndpoint, bool, error) { if err := ValidateProtocol(protocol); err != nil { return ResolvedEndpoint{}, false, fmt.Errorf("OCR environment: %w", err) } + if protocol == ProtocolAnthropicBedrock { + return ResolvedEndpoint{}, false, fmt.Errorf("OCR environment: %w", errBedrockNotConfigurable(envOCRLLMProtocol)) + } } if protocol == "" { useAnthropic := true // default true @@ -294,6 +322,13 @@ type providerEntryConfig struct { ExtraBody map[string]any `json:"extra_body,omitempty"` ExtraHeaders map[string]string `json:"extra_headers,omitempty"` RetryCodes []int `json:"retry_codes,omitempty"` + + // AWSProfile and AWSRegion apply to ambient-auth providers that sign with + // SigV4 (currently bedrock). Both are optional: without them the standard + // AWS chain decides, same as any other AWS tool. Setting them in config + // makes a review run reproducible without exporting AWS_PROFILE first. + AWSProfile string `json:"aws_profile,omitempty"` + AWSRegion string `json:"aws_region,omitempty"` } type configFile struct { @@ -388,12 +423,6 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, apiKey = v } } - // No credential at all is still an error here, before any other validation: - // only the command's *execution* is deferred, not the emptiness check. - if apiKey == "" && apiKeyCmd == "" { - return ResolvedEndpoint{}, false, fmt.Errorf("provider %q has no api_key or api_key_cmd configured and no environment variable fallback found", cfg.Provider) - } - var url, protocol, authHeader, model string var extraBody map[string]any @@ -415,18 +444,44 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, protocol = normalized } } else { - // Custom provider: url and protocol are required; model can come from cfg.Model. - if entry.URL == "" || entry.Protocol == "" { - return ResolvedEndpoint{}, false, fmt.Errorf("custom provider %q requires url and protocol fields", cfg.Provider) + // Custom provider: protocol is always required; model can come from + // cfg.Model. url is required for every protocol that names an HTTP + // endpoint, which is all of them except bedrock — there the region + // decides the host, so demanding a url would mean storing a value the + // client never reads. + if entry.Protocol == "" { + return ResolvedEndpoint{}, false, fmt.Errorf("custom provider %q requires a protocol field", cfg.Provider) } normalized := NormalizeProtocol(entry.Protocol) if err := ValidateProtocol(normalized); err != nil { return ResolvedEndpoint{}, false, fmt.Errorf("custom provider %q: %w", cfg.Provider, err) } + if normalized != ProtocolAnthropicBedrock && entry.URL == "" { + return ResolvedEndpoint{}, false, fmt.Errorf("custom provider %q requires a url field for protocol %q", cfg.Provider, normalized) + } url = entry.URL protocol = normalized } + // Ambient auth follows the protocol actually in force, which is why this is + // resolved after the override above rather than read off the preset. A preset + // declares ambient auth (AmbientAuth), but an entry may override the preset's + // protocol: a bedrock preset switched to "openai" speaks a protocol with no + // SigV4 signing and needs a token like anything else. Conversely an entry + // that selects the bedrock protocol explicitly signs its requests whatever + // the preset says. + ambientAuth := protocol == ProtocolAnthropicBedrock || + (isPreset && preset.AmbientAuth && entry.Protocol == "") + + // No credential at all is an error, and it is reported before api_key_cmd + // runs: only the command's *execution* is deferred, not the emptiness check. + // An ambient-auth provider is the exception — it has no key to configure, + // since credentials come from the environment's own chain and the request is + // signed rather than bearing a token. + if apiKey == "" && apiKeyCmd == "" && !ambientAuth { + return ResolvedEndpoint{}, false, fmt.Errorf("provider %q has no api_key or api_key_cmd configured and no environment variable fallback found", cfg.Provider) + } + if cfg.Model != "" { model = cfg.Model } @@ -441,9 +496,17 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, } availableModels = append(availableModels, entry.Models...) + // A preset's Models list doubles as an allowlist for --model. For an + // ambient-auth provider it cannot: Bedrock identifiers are scoped to an + // account and a region, and an application inference profile ARN — a + // supported value, and the one to use when spend has to be attributed — can + // never appear in a list compiled upstream. The list stays a picker for + // `ocr config model`; it does not gate an override. + gateOverrideOnModelList := !ambientAuth + // Apply model override with validation. if modelOverride != "" { - if len(availableModels) > 0 { + if gateOverrideOnModelList && len(availableModels) > 0 { if !ModelListContains(availableModels, modelOverride) { return ResolvedEndpoint{}, false, fmt.Errorf( "model %q is not available for provider %q; available models: %s", @@ -499,9 +562,11 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, // Single api_key_cmd resolution site for both preset and custom providers, // as late as possible: everything above can fail without running the - // command. apiKey is empty here only when api_key_cmd is set (guaranteed by - // the emptiness check above), and a failing command is a hard error. - if apiKey == "" { + // command. An ambient-auth provider skips it entirely — the request is + // signed, so the command's output would be discarded, and running it anyway + // means a real 1Password / Touch ID prompt for a value nothing consumes. + // For everyone else a failing command is a hard error. + if apiKey == "" && apiKeyCmd != "" && !ambientAuth { resolved, err := resolveKeyCmd(apiKeyCmd, fmt.Sprintf("api_key_cmd for provider %q", cfg.Provider)) if err != nil { return ResolvedEndpoint{}, false, err @@ -521,6 +586,9 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, ExtraHeaders: extraHeaders, Timeout: timeout, RetryCodes: retryCodes, + AmbientAuth: ambientAuth, + AWSProfile: entry.AWSProfile, + AWSRegion: entry.AWSRegion, }, true, nil } @@ -562,6 +630,9 @@ func tryLegacyLlmConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, if err := ValidateProtocol(protocol); err != nil { return ResolvedEndpoint{}, false, fmt.Errorf("OCR config file: %w", err) } + if protocol == ProtocolAnthropicBedrock { + return ResolvedEndpoint{}, false, fmt.Errorf("OCR config file: %w", errBedrockNotConfigurable("llm.protocol")) + } } if protocol == "" { useAnthropic := true // default true diff --git a/pages/src/content/docs/en/configuration.md b/pages/src/content/docs/en/configuration.md index 7ea1f0bc5..022988ab4 100644 --- a/pages/src/content/docs/en/configuration.md +++ b/pages/src/content/docs/en/configuration.md @@ -45,6 +45,7 @@ environment variable. | Name | Protocol | Base URL | API key env var | |---|---|---|---| | `anthropic` | anthropic | `https://api.anthropic.com` | `ANTHROPIC_API_KEY` | +| `bedrock` | anthropic-bedrock | derived from `aws_region` | — (AWS credential chain) | | `openai` | openai | `https://api.openai.com/v1` | `OPENAI_API_KEY` | | `gemini` | openai | `https://generativelanguage.googleapis.com/v1beta/openai` | `GEMINI_API_KEY` | | `dashscope` | openai | `https://dashscope.aliyuncs.com/compatible-mode/v1` | `DASHSCOPE_API_KEY` | @@ -84,11 +85,58 @@ The configured `url` takes precedence over the preset Base URL. When `providers..url` is unset (or cleared), OCR falls back to the preset default — so you only need to set it when your endpoint differs. +### AWS Bedrock + +`bedrock` speaks the same Messages API as `anthropic`, but requests are +SigV4-signed from the standard AWS credential chain instead of carrying an API +key, and the region decides the host. There is no `api_key` to set, and none is +accepted as a substitute for a signature: + +```bash +ocr config set provider bedrock +ocr config set model us.anthropic.claude-sonnet-4-6 +ocr config set providers.bedrock.aws_region us-west-2 +ocr config set providers.bedrock.aws_profile example-profile +``` + +| Field | Meaning | +|---|---| +| `providers.bedrock.aws_region` | Region whose `bedrock-runtime` host serves the request. Falls back to `AWS_REGION` or the active profile. | +| `providers.bedrock.aws_profile` | Named profile to resolve credentials from. Falls back to `AWS_PROFILE` or the ambient chain. | + +Both fields are optional: left unset, the standard chain decides, as with any +other AWS tool. Pinning them makes a run reproducible without exporting +`AWS_PROFILE` first, which matters most on CI runners carrying a different +default. + +Model identifiers are scoped to an account **and** a region, so the list OCR +ships is a starting point rather than a closed set — an inference profile ID or +an application inference profile ARN valid for your account is accepted even +when absent from it. Run `aws bedrock list-inference-profiles --region ` +to see what an account offers; a version suffix such as `-v1:0` is invalid for +the newer families. + +`ocr llm test` reports the region and profile in place of a URL, because bedrock +has no configured URL — the region decides the host: + +``` +Source: provider:bedrock +Region: us-east-1 +Profile: example-profile +Model: claude-sonnet-5 +✓ Connection test successful +``` + +Bedrock is **not** available through `llm.protocol` or `OCR_LLM_PROTOCOL`. That +block describes one URL and one token, has nowhere to put a region or a profile, +and bedrock uses neither value it does carry, so the combination is rejected +rather than accepted and ignored. + ### Custom providers Any provider name not in the table above is treated as custom and must supply at least `url` and `protocol` (`protocol` is `anthropic`, -`openai`, or `openai-responses`): +`openai`, `openai-responses`, or `anthropic-bedrock`): ```bash ocr config set provider my-gateway @@ -109,6 +157,18 @@ ocr config set custom_providers.openai-responses-gateway.model gpt-5 ocr config set custom_providers.openai-responses-gateway.api_key "$OPENAI_API_KEY" ``` +A custom provider on the `anthropic-bedrock` protocol needs no `url` — the +region decides the host — and takes the same AWS fields as the built-in. This is +how a second region or profile gets its own entry: + +```bash +ocr config set provider bedrock-eu +ocr config set custom_providers.bedrock-eu.protocol anthropic-bedrock +ocr config set custom_providers.bedrock-eu.aws_region eu-west-1 +ocr config set custom_providers.bedrock-eu.aws_profile eu-profile +ocr config set custom_providers.bedrock-eu.model eu.anthropic.claude-sonnet-4-6 +``` + The `url` can be either the API base URL or the full `/responses` endpoint — OCR normalizes it either way. A local model served by Ollama is just a custom provider pointing at the diff --git a/pages/src/content/docs/ja/configuration.md b/pages/src/content/docs/ja/configuration.md index e62909962..f43040dbf 100644 --- a/pages/src/content/docs/ja/configuration.md +++ b/pages/src/content/docs/ja/configuration.md @@ -43,6 +43,7 @@ ocr config set providers.anthropic.api_key sk-ant-xxxxxxxxxx | 名称 | プロトコル | Base URL | API key 環境変数 | |---|---|---|---| | `anthropic` | anthropic | `https://api.anthropic.com` | `ANTHROPIC_API_KEY` | +| `bedrock` | anthropic-bedrock | `aws_region` から決定 | —（AWS 認証情報チェーン） | | `openai` | openai | `https://api.openai.com/v1` | `OPENAI_API_KEY` | | `gemini` | openai | `https://generativelanguage.googleapis.com/v1beta/openai` | `GEMINI_API_KEY` | | `dashscope` | openai | `https://dashscope.aliyuncs.com/compatible-mode/v1` | `DASHSCOPE_API_KEY` | @@ -82,11 +83,57 @@ ocr config set providers.litellm.url https://gateway.internal:8000/v1 `providers..url` が未設定（または削除）の場合、OCR はプリセット デフォルトにフォールバックします——エンドポイントが異なる場合のみ設定すればよいです。 +### AWS Bedrock + +`bedrock` は `anthropic` と同じ Messages API を話しますが、API key を持たせる +代わりに標準の AWS 認証情報チェーンから SigV4 署名を行い、ホストはリージョンが +決めます。設定すべき `api_key` はなく、署名の代わりとして受け付けることも +ありません。 + +```bash +ocr config set provider bedrock +ocr config set model us.anthropic.claude-sonnet-4-6 +ocr config set providers.bedrock.aws_region us-west-2 +ocr config set providers.bedrock.aws_profile example-profile +``` + +| フィールド | 意味 | +|---|---| +| `providers.bedrock.aws_region` | リクエストを処理する `bedrock-runtime` ホストのリージョン。未設定なら `AWS_REGION` または有効なプロファイルにフォールバックします。 | +| `providers.bedrock.aws_profile` | 認証情報を解決する名前付きプロファイル。未設定なら `AWS_PROFILE` または周囲のチェーンにフォールバックします。 | + +どちらも任意です。未設定なら他の AWS ツールと同様に標準チェーンが決めます。 +明示的に固定しておくと、先に `AWS_PROFILE` をエクスポートしなくても実行を +再現でき、既定値が異なる CI ランナーで特に効きます。 + +モデル識別子はアカウント**および**リージョンにスコープされるため、OCR が同梱 +する一覧は出発点であって閉じた集合ではありません。アカウントで有効な推論 +プロファイル ID やアプリケーション推論プロファイル ARN は、一覧になくても +受け付けられます。`aws bedrock list-inference-profiles --region ` で +そのアカウントが提供するものを確認できます。なお新しいファミリーでは +`-v1:0` のようなバージョンサフィックスは無効です。 + +bedrock には設定された URL がなく、ホストはリージョンが決めるため、 +`ocr llm test` は URL の代わりにリージョンとプロファイルを表示します。 + +``` +Source: provider:bedrock +Region: us-east-1 +Profile: example-profile +Model: claude-sonnet-5 +✓ Connection test successful +``` + +`llm.protocol` および `OCR_LLM_PROTOCOL` では bedrock を選べ**ません**。この +ブロックは URL とトークンを 1 つずつ記述するもので、リージョンやプロファイルを +置く場所がなく、bedrock はそこにある値をどちらも使いません。そのため黙って +無視するのではなく、明示的に拒否されます。 + ### カスタム provider 上記の表にない provider 名はすべてカスタムとみなされ、少なくとも `url` と `protocol` を指定する必要があります（`protocol` は `anthropic`、`openai`、 -または `openai-responses`）。 +`openai-responses`、または `anthropic-bedrock`）。 ```bash ocr config set provider my-gateway @@ -107,6 +154,18 @@ ocr config set custom_providers.openai-responses-gateway.model gpt-5 ocr config set custom_providers.openai-responses-gateway.api_key "$OPENAI_API_KEY" ``` +`anthropic-bedrock` プロトコルのカスタム provider に `url` は不要です（ホストは +リージョンが決めます）。組み込みと同じ AWS フィールドを取れるので、2 つめの +リージョンやプロファイルを別エントリとして持たせられます。 + +```bash +ocr config set provider bedrock-eu +ocr config set custom_providers.bedrock-eu.protocol anthropic-bedrock +ocr config set custom_providers.bedrock-eu.aws_region eu-west-1 +ocr config set custom_providers.bedrock-eu.aws_profile eu-profile +ocr config set custom_providers.bedrock-eu.model eu.anthropic.claude-sonnet-4-6 +``` + `url` には API の Base URL または完全な `/responses` エンドポイントのどちらを指定してもよく、OCR がどちらの形式も正規化します。 Ollama で動かすローカルモデルは、ローカルの OpenAI 互換エンドポイントを diff --git a/pages/src/content/docs/ru/configuration.md b/pages/src/content/docs/ru/configuration.md index 7ad074d3b..46dc86d56 100644 --- a/pages/src/content/docs/ru/configuration.md +++ b/pages/src/content/docs/ru/configuration.md @@ -48,6 +48,7 @@ API-ключ. Если `providers..api_key` не задан, OCR испо | Имя | Протокол | Базовый URL | Переменная окружения для API-ключа | |---|---|---|---| | `anthropic` | anthropic | `https://api.anthropic.com` | `ANTHROPIC_API_KEY` | +| `bedrock` | anthropic-bedrock | определяется `aws_region` | — (цепочка учётных данных AWS) | | `openai` | openai | `https://api.openai.com/v1` | `OPENAI_API_KEY` | | `gemini` | openai | `https://generativelanguage.googleapis.com/v1beta/openai` | `GEMINI_API_KEY` | | `dashscope` | openai | `https://dashscope.aliyuncs.com/compatible-mode/v1` | `DASHSCOPE_API_KEY` | @@ -89,12 +90,59 @@ ocr config set providers.litellm.url https://gateway.internal:8000/v1 предустановленному значению по умолчанию — поэтому его нужно задавать только когда ваша конечная точка отличается. +### AWS Bedrock + +`bedrock` использует тот же Messages API, что и `anthropic`, но запросы +подписываются по SigV4 из стандартной цепочки учётных данных AWS вместо +передачи API-ключа, а хост определяется регионом. Задавать `api_key` не нужно, +и он не принимается как замена подписи: + +```bash +ocr config set provider bedrock +ocr config set model us.anthropic.claude-sonnet-4-6 +ocr config set providers.bedrock.aws_region us-west-2 +ocr config set providers.bedrock.aws_profile example-profile +``` + +| Поле | Значение | +|---|---| +| `providers.bedrock.aws_region` | Регион, чей хост `bedrock-runtime` обслуживает запрос. По умолчанию — `AWS_REGION` или активный профиль. | +| `providers.bedrock.aws_profile` | Именованный профиль для получения учётных данных. По умолчанию — `AWS_PROFILE` или окружающая цепочка. | + +Оба поля необязательны: если они не заданы, выбор делает стандартная цепочка, +как и для любого другого инструмента AWS. Явная фиксация делает запуск +воспроизводимым без предварительного экспорта `AWS_PROFILE` — это особенно +важно на CI-раннерах с другим значением по умолчанию. + +Идентификаторы моделей привязаны к аккаунту **и** к региону, поэтому список, +который поставляется с OCR, — отправная точка, а не закрытый набор: подходящий +для вашего аккаунта ID inference-профиля или ARN прикладного inference-профиля +принимается, даже если его нет в списке. Выполните +`aws bedrock list-inference-profiles --region `, чтобы увидеть, что +доступно в аккаунте; суффикс версии вроде `-v1:0` недопустим для новых семейств. + +`ocr llm test` показывает регион и профиль вместо URL, потому что у bedrock нет +настроенного URL — хост определяется регионом: + +``` +Source: provider:bedrock +Region: us-east-1 +Profile: example-profile +Model: claude-sonnet-5 +✓ Connection test successful +``` + +Через `llm.protocol` и `OCR_LLM_PROTOCOL` bedrock **недоступен**. Этот блок +описывает один URL и один токен, в нём негде указать регион или профиль, а сами +эти значения bedrock не использует, поэтому такая комбинация отклоняется, а не +принимается и молча игнорируется. + ### Пользовательские провайдеры Любое имя провайдера, которого нет в таблице выше, считается пользовательским. Для него необходимо задать как минимум `url` и `protocol` -(`protocol` может принимать значения `anthropic`, `openai` или -`openai-responses`): +(`protocol` может принимать значения `anthropic`, `openai`, +`openai-responses` или `anthropic-bedrock`): ```bash ocr config set provider my-gateway @@ -115,6 +163,18 @@ ocr config set custom_providers.openai-responses-gateway.model gpt-5 ocr config set custom_providers.openai-responses-gateway.api_key "$OPENAI_API_KEY" ``` +Пользовательскому провайдеру на протоколе `anthropic-bedrock` не нужен `url` — +хост определяется регионом, — и он принимает те же поля AWS, что и встроенный. +Так второй регион или профиль получает собственную запись: + +```bash +ocr config set provider bedrock-eu +ocr config set custom_providers.bedrock-eu.protocol anthropic-bedrock +ocr config set custom_providers.bedrock-eu.aws_region eu-west-1 +ocr config set custom_providers.bedrock-eu.aws_profile eu-profile +ocr config set custom_providers.bedrock-eu.model eu.anthropic.claude-sonnet-4-6 +``` + В качестве `url` можно указать как базовый URL API, так и полный эндпоинт `/responses` — OCR нормализует оба варианта. diff --git a/pages/src/content/docs/zh/configuration.md b/pages/src/content/docs/zh/configuration.md index 1aeae918b..c620f0fb6 100644 --- a/pages/src/content/docs/zh/configuration.md +++ b/pages/src/content/docs/zh/configuration.md @@ -42,6 +42,7 @@ ocr config set providers.anthropic.api_key sk-ant-xxxxxxxxxx | 名称 | 协议 | Base URL | API key 环境变量 | |---|---|---|---| | `anthropic` | anthropic | `https://api.anthropic.com` | `ANTHROPIC_API_KEY` | +| `bedrock` | anthropic-bedrock | 由 `aws_region` 决定 | —（AWS 凭证链） | | `openai` | openai | `https://api.openai.com/v1` | `OPENAI_API_KEY` | | `gemini` | openai | `https://generativelanguage.googleapis.com/v1beta/openai` | `GEMINI_API_KEY` | | `dashscope` | openai | `https://dashscope.aliyuncs.com/compatible-mode/v1` | `DASHSCOPE_API_KEY` | @@ -79,10 +80,53 @@ ocr config set providers.litellm.url https://gateway.internal:8000/v1 配置的 `url` 优先于预设 Base URL。当 `providers..url` 未设置（或 被清除）时，OCR 回退到预设默认值——因此只需在端点不同时才设置。 +### AWS Bedrock + +`bedrock` 使用与 `anthropic` 相同的 Messages API，但请求不携带 API key，而是 +用标准 AWS 凭证链做 SigV4 签名，主机由区域决定。没有 `api_key` 需要设置，也不 +接受用它替代签名： + +```bash +ocr config set provider bedrock +ocr config set model us.anthropic.claude-sonnet-4-6 +ocr config set providers.bedrock.aws_region us-west-2 +ocr config set providers.bedrock.aws_profile example-profile +``` + +| 字段 | 含义 | +|---|---| +| `providers.bedrock.aws_region` | 处理请求的 `bedrock-runtime` 主机所在区域。未设置时回退到 `AWS_REGION` 或当前 profile。 | +| `providers.bedrock.aws_profile` | 解析凭证所用的具名 profile。未设置时回退到 `AWS_PROFILE` 或环境中的凭证链。 | + +两者都是可选的：不设置时由标准凭证链决定，与其他 AWS 工具一致。显式固定可以 +让运行结果可复现，无需先导出 `AWS_PROFILE`——在默认值不同的 CI runner 上尤为 +重要。 + +模型标识符同时受账号**和**区域限制，因此 OCR 内置的列表只是起点，而非封闭集合： +只要在你的账号中有效，推理配置文件 ID 或应用推理配置文件 ARN 即使不在列表中也 +会被接受。运行 `aws bedrock list-inference-profiles --region ` 可以查看 +账号提供了哪些；注意新系列不接受 `-v1:0` 这类版本后缀。 + +bedrock 没有配置的 URL——主机由区域决定——所以 `ocr llm test` 显示区域和 profile +而不是 URL： + +``` +Source: provider:bedrock +Region: us-east-1 +Profile: example-profile +Model: claude-sonnet-5 +✓ Connection test successful +``` + +`llm.protocol` 和 `OCR_LLM_PROTOCOL` **不支持** bedrock。该配置块描述的是一个 +URL 加一个 token，没有地方放区域或 profile，而 bedrock 这两个值都不使用，因此 +会被明确拒绝，而不是接受后悄悄忽略。 + ### 自定义 provider 任何不在上表中的 provider 名都视为自定义，至少要提供 `url` 和 `protocol` -（`protocol` 取 `anthropic`、`openai` 或 `openai-responses`）： +（`protocol` 取 `anthropic`、`openai`、`openai-responses` 或 +`anthropic-bedrock`）： ```bash ocr config set provider my-gateway @@ -103,6 +147,18 @@ ocr config set custom_providers.openai-responses-gateway.model gpt-5 ocr config set custom_providers.openai-responses-gateway.api_key "$OPENAI_API_KEY" ``` +使用 `anthropic-bedrock` 协议的自定义 provider 不需要 `url`——主机由区域决定—— +并且可以使用与内置 provider 相同的 AWS 字段。第二个区域或 profile 就是这样拥有 +自己的条目的： + +```bash +ocr config set provider bedrock-eu +ocr config set custom_providers.bedrock-eu.protocol anthropic-bedrock +ocr config set custom_providers.bedrock-eu.aws_region eu-west-1 +ocr config set custom_providers.bedrock-eu.aws_profile eu-profile +ocr config set custom_providers.bedrock-eu.model eu.anthropic.claude-sonnet-4-6 +``` + `url` 既可以填 API 的 Base URL，也可以填完整的 `/responses` 端点，OCR 会自动归一化处理。 用 Ollama 跑本地模型，就是一个指向本地 OpenAI 兼容端点的自定义 provider：