From 32638622fe29bb60e36745f9710a7c9f42cc8508 Mon Sep 17 00:00:00 2001 From: Luis Rodriguez Date: Mon, 3 Aug 2026 09:39:53 -0400 Subject: [PATCH 1/7] feat(providers): add AWS Bedrock as a built-in provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bedrock serves the same Messages API as api.anthropic.com, so this reuses AnthropicClient wholesale and lets the official SDK's bedrock middleware handle 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 new protocol implementation, no AWS request plumbing. Configuration is an empty provider entry — there is no api_key to set: { "provider": "bedrock", "model": "us.anthropic.claude-sonnet-4-6", "providers": { "bedrock": { "aws_profile": "...", "aws_region": "..." } } } aws_profile and aws_region are optional; without them the standard AWS chain decides, as with any other AWS tool. Setting them makes a run reproducible without exporting AWS_PROFILE first. Model accepts a foundation model ID, an inference profile ID, or an application inference profile ARN when usage has to be attributed for cost allocation. Four things this needed beyond registering a provider, each found by running it rather than reading it: - The resolver required a non-empty api_key, and separately required both URL and Token to consider an endpoint complete. Bedrock has none of the three, so a correct config fell through every strategy and reported "no valid LLM endpoint configured" — the error for having configured nothing. Both gates now recognise ambient authentication, via an AmbientAuth flag on Provider and ResolvedEndpoint. Providers that do use api_key are unaffected, which TestNonAmbientProviderStillRequiresAPIKey pins. - bedrock.WithConfig prefers bearer auth over SigV4 whenever cfg.BearerAuthTokenProvider is non-nil, and LoadDefaultConfig populates that provider from the SSO token cache. An SSO-authenticated caller — most enterprise setups — therefore sent its OIDC access token and got 403 "Invalid API Key format: Must start with pre-defined prefix". The provider is cleared unless AWS_BEARER_TOKEN_BEDROCK was set deliberately, which restores SigV4 while leaving an explicit bearer token working. - The SDK would also attach an API-key header of its own, which Bedrock rejects even when empty. Authorization and X-Api-Key are removed before the signing middleware runs. - bedrock.WithLoadDefaultConfig panics when AWS config cannot be loaded. A CLI should not answer an expired session with a stack trace, so the config is loaded directly and the failure deferred to the first request as a sentence naming the likely fix. The preset's Models list is taken verbatim from `aws bedrock list-inference-profiles` on a live account rather than inferred: suffix conventions vary per family, so us.anthropic.claude-sonnet-5 is correct while us.anthropic.claude-sonnet-5-v1:0 is rejected with 400 "The provided model identifier is invalid." The global.* cross-region variants are listed alongside us.* since either is a valid routing target. That list only gates --model overrides; an application inference profile ARN still works via the model field. Two existing tests needed updating: the provider-order list gains "bedrock", and TestProviders_AllProtocolsCanonical now delegates to ValidateProtocol instead of re-listing the canonical names, so the next protocol added cannot silently leave it behind. Verified end-to-end against a live Bedrock account: reviews complete and return findings using SigV4 credentials from an SSO profile, with no AWS variables in the environment. Co-Authored-By: Claude Opus 5 (1M context) --- go.mod | 14 +++ go.sum | 28 ++++++ internal/llm/bedrock_test.go | 165 +++++++++++++++++++++++++++++++++ internal/llm/client.go | 119 ++++++++++++++++++++++++ internal/llm/protocol.go | 14 ++- internal/llm/providers.go | 36 +++++++ internal/llm/providers_test.go | 13 ++- internal/llm/resolver.go | 41 ++++++-- 8 files changed, 417 insertions(+), 13 deletions(-) create mode 100644 internal/llm/bedrock_test.go diff --git a/go.mod b/go.mod index 41382357..58ae224d 100644 --- a/go.mod +++ b/go.mod @@ -7,6 +7,7 @@ require ( charm.land/bubbletea/v2 v2.0.7 charm.land/lipgloss/v2 v2.0.4 github.com/anthropics/anthropic-sdk-go v1.55.1 + github.com/aws/aws-sdk-go-v2/config v1.27.27 github.com/bmatcuk/doublestar/v4 v4.10.0 github.com/google/uuid v1.6.0 github.com/modelcontextprotocol/go-sdk v1.6.1 @@ -29,6 +30,19 @@ require ( require ( github.com/atotto/clipboard v0.1.4 // indirect + github.com/aws/aws-sdk-go-v2 v1.30.3 // indirect + github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.6.3 // indirect + github.com/aws/aws-sdk-go-v2/credentials v1.17.27 // indirect + github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.11 // indirect + github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.15 // indirect + github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.15 // indirect + github.com/aws/aws-sdk-go-v2/internal/ini v1.8.0 // indirect + github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.3 // indirect + github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.17 // indirect + github.com/aws/aws-sdk-go-v2/service/sso v1.22.4 // indirect + github.com/aws/aws-sdk-go-v2/service/ssooidc v1.26.4 // indirect + github.com/aws/aws-sdk-go-v2/service/sts v1.30.3 // indirect + github.com/aws/smithy-go v1.20.3 // 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 b7de3d7c..1335eb6b 100644 --- a/go.sum +++ b/go.sum @@ -8,6 +8,34 @@ github.com/anthropics/anthropic-sdk-go v1.55.1 h1:GxukHUVou6AFIngxa/Aw1z79hmwg13 github.com/anthropics/anthropic-sdk-go v1.55.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.30.3 h1:jUeBtG0Ih+ZIFH0F4UkmL9w3cSpaMv9tYYDbzILP8dY= +github.com/aws/aws-sdk-go-v2 v1.30.3/go.mod h1:nIQjQVp5sfpQcTc9mPSr1B0PaWK5ByX9MOoDadSN4lc= +github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.6.3 h1:tW1/Rkad38LA15X4UQtjXZXNKsCgkshC3EbmcUmghTg= +github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.6.3/go.mod h1:UbnqO+zjqk3uIt9yCACHJ9IVNhyhOCnYk8yA19SAWrM= +github.com/aws/aws-sdk-go-v2/config v1.27.27 h1:HdqgGt1OAP0HkEDDShEl0oSYa9ZZBSOmKpdpsDMdO90= +github.com/aws/aws-sdk-go-v2/config v1.27.27/go.mod h1:MVYamCg76dFNINkZFu4n4RjDixhVr51HLj4ErWzrVwg= +github.com/aws/aws-sdk-go-v2/credentials v1.17.27 h1:2raNba6gr2IfA0eqqiP2XiQ0UVOpGPgDSi0I9iAP+UI= +github.com/aws/aws-sdk-go-v2/credentials v1.17.27/go.mod h1:gniiwbGahQByxan6YjQUMcW4Aov6bLC3m+evgcoN4r4= +github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.11 h1:KreluoV8FZDEtI6Co2xuNk/UqI9iwMrOx/87PBNIKqw= +github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.11/go.mod h1:SeSUYBLsMYFoRvHE0Tjvn7kbxaUhl75CJi1sbfhMxkU= +github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.15 h1:SoNJ4RlFEQEbtDcCEt+QG56MY4fm4W8rYirAmq+/DdU= +github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.15/go.mod h1:U9ke74k1n2bf+RIgoX1SXFed1HLs51OgUSs+Ph0KJP8= +github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.15 h1:C6WHdGnTDIYETAm5iErQUiVNsclNx9qbJVPIt03B6bI= +github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.15/go.mod h1:ZQLZqhcu+JhSrA9/NXRm8SkDvsycE+JkV3WGY41e+IM= +github.com/aws/aws-sdk-go-v2/internal/ini v1.8.0 h1:hT8rVHwugYE2lEfdFE0QWVo81lF7jMrYJVDWI+f+VxU= +github.com/aws/aws-sdk-go-v2/internal/ini v1.8.0/go.mod h1:8tu/lYfQfFe6IGnaOdrpVgEL2IrrDOf6/m9RQum4NkY= +github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.3 h1:dT3MqvGhSoaIhRseqw2I0yH81l7wiR2vjs57O51EAm8= +github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.3/go.mod h1:GlAeCkHwugxdHaueRr4nhPuY+WW+gR8UjlcqzPr1SPI= +github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.17 h1:HGErhhrxZlQ044RiM+WdoZxp0p+EGM62y3L6pwA4olE= +github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.17/go.mod h1:RkZEx4l0EHYDJpWppMJ3nD9wZJAa8/0lq9aVC+r2UII= +github.com/aws/aws-sdk-go-v2/service/sso v1.22.4 h1:BXx0ZIxvrJdSgSvKTZ+yRBeSqqgPM89VPlulEcl37tM= +github.com/aws/aws-sdk-go-v2/service/sso v1.22.4/go.mod h1:ooyCOXjvJEsUw7x+ZDHeISPMhtwI3ZCB7ggFMcFfWLU= +github.com/aws/aws-sdk-go-v2/service/ssooidc v1.26.4 h1:yiwVzJW2ZxZTurVbYWA7QOrAaCYQR72t0wrSBfoesUE= +github.com/aws/aws-sdk-go-v2/service/ssooidc v1.26.4/go.mod h1:0oxfLkpz3rQ/CHlx5hB7H69YUpFiI1tql6Q6Ne+1bCw= +github.com/aws/aws-sdk-go-v2/service/sts v1.30.3 h1:ZsDKRLXGWHk8WdtyYMoGNO7bTudrvuKpDKgMVRlepGE= +github.com/aws/aws-sdk-go-v2/service/sts v1.30.3/go.mod h1:zwySh8fpFyXp9yOr/KVzxOl8SRqgf/IDw5aUt9UKFcQ= +github.com/aws/smithy-go v1.20.3 h1:ryHwveWzPV5BIof6fyDvor6V3iUL7nTfiTKXHiW05nE= +github.com/aws/smithy-go v1.20.3/go.mod h1:krry+ya/rV9RDcV/Q16kpu6ypI4K2czasz0NC3qS14E= 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 00000000..8915fbb5 --- /dev/null +++ b/internal/llm/bedrock_test.go @@ -0,0 +1,165 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "encoding/json" + "os" + "path/filepath" + "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) + } +} + +// TestResolveBedrockPassesAWSSettings covers aws_profile / aws_region reaching +// the client, so a review run is reproducible without exporting AWS_PROFILE. +func TestResolveBedrockPassesAWSSettings(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_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) + } +} + +// 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") + } +} + +// 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 e3ec7190..3cbc7134 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -15,12 +15,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" @@ -223,6 +226,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 +284,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 +742,12 @@ 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 } // NewAnthropicClient creates a new Anthropic Messages API client. @@ -791,6 +809,103 @@ 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() + } + + // The middleware only rewrites requests whose path is already /v1/messages, + // so keep cfg.URL in the same shape the plain Anthropic client uses. It is + // not turned into a base URL: the region decides the host. + if cfg.URL != "" && !strings.HasSuffix(strings.TrimRight(cfg.URL, "/"), "/v1/messages") { + cfg.URL = strings.TrimRight(cfg.URL, "/") + "/v1/messages" + } + + 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)) + } + awsCfg, err := awsconfig.LoadDefaultConfig(context.Background(), loadOpts...) + if err != nil { + return &AnthropicClient{ + cfg: cfg, + 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 --profile `", err), + } + } + if awsCfg.Region == "" { + return &AnthropicClient{ + cfg: cfg, + 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"), + } + } + + // Force SigV4 unless a Bedrock bearer token was set deliberately. + // + // 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 here restores the SigV4 path. + // WithConfig re-reads AWS_BEARER_TOKEN_BEDROCK when the provider is nil, so + // an explicitly configured bearer token still wins. + if os.Getenv("AWS_BEARER_TOKEN_BEDROCK") == "" { + awsCfg.BearerAuthTokenProvider = nil + } + + // Appended after the options above so its base URL and middleware win. + opts = append(opts, bedrock.WithConfig(awsCfg)) + + return &AnthropicClient{ + cfg: cfg, + sdk: anthropic.NewClient(opts...), + } +} + // CompletionsWithCtx sends a chat completion request with context support. // // The deferred finalizeRequest is this client's boundary for the retry report; @@ -806,6 +921,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 diff --git a/internal/llm/protocol.go b/internal/llm/protocol.go index 8a56b861..956a70ac 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,6 +53,8 @@ func NormalizeProtocol(raw string) string { return ProtocolOpenAIChatCompletions case ProtocolOpenAIResponses: return ProtocolOpenAIResponses + case ProtocolAnthropicBedrock: + return ProtocolAnthropicBedrock default: return normalized } @@ -54,9 +64,9 @@ func NormalizeProtocol(raw string) string { // 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 e02d0a27..c8941e1a 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 48389c36..dd66c176 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 85b08342..12cf6653 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 } } @@ -294,6 +309,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 { @@ -389,8 +411,11 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, } } // 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 == "" { + // only the command's *execution* is deferred, not the emptiness check. An + // ambient-auth provider (AWS SigV4, for instance) 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 == "" && !(isPreset && preset.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) } @@ -499,9 +524,10 @@ 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. Both are empty only for an ambient-auth provider, which the + // emptiness check above lets through and which has no command to run; for + // everyone else a failing command is a hard error. + if apiKey == "" && apiKeyCmd != "" { resolved, err := resolveKeyCmd(apiKeyCmd, fmt.Sprintf("api_key_cmd for provider %q", cfg.Provider)) if err != nil { return ResolvedEndpoint{}, false, err @@ -521,6 +547,9 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, ExtraHeaders: extraHeaders, Timeout: timeout, RetryCodes: retryCodes, + AmbientAuth: isPreset && preset.AmbientAuth, + AWSProfile: entry.AWSProfile, + AWSRegion: entry.AWSRegion, }, true, nil } From b4c77058a03ba17081fb48c91e48564efefe7c04 Mon Sep 17 00:00:00 2001 From: Luis Rodriguez Date: Mon, 3 Aug 2026 09:54:29 -0400 Subject: [PATCH 2/7] feat(config): configure and diagnose an ambient-auth provider from the CLI MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Registering the provider was not enough to make it usable: every config-related path still assumed an api_key, and Bedrock's own error wording sends users after the wrong problem. - ProviderEntry gains aws_profile and aws_region. They were readable by the resolver but absent from the struct the CLI marshals, and config is unmarshalled into it and written back on every config command — so a hand-written aws_region was silently deleted the first time the user ran `ocr config model`, with no error and nothing to suggest why the next review reached a different region. - `ocr config set providers..aws_region|aws_profile` now works, for both the providers and custom_providers paths. Values are trimmed; whitespace inside one is rejected. Region names are deliberately not validated against a fixed list — AWS adds regions faster than an embedded list stays correct, and a wrong region already fails at request time. Setting either field on a provider that authenticates by api_key is an error rather than dead config that reads as applied. - The provider wizard treats the model step as final for an ambient provider instead of demanding a key. An API-key prompt that has to be left blank reads as a step the user failed to complete, and applyOfficialProviderConfig rejected the empty value anyway, so bedrock was unreachable through `ocr config provider` entirely. The gate is now a named check keyed off AmbientAuth, so key-based providers keep the requirement. - `ocr llm test` prints the resolved region and profile in place of the URL, which is empty for bedrock because the region decides the host. A request that reached the wrong region otherwise fails as though the model ID were malformed. - Bedrock rejections are translated into the action that fixes them, since two of them are actively misleading as the service words them: "Invalid API Key format" names a credential no bedrock user can configure (it means a bearer token reached the request), and a model merely absent from the region comes back as "The provided model identifier is invalid." Expired credentials point at `aws sso login` with the profile filled in; AccessDenied is named as an IAM gap on bedrock:InvokeModel rather than a bad credential; a rejected model points at `aws bedrock list-inference-profiles` and the -v1:0 suffix trap. Every other protocol shares this client type, so the translation is gated on the bedrock flag and returns other errors untouched. The unknown-config-key message is pinned byte-for-byte by an existing test; it is updated for the two new provider fields and for anthropic-bedrock as a protocol value. Verified against a live Bedrock account: `ocr llm test` reports region and profile and completes over SigV4; a -v1:0 model ID and an unresolvable profile each produce their intended message rather than a bare 400 or 403. Co-Authored-By: Claude Opus 5 (1M context) --- .../apply_provider_field_test.go | 16 +- cmd/opencodereview/bedrock_config_test.go | 233 ++++++++++++++++++ cmd/opencodereview/config_cmd.go | 60 ++++- cmd/opencodereview/config_cmd_test.go | 4 +- cmd/opencodereview/llm_cmd.go | 25 +- cmd/opencodereview/provider_cmd.go | 44 ++-- cmd/opencodereview/provider_tui.go | 15 ++ cmd/opencodereview/provider_tui_funcs_test.go | 2 + internal/llm/bedrock_test.go | 109 ++++++++ internal/llm/client.go | 103 +++++++- 10 files changed, 575 insertions(+), 36 deletions(-) create mode 100644 cmd/opencodereview/bedrock_config_test.go diff --git a/cmd/opencodereview/apply_provider_field_test.go b/cmd/opencodereview/apply_provider_field_test.go index 1b9a6e34..d266e88c 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 00000000..e3707787 --- /dev/null +++ b/cmd/opencodereview/bedrock_config_test.go @@ -0,0 +1,233 @@ +// 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) + } +} + +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 58b0dd47..d137308f 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. @@ -559,12 +569,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 @@ -619,12 +629,52 @@ 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: a built-in preset that authenticates from the +// environment, or a custom provider already declared to speak the Bedrock +// protocol. Storing them anywhere else would be dead config that reads as +// applied, so it is rejected instead. +func providerAcceptsAWSSettings(providerName string, entry *ProviderEntry) bool { + if preset, isPreset := llm.LookupProvider(providerName); isPreset { + return preset.AmbientAuth + } + return llm.NormalizeProtocol(entry.Protocol) == llm.ProtocolAnthropicBedrock +} + +// 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 +752,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 +772,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 54f9dc08..e7cf9100 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 65adce6e..b07cfd9a 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 3fa20b51..e32338ee 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 2acc0286..124196f8 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -966,6 +966,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, "" } @@ -1248,6 +1253,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)) @@ -1800,6 +1807,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() diff --git a/cmd/opencodereview/provider_tui_funcs_test.go b/cmd/opencodereview/provider_tui_funcs_test.go index 6b2ca982..a961d43a 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/internal/llm/bedrock_test.go b/internal/llm/bedrock_test.go index 8915fbb5..faefba08 100644 --- a/internal/llm/bedrock_test.go +++ b/internal/llm/bedrock_test.go @@ -5,8 +5,10 @@ package llm import ( "encoding/json" + "errors" "os" "path/filepath" + "strings" "testing" ) @@ -140,6 +142,113 @@ func TestNonAmbientProviderStillRequiresAPIKey(t *testing.T) { } } +// 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) { + 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"}, + }, + { + name: "IAM gap, not a bad credential", + err: errors.New("operation error Bedrock Runtime: AccessDeniedException: user is not authorized to perform this action"), + wantAll: []string{"bedrock:InvokeModel", "IAM policy gap"}, + }, + { + 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"}, + }, + { + 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{"IAM", "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 diff --git a/internal/llm/client.go b/internal/llm/client.go index 3cbc7134..d39543a2 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -748,6 +748,15 @@ type AnthropicClient struct { // 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. @@ -869,14 +878,18 @@ func NewAnthropicBedrockClient(cfg ClientConfig) *AnthropicClient { awsCfg, err := awsconfig.LoadDefaultConfig(context.Background(), loadOpts...) if err != nil { return &AnthropicClient{ - cfg: cfg, + 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 --profile `", err), + " 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, + 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"), } @@ -901,11 +914,89 @@ func NewAnthropicBedrockClient(cfg ClientConfig) *AnthropicClient { opts = append(opts, bedrock.WithConfig(awsCfg)) return &AnthropicClient{ - cfg: cfg, - sdk: anthropic.NewClient(opts...), + 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() + + switch { + // Checked first: the bearer-token path produces this even when credentials + // are otherwise valid, so a later "expired"/"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, "ExpiredToken"), strings.Contains(msg, "expired"), + 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)) + case strings.Contains(msg, "AccessDenied"): + return fmt.Errorf("bedrock denied access to model %q (%s): %w\n"+ + " credentials are valid, so this is an IAM policy gap: the identity needs bedrock:InvokeModel on this model in this region", model, where, err) + case strings.Contains(msg, "ValidationException"), + strings.Contains(msg, "model identifier is invalid"), + strings.Contains(msg, "don't have access to the model"): + 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)) + } + 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; @@ -957,7 +1048,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 From 0b01ab39164e8284c048c04162c13124af02d3fa Mon Sep 17 00:00:00 2001 From: Luis Rodriguez Date: Mon, 3 Aug 2026 11:01:48 -0400 Subject: [PATCH 3/7] fix(bedrock): correct bearer-token precedence, error triage, and model gating MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four defects found reviewing the two commits before this one. Each was verified by execution or against SDK source, not inferred. - AWS_BEARER_TOKEN_BEDROCK was unreachable for exactly the users it was meant to serve. The provider was cleared only when the variable was unset, on the strength of WithConfig's doc comment ("if the AWS_BEARER_TOKEN_BEDROCK environment variable is set, it is used"). The code disagrees with that comment: bedrock.go consults the variable only `if cfg.BearerAuthTokenProvider == nil`. So an SSO profile plus a deliberately configured Bedrock API key sent the SSO OIDC token instead of the key — the same silent substitution this patch exists to prevent, and explainError then blamed a token that never left the machine. Cleared unconditionally now, which is what gives the variable the precedence it documents. - A model that the account has not enabled was reported as an IAM problem. Bedrock answers both authorization failures with AccessDeniedException, and the fixes have nothing in common: "You don't have access to the model with the specified model ID" needs model access granted in the console, per account and per region, which no IAM policy provides. The specific wording is now matched ahead of the generic code, and the clause for it is no longer stranded in an unreachable branch. - A bare ValidationException match claimed every request-shape rejection was a model-ID problem: "Input is too long for requested model" sent the user off to list inference profiles. Only the model-identifier wording is matched now; everything else keeps the service's own message, which is the whole point of the function. The credential-expiry arm likewise no longer matches a bare "expired", which caught `x509: certificate has expired`. - --model rejected any Bedrock identifier absent from the preset's Models list, contradicting both the preset's own comment and this PR's description. A preset list cannot be an allowlist here: identifiers are scoped to an account and a region, and an application inference profile ARN — the value 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 no longer gates an override for an ambient-auth provider. Key-based providers keep the check, so a typo against a hosted API is still caught locally. Also: dropped a cfg.URL normalization block that could not have any effect, since WithConfig is appended last and installs its own base URL — the comment claimed a purpose the code did not have. Pinned AWS_CONFIG_FILE in the test that constructs a client, which was reading the developer's real ~/.aws/config. Fixed the column alignment of the region line in `ocr llm test`. Verified: `ocr llm test` still completes over SigV4 against a live account; an identifier the preset does not list now reaches Bedrock and returns Bedrock's own verdict rather than a local rejection. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/opencodereview/llm_cmd.go | 2 +- internal/llm/bedrock_test.go | 81 +++++++++++++++++++++++++++++++++-- internal/llm/client.go | 64 ++++++++++++++++----------- internal/llm/resolver.go | 10 ++++- 4 files changed, 127 insertions(+), 30 deletions(-) diff --git a/cmd/opencodereview/llm_cmd.go b/cmd/opencodereview/llm_cmd.go index b07cfd9a..c145ec9e 100644 --- a/cmd/opencodereview/llm_cmd.go +++ b/cmd/opencodereview/llm_cmd.go @@ -112,7 +112,7 @@ func runLLMTest() error { // 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) + fmt.Printf("Region: %s\n", region) if profile != "" { fmt.Printf("Profile: %s\n", profile) } else { diff --git a/internal/llm/bedrock_test.go b/internal/llm/bedrock_test.go index faefba08..1cd67aeb 100644 --- a/internal/llm/bedrock_test.go +++ b/internal/llm/bedrock_test.go @@ -97,6 +97,12 @@ func TestResolveBedrockWithoutAPIKey(t *testing.T) { // 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", @@ -127,6 +133,48 @@ func TestResolveBedrockPassesAWSSettings(t *testing.T) { } } +// 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) { @@ -166,20 +214,45 @@ func TestExplainErrorClassifiesBedrockFailures(t *testing.T) { wantAll: []string{"aws sso login --profile example-profile"}, }, { - name: "IAM gap, not a bad credential", - err: errors.New("operation error Bedrock Runtime: AccessDeniedException: user is not authorized to perform this action"), - wantAll: []string{"bedrock:InvokeModel", "IAM policy gap"}, + // 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"}, }, { 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{"IAM", "sso login"}, + wantNone: []string{"authorization gap", "sso login"}, }, } diff --git a/internal/llm/client.go b/internal/llm/client.go index d39543a2..4997489c 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -838,13 +838,10 @@ func NewAnthropicBedrockClient(cfg ClientConfig) *AnthropicClient { cfg.SessionKey = NewSessionKey() } - // The middleware only rewrites requests whose path is already /v1/messages, - // so keep cfg.URL in the same shape the plain Anthropic client uses. It is - // not turned into a base URL: the region decides the host. - if cfg.URL != "" && !strings.HasSuffix(strings.TrimRight(cfg.URL, "/"), "/v1/messages") { - cfg.URL = strings.TrimRight(cfg.URL, "/") + "/v1/messages" - } - + // 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")), @@ -895,7 +892,7 @@ func NewAnthropicBedrockClient(cfg ClientConfig) *AnthropicClient { } } - // Force SigV4 unless a Bedrock bearer token was set deliberately. + // Drop the credential-chain bearer token, always. // // bedrock.WithConfig prefers bearer auth over SigV4 whenever // cfg.BearerAuthTokenProvider is non-nil, and LoadDefaultConfig populates @@ -903,12 +900,17 @@ func NewAnthropicBedrockClient(cfg ClientConfig) *AnthropicClient { // 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 here restores the SigV4 path. - // WithConfig re-reads AWS_BEARER_TOKEN_BEDROCK when the provider is nil, so - // an explicitly configured bearer token still wins. - if os.Getenv("AWS_BEARER_TOKEN_BEDROCK") == "" { - awsCfg.BearerAuthTokenProvider = nil - } + // 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 after the options above so its base URL and middleware win. opts = append(opts, bedrock.WithConfig(awsCfg)) @@ -963,9 +965,13 @@ func (c *AnthropicClient) explainError(model string, err error) error { 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 { - // Checked first: the bearer-token path produces this even when credentials - // are otherwise valid, so a later "expired"/"denied" branch would mislabel it. + // 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"+ @@ -973,20 +979,30 @@ func (c *AnthropicClient) explainError(model string, err error) error { } 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, "ExpiredToken"), strings.Contains(msg, "expired"), + case strings.Contains(msg, "don't have access to the model"), + strings.Contains(msg, "not authorized to invoke this API operation"): + 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)) case strings.Contains(msg, "AccessDenied"): return fmt.Errorf("bedrock denied access to model %q (%s): %w\n"+ - " credentials are valid, so this is an IAM policy gap: the identity needs bedrock:InvokeModel on this model in this region", model, where, err) - case strings.Contains(msg, "ValidationException"), - strings.Contains(msg, "model identifier is invalid"), - strings.Contains(msg, "don't have access to the model"): - 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)) + " 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) } diff --git a/internal/llm/resolver.go b/internal/llm/resolver.go index 12cf6653..0a424b18 100644 --- a/internal/llm/resolver.go +++ b/internal/llm/resolver.go @@ -466,9 +466,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 := !(isPreset && preset.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", From 158cc1333c198e38e158265becbe41535030c60a Mon Sep 17 00:00:00 2001 From: Luis Rodriguez Date: Mon, 3 Aug 2026 16:57:07 -0400 Subject: [PATCH 4/7] fix(bedrock): honour an entry-level protocol override for ambient auth MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit OCR's own review of this PR found that ambient auth was read off the preset while the protocol could be overridden per entry, which left two ways to configure something that reads as applied and cannot work. `providers.bedrock.protocol = openai` resolved with no api_key and no URL: the key requirement was skipped because the preset declares AmbientAuth, but the endpoint then spoke a protocol with no SigV4 signing and carried nothing to authenticate with. Ambient auth is now derived from the protocol actually in force, after the override is applied, so such an entry needs a token again — and conversely an entry that selects the bedrock protocol explicitly signs its requests whatever preset it sits under. The same value gates the --model allowlist, which had the same preset-only assumption. `ocr config set providers.bedrock.aws_region` accepted AWS settings on that same overridden entry. The check now lets the entry's protocol decide whenever it sets one, falling back to the preset's flag only when the entry is silent. Also corrects two stale doc comments the review flagged: ValidateProtocol accepts four protocol names, not three, and the package comment now lists anthropic-bedrock among the supported protocols. The third finding in that review — a bare ValidationException match in explainError — was already fixed in the preceding commit; the bot reviewed the commit before it. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/opencodereview/bedrock_config_test.go | 26 ++++++++++++++++ cmd/opencodereview/config_cmd.go | 18 ++++++----- internal/llm/bedrock_test.go | 37 +++++++++++++++++++++++ internal/llm/client.go | 1 + internal/llm/protocol.go | 2 +- internal/llm/resolver.go | 32 +++++++++++++------- 6 files changed, 97 insertions(+), 19 deletions(-) diff --git a/cmd/opencodereview/bedrock_config_test.go b/cmd/opencodereview/bedrock_config_test.go index e3707787..f0a93852 100644 --- a/cmd/opencodereview/bedrock_config_test.go +++ b/cmd/opencodereview/bedrock_config_test.go @@ -163,6 +163,32 @@ func TestSetCustomProviderAWSSettingsFollowProtocol(t *testing.T) { } } +// 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) + } +} + func TestCheckAPIKeyRequirement(t *testing.T) { bedrock, ok := llm.LookupProvider("bedrock") if !ok { diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index d137308f..3601f7ad 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -649,15 +649,19 @@ func applyProviderField(providerName string, entry *ProviderEntry, field, key, v } // providerAcceptsAWSSettings reports whether aws_region / aws_profile mean -// anything for this provider: a built-in preset that authenticates from the -// environment, or a custom provider already declared to speak the Bedrock -// protocol. Storing them anywhere else would be dead config that reads as -// applied, so it is rejected instead. +// 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 preset, isPreset := llm.LookupProvider(providerName); isPreset { - return preset.AmbientAuth + if entry.Protocol != "" { + return llm.NormalizeProtocol(entry.Protocol) == llm.ProtocolAnthropicBedrock } - 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 diff --git a/internal/llm/bedrock_test.go b/internal/llm/bedrock_test.go index 1cd67aeb..842bf428 100644 --- a/internal/llm/bedrock_test.go +++ b/internal/llm/bedrock_test.go @@ -133,6 +133,43 @@ func TestResolveBedrockPassesAWSSettings(t *testing.T) { } } +// 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") + } + }) +} + // 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 diff --git a/internal/llm/client.go b/internal/llm/client.go index 4997489c..713f2931 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 diff --git a/internal/llm/protocol.go b/internal/llm/protocol.go index 956a70ac..04250e9c 100644 --- a/internal/llm/protocol.go +++ b/internal/llm/protocol.go @@ -60,7 +60,7 @@ func NormalizeProtocol(raw string) string { } } -// 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 { diff --git a/internal/llm/resolver.go b/internal/llm/resolver.go index 0a424b18..7058bc48 100644 --- a/internal/llm/resolver.go +++ b/internal/llm/resolver.go @@ -410,15 +410,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. An - // ambient-auth provider (AWS SigV4, for instance) 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 == "" && !(isPreset && preset.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) - } - var url, protocol, authHeader, model string var extraBody map[string]any @@ -452,6 +443,25 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, 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 } @@ -472,7 +482,7 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, // 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 := !(isPreset && preset.AmbientAuth) + gateOverrideOnModelList := !ambientAuth // Apply model override with validation. if modelOverride != "" { @@ -555,7 +565,7 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, ExtraHeaders: extraHeaders, Timeout: timeout, RetryCodes: retryCodes, - AmbientAuth: isPreset && preset.AmbientAuth, + AmbientAuth: ambientAuth, AWSProfile: entry.AWSProfile, AWSRegion: entry.AWSRegion, }, true, nil From 68f19aba3e87af46e6a0957e7826ec6a25d9f9a0 Mon Sep 17 00:00:00 2001 From: Luis Rodriguez Date: Tue, 4 Aug 2026 14:22:46 -0400 Subject: [PATCH 5/7] build(deps): bump aws-sdk-go-v2 to clear GO-2026-5764 govulncheck fails the CI test job because the pinned AWS SDK tree pulls in github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.6.3, which is affected by GO-2026-5764 (fixed in v1.7.8). Upgrading the direct dependency aws-sdk-go-v2/config to current resolves eventstream to v1.7.16, past the fixed version. The diff is scoped to the AWS module tree (plus smithy-go, its runtime companion); no other dependencies move. The Bedrock provider's behavior is unchanged: the newer config module still populates BearerAuthTokenProvider from the SSO token cache, so the unconditional clearing in NewAnthropicBedrockClient remains necessary and correct, and it still does not consult AWS_BEARER_TOKEN_BEDROCK itself, so the anthropic-sdk-go re-read of that variable keeps its documented precedence. Co-Authored-By: Claude Opus 5 (1M context) --- go.mod | 29 +++++++++++++++-------------- go.sum | 58 ++++++++++++++++++++++++++++++---------------------------- 2 files changed, 45 insertions(+), 42 deletions(-) diff --git a/go.mod b/go.mod index 58ae224d..b5186483 100644 --- a/go.mod +++ b/go.mod @@ -7,7 +7,7 @@ require ( charm.land/bubbletea/v2 v2.0.7 charm.land/lipgloss/v2 v2.0.4 github.com/anthropics/anthropic-sdk-go v1.55.1 - github.com/aws/aws-sdk-go-v2/config v1.27.27 + github.com/aws/aws-sdk-go-v2/config v1.32.34 github.com/bmatcuk/doublestar/v4 v4.10.0 github.com/google/uuid v1.6.0 github.com/modelcontextprotocol/go-sdk v1.6.1 @@ -30,19 +30,20 @@ require ( require ( github.com/atotto/clipboard v0.1.4 // indirect - github.com/aws/aws-sdk-go-v2 v1.30.3 // indirect - github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.6.3 // indirect - github.com/aws/aws-sdk-go-v2/credentials v1.17.27 // indirect - github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.11 // indirect - github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.15 // indirect - github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.15 // indirect - github.com/aws/aws-sdk-go-v2/internal/ini v1.8.0 // indirect - github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.3 // indirect - github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.17 // indirect - github.com/aws/aws-sdk-go-v2/service/sso v1.22.4 // indirect - github.com/aws/aws-sdk-go-v2/service/ssooidc v1.26.4 // indirect - github.com/aws/aws-sdk-go-v2/service/sts v1.30.3 // indirect - github.com/aws/smithy-go v1.20.3 // indirect + github.com/aws/aws-sdk-go-v2 v1.43.3 // 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.33 // indirect + github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.18.34 // indirect + github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.34 // indirect + github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.34 // indirect + github.com/aws/aws-sdk-go-v2/internal/v4a v1.4.35 // 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.34 // indirect + github.com/aws/aws-sdk-go-v2/service/signin v1.5.3 // indirect + github.com/aws/aws-sdk-go-v2/service/sso v1.33.3 // indirect + github.com/aws/aws-sdk-go-v2/service/ssooidc v1.38.3 // indirect + github.com/aws/aws-sdk-go-v2/service/sts v1.45.3 // 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 1335eb6b..5b68339d 100644 --- a/go.sum +++ b/go.sum @@ -8,34 +8,36 @@ github.com/anthropics/anthropic-sdk-go v1.55.1 h1:GxukHUVou6AFIngxa/Aw1z79hmwg13 github.com/anthropics/anthropic-sdk-go v1.55.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.30.3 h1:jUeBtG0Ih+ZIFH0F4UkmL9w3cSpaMv9tYYDbzILP8dY= -github.com/aws/aws-sdk-go-v2 v1.30.3/go.mod h1:nIQjQVp5sfpQcTc9mPSr1B0PaWK5ByX9MOoDadSN4lc= -github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.6.3 h1:tW1/Rkad38LA15X4UQtjXZXNKsCgkshC3EbmcUmghTg= -github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.6.3/go.mod h1:UbnqO+zjqk3uIt9yCACHJ9IVNhyhOCnYk8yA19SAWrM= -github.com/aws/aws-sdk-go-v2/config v1.27.27 h1:HdqgGt1OAP0HkEDDShEl0oSYa9ZZBSOmKpdpsDMdO90= -github.com/aws/aws-sdk-go-v2/config v1.27.27/go.mod h1:MVYamCg76dFNINkZFu4n4RjDixhVr51HLj4ErWzrVwg= -github.com/aws/aws-sdk-go-v2/credentials v1.17.27 h1:2raNba6gr2IfA0eqqiP2XiQ0UVOpGPgDSi0I9iAP+UI= -github.com/aws/aws-sdk-go-v2/credentials v1.17.27/go.mod h1:gniiwbGahQByxan6YjQUMcW4Aov6bLC3m+evgcoN4r4= -github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.11 h1:KreluoV8FZDEtI6Co2xuNk/UqI9iwMrOx/87PBNIKqw= -github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.16.11/go.mod h1:SeSUYBLsMYFoRvHE0Tjvn7kbxaUhl75CJi1sbfhMxkU= -github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.15 h1:SoNJ4RlFEQEbtDcCEt+QG56MY4fm4W8rYirAmq+/DdU= -github.com/aws/aws-sdk-go-v2/internal/configsources v1.3.15/go.mod h1:U9ke74k1n2bf+RIgoX1SXFed1HLs51OgUSs+Ph0KJP8= -github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.15 h1:C6WHdGnTDIYETAm5iErQUiVNsclNx9qbJVPIt03B6bI= -github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.6.15/go.mod h1:ZQLZqhcu+JhSrA9/NXRm8SkDvsycE+JkV3WGY41e+IM= -github.com/aws/aws-sdk-go-v2/internal/ini v1.8.0 h1:hT8rVHwugYE2lEfdFE0QWVo81lF7jMrYJVDWI+f+VxU= -github.com/aws/aws-sdk-go-v2/internal/ini v1.8.0/go.mod h1:8tu/lYfQfFe6IGnaOdrpVgEL2IrrDOf6/m9RQum4NkY= -github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.3 h1:dT3MqvGhSoaIhRseqw2I0yH81l7wiR2vjs57O51EAm8= -github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.3/go.mod h1:GlAeCkHwugxdHaueRr4nhPuY+WW+gR8UjlcqzPr1SPI= -github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.17 h1:HGErhhrxZlQ044RiM+WdoZxp0p+EGM62y3L6pwA4olE= -github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.11.17/go.mod h1:RkZEx4l0EHYDJpWppMJ3nD9wZJAa8/0lq9aVC+r2UII= -github.com/aws/aws-sdk-go-v2/service/sso v1.22.4 h1:BXx0ZIxvrJdSgSvKTZ+yRBeSqqgPM89VPlulEcl37tM= -github.com/aws/aws-sdk-go-v2/service/sso v1.22.4/go.mod h1:ooyCOXjvJEsUw7x+ZDHeISPMhtwI3ZCB7ggFMcFfWLU= -github.com/aws/aws-sdk-go-v2/service/ssooidc v1.26.4 h1:yiwVzJW2ZxZTurVbYWA7QOrAaCYQR72t0wrSBfoesUE= -github.com/aws/aws-sdk-go-v2/service/ssooidc v1.26.4/go.mod h1:0oxfLkpz3rQ/CHlx5hB7H69YUpFiI1tql6Q6Ne+1bCw= -github.com/aws/aws-sdk-go-v2/service/sts v1.30.3 h1:ZsDKRLXGWHk8WdtyYMoGNO7bTudrvuKpDKgMVRlepGE= -github.com/aws/aws-sdk-go-v2/service/sts v1.30.3/go.mod h1:zwySh8fpFyXp9yOr/KVzxOl8SRqgf/IDw5aUt9UKFcQ= -github.com/aws/smithy-go v1.20.3 h1:ryHwveWzPV5BIof6fyDvor6V3iUL7nTfiTKXHiW05nE= -github.com/aws/smithy-go v1.20.3/go.mod h1:krry+ya/rV9RDcV/Q16kpu6ypI4K2czasz0NC3qS14E= +github.com/aws/aws-sdk-go-v2 v1.43.3 h1:XJIcfv8uDs2ukdQsoAC8/Ebu1ejxwzlayl2ZsiFns2A= +github.com/aws/aws-sdk-go-v2 v1.43.3/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.34 h1:o+YAizrX562nEZXaB38uYTK8RvIsvW0uuRP+e5e0Pfk= +github.com/aws/aws-sdk-go-v2/config v1.32.34/go.mod h1:wc0zYRChOniiufvdWiRVf3jgXSgbkvaD683IHHHc2ZQ= +github.com/aws/aws-sdk-go-v2/credentials v1.19.33 h1:/e5V3EWfeDiW6cuRxHsC8gbwko4/vvVYPJR2afBKFFY= +github.com/aws/aws-sdk-go-v2/credentials v1.19.33/go.mod h1:ZxAmkcyOM9beY/WO9oxp2oVPXiP3rq5N1/p4NbenJdE= +github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.18.34 h1:1EsGke6rTD2CG3j2MMVB77n6Q+FlbQWYI/dFdLWBNtM= +github.com/aws/aws-sdk-go-v2/feature/ec2/imds v1.18.34/go.mod h1:5B1Z/QbaWzqoWRzYxZfmCbDDRcvUHcfAIQw/S+KfDmc= +github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.34 h1:vuIfjzoeqhQMGJyOBU3t0ZEjn2jrN8Bbg1N4CgjzM5Q= +github.com/aws/aws-sdk-go-v2/internal/configsources v1.4.34/go.mod h1:hP28cN4CPJLZHirdQPrZR50JcLN4ApRJP2tzG8cRlhY= +github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.34 h1:9faHsnqxJ1vDvB4wMZy/ajIDyz5QhllQjjc72RJpXAw= +github.com/aws/aws-sdk-go-v2/internal/endpoints/v2 v2.7.34/go.mod h1:Yp6nIyejpa23nzlB/LhT63KTla9Jdi06nv/HH/OkAH8= +github.com/aws/aws-sdk-go-v2/internal/v4a v1.4.35 h1:Oe8gMKJLO5awqpa5EhAGKVnBv1s+brdWVuxM2mDa7zA= +github.com/aws/aws-sdk-go-v2/internal/v4a v1.4.35/go.mod h1:FZevcG9cOST/FWAAUhHIchjR9fXFXFRCWodOhx+PDLA= +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.34 h1:sYg4qHWLqsjp15PzX7XCOHSOgKEGoZ5vQY43VvZ1pas= +github.com/aws/aws-sdk-go-v2/service/internal/presigned-url v1.13.34/go.mod h1:N58SSz3roKf1HzW5qRaOiyk6MbDLTKgLPvlTfJ90iyI= +github.com/aws/aws-sdk-go-v2/service/signin v1.5.3 h1:togAtAmgV5IGMnQDuBDJeM8z5Y5RN6G7xeOgphWz+Yc= +github.com/aws/aws-sdk-go-v2/service/signin v1.5.3/go.mod h1:T7xKUUUvN7W3RW8UmMvKnD12xqh+Ux2gCPHPhnt64Dg= +github.com/aws/aws-sdk-go-v2/service/sso v1.33.3 h1:YjH64OUytnWZBHUtM9GMyi4ZWBiSQdEJkZuPykOIe44= +github.com/aws/aws-sdk-go-v2/service/sso v1.33.3/go.mod h1:5qoHcDZDTSJotoKk1bvVRPv1MXaL/NhfY9ng8D1g/ig= +github.com/aws/aws-sdk-go-v2/service/ssooidc v1.38.3 h1:A4o1di/XGaqtw6r3toSBrFX2U7mVSLqg7jo9wL4I+cU= +github.com/aws/aws-sdk-go-v2/service/ssooidc v1.38.3/go.mod h1:sKuKz2kHtrGVtFu34vbM3LWSA9CKD9YZUmm6e5PPqRA= +github.com/aws/aws-sdk-go-v2/service/sts v1.45.3 h1:Fi7+DiKN1+QphlajvE6FqeZ8GRbnnRul7zTdUiRpbGc= +github.com/aws/aws-sdk-go-v2/service/sts v1.45.3/go.mod h1:KCc3e27fHZUGtzpek7wZcp6dyCpGkJJo/+3PBujh/yU= +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= From a6933d417e8b176ad1f1364f178fec379f78aa47 Mon Sep 17 00:00:00 2001 From: Luis Rodriguez Date: Tue, 18 Aug 2026 16:15:50 -0400 Subject: [PATCH 6/7] fix(bedrock): correct error triage, ambient credential handling, and the protocol's reach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the six pre-merge items from review. - "not authorized to invoke this API operation" sat in the model-access branch. It is IAM's own wording, so it pointed at a console toggle when the fix is a policy change; it now falls to the AccessDenied branch, whose message names bedrock:InvokeModel. A test case carries the phrase without "AccessDenied" in the text, so the phrase itself is pinned to the authorization branch rather than the exception name. - TestExplainErrorClassifiesBedrockFailures read the ambient AWS_BEARER_TOKEN_BEDROCK and asserted the message produced when it is unset, so the suite failed on any machine that exports one. It pins the variable empty. Reproduced before the fix with AWS_BEARER_TOKEN_BEDROCK=sk-x go test -run TestExplainError. - api_key_cmd ran for an ambient-auth provider. The output is discarded by a signed request, and the command is typically a secret-manager read, so a bedrock user got a real 1Password / Touch ID prompt for nothing — while the comment above the call claimed it could not happen. Gated on !ambientAuth; the new test proves non-execution with a sentinel file, and fails without the gate. - llm.protocol and OCR_LLM_PROTOCOL validated anthropic-bedrock and then ignored it. Both strategies describe one URL and one token, have nowhere to carry a region or a profile, and bedrock uses neither value they do carry, so the request would have been signed and re-hosted with the rest of the block silently dropped. Both now reject it, as does `ocr config set llm.protocol`, at the point the value is typed. - The custom-provider contract was split in two: the TUI never offered bedrock, while the resolver demanded a url the bedrock client never reads. Settled toward supporting it — a provider entry is the one place with somewhere to put aws_region and aws_profile, which is what lets a second region or profile have its own entry. url is now required for every protocol except bedrock, the Custom form offers bedrock and ends at the protocol step (there is no url, api key or auth header to collect), and switching an existing entry to it clears the three fields the previous protocol needed rather than leaving them as dead config. The Manual form keeps the three-protocol list, since it writes llm.url and llm.auth_token. - Documented the bedrock.WithConfig append site: options wrap in order, so appending last leaves signing innermost — closest to the wire, and re-run on each retry rather than replaying a stale signature. Co-Authored-By: Claude Opus 5 (1M context) --- cmd/opencodereview/bedrock_config_test.go | 26 ++++ cmd/opencodereview/config_cmd.go | 6 + cmd/opencodereview/provider_tui.go | 134 ++++++++++++----- .../provider_tui_customform_test.go | 60 ++++++++ cmd/opencodereview/provider_tui_test.go | 28 +++- internal/llm/bedrock_test.go | 139 ++++++++++++++++++ internal/llm/client.go | 15 +- internal/llm/resolver.go | 38 ++++- 8 files changed, 395 insertions(+), 51 deletions(-) diff --git a/cmd/opencodereview/bedrock_config_test.go b/cmd/opencodereview/bedrock_config_test.go index f0a93852..44da6b8f 100644 --- a/cmd/opencodereview/bedrock_config_test.go +++ b/cmd/opencodereview/bedrock_config_test.go @@ -189,6 +189,32 @@ func TestAWSSettingsRejectedWhenEntryOverridesProtocol(t *testing.T) { } } +// 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 { diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 3601f7ad..7a5b7031 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -505,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 diff --git a/cmd/opencodereview/provider_tui.go b/cmd/opencodereview/provider_tui.go index 124196f8..fdd98013 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 { @@ -1129,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: @@ -1153,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) { @@ -1211,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 { @@ -1325,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 { @@ -1465,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 @@ -1942,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, } @@ -2005,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, } } @@ -2192,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 { @@ -2213,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: @@ -2271,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}, @@ -2284,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 4ee9f1de..a05cd7b5 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_test.go b/cmd/opencodereview/provider_tui_test.go index 5e108b89..5edf3e9b 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/internal/llm/bedrock_test.go b/internal/llm/bedrock_test.go index 842bf428..7f1a3e80 100644 --- a/internal/llm/bedrock_test.go +++ b/internal/llm/bedrock_test.go @@ -94,6 +94,33 @@ func TestResolveBedrockWithoutAPIKey(t *testing.T) { } } +// 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) { @@ -170,6 +197,104 @@ func TestAmbientAuthFollowsTheEffectiveProtocol(t *testing.T) { }) } +// 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 @@ -232,6 +357,11 @@ func TestNonAmbientProviderStillRequiresAPIKey(t *testing.T) { // 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 { @@ -265,6 +395,15 @@ func TestExplainErrorClassifiesBedrockFailures(t *testing.T) { 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."), diff --git a/internal/llm/client.go b/internal/llm/client.go index 713f2931..b0a17225 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -913,7 +913,11 @@ func NewAnthropicBedrockClient(cfg ClientConfig) *AnthropicClient { // a static provider from it; unset, the SigV4 path runs. awsCfg.BearerAuthTokenProvider = nil - // Appended after the options above so its base URL and middleware win. + // 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{ @@ -980,8 +984,7 @@ func (c *AnthropicClient) explainError(model string, err error) error { } 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"), - strings.Contains(msg, "not authorized to invoke this API operation"): + 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"), @@ -996,7 +999,11 @@ func (c *AnthropicClient) explainError(model string, err error) error { 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)) - case strings.Contains(msg, "AccessDenied"): + // "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) } diff --git a/internal/llm/resolver.go b/internal/llm/resolver.go index 7058bc48..3b065402 100644 --- a/internal/llm/resolver.go +++ b/internal/llm/resolver.go @@ -233,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) @@ -252,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 @@ -431,14 +444,21 @@ 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 } @@ -542,10 +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. Both are empty only for an ambient-auth provider, which the - // emptiness check above lets through and which has no command to run; for - // everyone else a failing command is a hard error. - if apiKey == "" && apiKeyCmd != "" { + // 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 @@ -609,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 From f6d8f4e1d1163c57dd941eca69137ec87aa0e8d1 Mon Sep 17 00:00:00 2001 From: Luis Rodriguez Date: Tue, 18 Aug 2026 16:16:00 -0400 Subject: [PATCH 7/7] docs(pages): document the bedrock provider in all four locales MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The provider shipped with no documentation: no row in the built-in table, and no mention of aws_region or aws_profile, so the only way to learn either field existed was to read the resolver. Adds to en, ja, ru and zh: - a `bedrock` row in the built-in provider table, with the host derived from aws_region and no API key env var, since neither applies; - an "AWS Bedrock" section covering the two AWS fields and what each falls back to, why model IDs are not validated against the shipped list (they are scoped to an account and a region, and an application inference profile ARN can never appear in a list compiled upstream), the `-v1:0` suffix trap, and the region/profile lines `ocr llm test` prints in place of a URL; - anthropic-bedrock in the custom-provider protocol list, with the example that needs no url and takes the same AWS fields — the supported way to run a second region or profile; - a note that llm.protocol and OCR_LLM_PROTOCOL reject it, and why. Co-Authored-By: Claude Opus 5 (1M context) --- pages/src/content/docs/en/configuration.md | 62 ++++++++++++++++++++- pages/src/content/docs/ja/configuration.md | 61 ++++++++++++++++++++- pages/src/content/docs/ru/configuration.md | 64 +++++++++++++++++++++- pages/src/content/docs/zh/configuration.md | 58 +++++++++++++++++++- 4 files changed, 240 insertions(+), 5 deletions(-) diff --git a/pages/src/content/docs/en/configuration.md b/pages/src/content/docs/en/configuration.md index 7ea1f0bc..022988ab 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 e6290996..f43040db 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 7ad074d3..46dc86d5 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 1aeae918..c620f0fb 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：