From 4b211893d8cd4b796af1d2aeb691738a97303d42 Mon Sep 17 00:00:00 2001 From: Luis Rodriguez Date: Mon, 3 Aug 2026 09:39:53 -0400 Subject: [PATCH 1/5] 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 | 110 ++++++++++++++++++++++ internal/llm/protocol.go | 14 ++- internal/llm/providers.go | 36 +++++++ internal/llm/providers_test.go | 13 ++- internal/llm/resolver.go | 33 ++++++- 8 files changed, 404 insertions(+), 9 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..65e31a06 --- /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).(*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 d88928df..c6a4b992 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -14,12 +14,15 @@ import ( "errors" "fmt" "io" + "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" @@ -195,6 +198,11 @@ type ClientConfig struct { Timeout time.Duration // Request timeout ExtraBody map[string]any // Vendor-specific fields merged into every request body ExtraHeaders map[string]string // Extra HTTP headers sent with every request + + // AWSProfile and AWSRegion are used only by SigV4 providers (bedrock). + // Empty means the standard AWS credential chain decides. + AWSProfile string + AWSRegion string } // --- Factory --- @@ -217,10 +225,14 @@ func NewLLMClient(ep ResolvedEndpoint) LLMClient { Timeout: ep.Timeout, ExtraBody: ep.ExtraBody, ExtraHeaders: ep.ExtraHeaders, + 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: @@ -599,6 +611,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. @@ -650,8 +668,100 @@ 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 + } + + // 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"), + } + for k, v := range cfg.ExtraHeaders { + opts = append(opts, option.WithHeader(k, v)) + } + + // 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. func (c *AnthropicClient) CompletionsWithCtx(ctx context.Context, req ChatRequest) (*ChatResponse, error) { + 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 1f097dfe..d4b7000d 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 1e3113c1..6e41ecd9 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", "hy-tokenplan", "iflytek", "kimi", "litellm", "mimo", "minimax", "minimax-cn", "ollama-cloud", "openai", "tencent-tokenhub", "volcengine", "z-ai", "z-ai-coding"} + expected := []string{"anthropic", "baidu-qianfan", "bedrock", "dashscope", "dashscope-tokenplan", "deepseek", "edenai", "hy-tokenplan", "iflytek", "kimi", "litellm", "mimo", "minimax", "minimax-cn", "ollama-cloud", "openai", "tencent-tokenhub", "volcengine", "z-ai", "z-ai-coding"} if len(providers) != len(expected) { t.Fatalf("expected %d providers, got %d", len(expected), len(providers)) } @@ -283,11 +283,14 @@ func TestLookupProvider_LiteLLMDetails(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 9ae85fcc..66f14b33 100644 --- a/internal/llm/resolver.go +++ b/internal/llm/resolver.go @@ -31,6 +31,18 @@ type ResolvedEndpoint struct { // tryCCEnv and tryShellRC always leave it at 0 since those sources have no timeout // knob; users can still override via OCR_LLM_TIMEOUT. Timeout time.Duration + + // 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. @@ -112,7 +124,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) } } @@ -257,6 +272,13 @@ type providerEntryConfig struct { TimeoutSec int `json:"timeout_sec,omitempty"` // per-request HTTP timeout in seconds ExtraBody map[string]any `json:"extra_body,omitempty"` ExtraHeaders map[string]string `json:"extra_headers,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 { @@ -320,7 +342,11 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, apiKey = os.Getenv(preset.EnvVar) } } - if apiKey == "" { + // An ambient-auth provider (AWS SigV4, for instance) has no key to + // configure: credentials come from the environment's own chain, and the + // request is signed rather than bearing a token. Requiring api_key here + // would make such a provider unusable. + if apiKey == "" && !(isPreset && preset.AmbientAuth) { return ResolvedEndpoint{}, false, fmt.Errorf("provider %q has no api_key configured and no environment variable fallback found", cfg.Provider) } @@ -433,6 +459,9 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, ExtraBody: extraBody, ExtraHeaders: extraHeaders, Timeout: timeout, + AmbientAuth: isPreset && preset.AmbientAuth, + AWSProfile: entry.AWSProfile, + AWSRegion: entry.AWSRegion, }, true, nil } From e8a949f8f703e65dc95b66f25dc91dd5f167f11d Mon Sep 17 00:00:00 2001 From: Luis Rodriguez Date: Mon, 3 Aug 2026 09:54:29 -0400 Subject: [PATCH 2/5] 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 | 230 ++++++++++++++++++ cmd/opencodereview/config_cmd.go | 60 ++++- cmd/opencodereview/config_cmd_test.go | 4 +- cmd/opencodereview/llm_cmd.go | 25 +- cmd/opencodereview/provider_cmd.go | 31 ++- cmd/opencodereview/provider_tui.go | 13 + internal/llm/bedrock_test.go | 109 +++++++++ internal/llm/client.go | 103 +++++++- 9 files changed, 561 insertions(+), 30 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..1f187179 --- /dev/null +++ b/cmd/opencodereview/bedrock_config_test.go @@ -0,0 +1,230 @@ +// 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") + } +} + +// 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 b512f4e6..1aec1b1b 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -293,6 +293,16 @@ type ProviderEntry struct { TimeoutSec int `json:"timeout_sec,omitempty"` // per-request HTTP timeout in seconds ExtraBody map[string]any `json:"extra_body,omitempty"` ExtraHeaders map[string]string `json:"extra_headers,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. @@ -534,12 +544,12 @@ func setConfigValue(cfg *Config, key, value string) error { } cfg.Llm.ExtraBody = m default: - return fmt.Errorf("unknown config key: %s\nSupported keys: %s\nProvider fields: api_key, url, protocol, model, models, auth_header, extra_body, extra_headers\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, url, protocol, model, models, auth_header, extra_body, extra_headers, 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 @@ -577,12 +587,52 @@ func applyProviderField(entry *ProviderEntry, field, key, value string) error { return fmt.Errorf("invalid extra headers for %s: %w", key, err) } entry.ExtraHeaders = parsed + 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, url, protocol, model, models, auth_header, extra_body, extra_headers", field) + return fmt.Errorf("unknown provider field %q: supported fields are api_key, url, protocol, model, models, auth_header, extra_body, extra_headers, 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 == "" { @@ -660,7 +710,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 @@ -680,7 +730,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 fa4abd3c..0984d344 100644 --- a/cmd/opencodereview/config_cmd_test.go +++ b/cmd/opencodereview/config_cmd_test.go @@ -998,8 +998,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_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging\n" + - "Provider fields: api_key, url, protocol, model, models, auth_header, extra_body, extra_headers\n" + - "Protocol values: anthropic, openai, openai-responses\n" + + "Provider fields: api_key, url, protocol, model, models, auth_header, extra_body, extra_headers, 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 cad3e337..fe9c0a01 100644 --- a/cmd/opencodereview/llm_cmd.go +++ b/cmd/opencodereview/llm_cmd.go @@ -106,7 +106,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() @@ -118,6 +130,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 5b57410d..e654db98 100644 --- a/cmd/opencodereview/provider_cmd.go +++ b/cmd/opencodereview/provider_cmd.go @@ -227,6 +227,27 @@ func applyCustomProviderConfig(configPath string, cfg *Config, result providerTU return nil } +// checkAPIKeyRequirement decides whether a provider selection may be saved with +// no api_key. An ambient-auth provider has none to save: 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 string, preset llm.Provider, isPreset bool) error { + if apiKey != "" { + 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 or set $%s)", providerName, preset.EnvVar) + } + return nil + default: + return fmt.Errorf("API key is required for provider %s", providerName) + } +} + func applyOfficialProviderConfig(configPath string, cfg *Config, result providerTUIResult) error { if result.provider == "" { return fmt.Errorf("provider and model are required") @@ -238,14 +259,8 @@ func applyOfficialProviderConfig(configPath string, cfg *Config, result provider preset, isPreset := llm.LookupProvider(result.provider) - if result.apiKey == "" { - if isPreset && preset.EnvVar != "" { - if os.Getenv(preset.EnvVar) == "" { - return fmt.Errorf("API key is required for provider %s (configure it or set $%s)", result.provider, preset.EnvVar) - } - } else { - return fmt.Errorf("API key is required for provider %s", result.provider) - } + if err := checkAPIKeyRequirement(result.provider, result.apiKey, 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 87ca46d1..3d1c4af7 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -923,6 +923,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, "" } @@ -1734,6 +1739,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/internal/llm/bedrock_test.go b/internal/llm/bedrock_test.go index 65e31a06..d02f7fa8 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 c6a4b992..91d70e12 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -617,6 +617,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. @@ -720,14 +729,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"), } @@ -752,11 +765,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. func (c *AnthropicClient) CompletionsWithCtx(ctx context.Context, req ChatRequest) (*ChatResponse, error) { if c.initErr != nil { @@ -786,7 +877,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 6475eadb93ba14891d696f921667f270d98d444c Mon Sep 17 00:00:00 2001 From: Luis Rodriguez Date: Mon, 3 Aug 2026 11:01:48 -0400 Subject: [PATCH 3/5] 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 fe9c0a01..b1d6e0ef 100644 --- a/cmd/opencodereview/llm_cmd.go +++ b/cmd/opencodereview/llm_cmd.go @@ -110,7 +110,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 d02f7fa8..b87fa6cf 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 91d70e12..2c8384af 100644 --- a/internal/llm/client.go +++ b/internal/llm/client.go @@ -694,13 +694,10 @@ func NewAnthropicBedrockClient(cfg ClientConfig) *AnthropicClient { cfg.Timeout = 5 * time.Minute } - // 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")), @@ -746,7 +743,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 @@ -754,12 +751,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)) @@ -814,9 +816,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"+ @@ -824,20 +830,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 66f14b33..029fe133 100644 --- a/internal/llm/resolver.go +++ b/internal/llm/resolver.go @@ -397,9 +397,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 c90bfb5dc4ae51baffb44d35c92845200c4da239 Mon Sep 17 00:00:00 2001 From: Luis Rodriguez Date: Mon, 3 Aug 2026 16:57:07 -0400 Subject: [PATCH 4/5] 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 | 29 ++++++++++++------ 6 files changed, 95 insertions(+), 18 deletions(-) diff --git a/cmd/opencodereview/bedrock_config_test.go b/cmd/opencodereview/bedrock_config_test.go index 1f187179..3aa5792b 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 1aec1b1b..bb206ee0 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -607,15 +607,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 b87fa6cf..685d5ded 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 2c8384af..e8e97fb2 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 029fe133..ac4e4209 100644 --- a/internal/llm/resolver.go +++ b/internal/llm/resolver.go @@ -342,14 +342,6 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, apiKey = os.Getenv(preset.EnvVar) } } - // An ambient-auth provider (AWS SigV4, for instance) has no key to - // configure: credentials come from the environment's own chain, and the - // request is signed rather than bearing a token. Requiring api_key here - // would make such a provider unusable. - if apiKey == "" && !(isPreset && preset.AmbientAuth) { - return ResolvedEndpoint{}, false, fmt.Errorf("provider %q has no api_key configured and no environment variable fallback found", cfg.Provider) - } - var url, protocol, authHeader, model string var extraBody map[string]any @@ -383,6 +375,23 @@ 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 == "") + + // An ambient-auth provider has no key to configure: credentials come from + // the environment's own chain and the request is signed rather than bearing + // a token, so requiring api_key here would make it unusable. + if apiKey == "" && !ambientAuth { + return ResolvedEndpoint{}, false, fmt.Errorf("provider %q has no api_key configured and no environment variable fallback found", cfg.Provider) + } + if cfg.Model != "" { model = cfg.Model } @@ -403,7 +412,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 != "" { @@ -467,7 +476,7 @@ func tryProviderConfig(cfg configFile, modelOverride string) (ResolvedEndpoint, ExtraBody: extraBody, ExtraHeaders: extraHeaders, Timeout: timeout, - AmbientAuth: isPreset && preset.AmbientAuth, + AmbientAuth: ambientAuth, AWSProfile: entry.AWSProfile, AWSRegion: entry.AWSRegion, }, true, nil From 48b9f4070ceba4fb0f7445ee2059e43c1479589b Mon Sep 17 00:00:00 2001 From: Luis Rodriguez Date: Tue, 4 Aug 2026 14:22:46 -0400 Subject: [PATCH 5/5] 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=