feat(providers): add AWS Bedrock as a built-in provider with native SigV4 auth

[cru-Luis-Rodriguez]

Closes #659. Adds bedrock as a built-in provider with native SigV4 auth, taking option (1) direct dependency as you decided in that issue.

Two commits: the provider itself, then the config/CLI surface it turned out to need. The second one exists because registering a provider was not sufficient to make it configurable — details under Q1–Q3 below.

Sample config

{
  "provider": "bedrock",
  "model": "us.anthropic.claude-sonnet-4-6",
  "providers": {
    "bedrock": {
      "aws_region": "us-west-2",
      "aws_profile": "example-profile"
    }
  }
}

Both AWS fields are optional; without them the standard credential chain decides, as with any other AWS tool. There is no api_key line, and none is accepted as a substitute for a signature.

Answers to the five questions from #659

1. Provider entry shape. api_key is optional and ignored, not forbidden. Rejecting a stray key would punish someone who pasted one out of habit, and it cannot leak into a request: the client deletes Authorization and X-Api-Key before the signing middleware runs. The resolver's gate is now apiKey == "" && apiKeyCmd == "" && !ambientAuth, where ambientAuth follows the protocol actually in force rather than the preset flag — an entry that overrides providers.bedrock.protocol to a non-signing protocol needs a token again, and an api_key_cmd from #605 satisfies the requirement exactly as a static key does. Every key-based provider keeps its requirement — TestNonAmbientProviderStillRequiresAPIKey pins that.

aws_region and aws_profile are real fields on the provider entry, JSON-serializable under providers.bedrock, not read purely from the env chain. Pinning them is what makes a run reproducible without exporting AWS_PROFILE first, which matters most on CI runners that carry a different default.

While wiring that up I found a bug worth fixing here rather than later: the fields were readable by the resolver but absent from the ProviderEntry struct the CLI marshals. Config is unmarshalled into that struct 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 or ocr config set — no error, and nothing to explain why the next review reached a different region. TestConfigRoundTripKeepsAWSSettings is the regression test.

2. Interactive wizard. Keyed off AmbientAuth, never off a provider name. For an ambient provider the model step is the last one — the wizard confirms there instead of advancing to an API-key prompt that has to be left blank, which reads as a step the user failed to complete. Before this, bedrock was unreachable through ocr config provider entirely: applyOfficialProviderConfig rejected the empty key. That gate is now a named checkAPIKeyRequirement so the ambient case is explicit and testable, and apiKeyStepCanConfirm also accepts an empty value for an ambient provider, which is the state reachable when an existing config is edited.

Yes, it still runs the connection test at the end — that is exactly where an expired SSO session surfaces, and the cheapest moment to catch it.

What is not in this PR: collecting region and profile as two optional inputs in that step. It needs a real design decision about the wizard's shape (placeholders showing what the AWS chain currently resolves, so blank reads as "inherit us-west-2" rather than "unset"), and I would rather agree on it with you than guess. Until then the wizard configures bedrock and leaves both to the AWS chain, and ocr config set covers pinning them. Happy to add it as a follow-up commit here.

3. Non-interactive config. Yes, both need to be settable, and they now are:

ocr config set providers.bedrock.aws_region us-west-2
ocr config set providers.bedrock.aws_profile example-profile

One applyProviderField case covers providers.* and custom_providers.*, since setProviderValue and setCustomProviderField both funnel there. Values are trimmed; whitespace inside one is rejected. Region names are 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 with a clear message (see Q5). Setting either field on a provider that authenticates by api_key is an error rather than stored dead config, which catches providers.anthropic.aws_region typos; for a custom provider it is accepted once the entry declares the bedrock protocol.

The unknown-config-key message is pinned byte-for-byte by an existing test, so it is updated for the two new fields and for anthropic-bedrock as a protocol value.

4. Model list semantics. The preset ships a Models list, but it should be read as a starting point, not a closed set, and free-form entry has to keep working — the existing custom-model path in the model picker already provides it. Three reasons Bedrock cannot be validated like a hosted API:

• Inference profile IDs differ per account and per region.
• An application inference profile ARN is account-specific and long, and is the right value when usage has to be attributed for cost allocation.
• Suffix conventions vary per family, so IDs cannot be derived. us.anthropic.claude-sonnet-5 is correct while us.anthropic.claude-sonnet-5-v1:0 is rejected outright. The listed IDs are taken verbatim from aws bedrock list-inference-profiles on a live account rather than inferred, and global.* variants are listed beside us.* since either is a valid routing target.

So an ID missing from the preset must not be rejected. That is now implemented rather than merely argued: preset.Models still gates a --model override for key-based providers — a typo against a hosted API is worth catching locally — but not for an ambient-auth preset, where the list is a picker for ocr config model and nothing more. Before that change, --model with an application inference profile ARN failed locally with "is not available for provider", which contradicted this very section.

5. Connection-test messaging. Bedrock's own wording sends people after the wrong problem, so failures are classified before falling through to a generic error. Real output from this branch:

$ ocr llm test
Source: provider:bedrock
Region: us-east-1
Profile: example-profile
Model:  claude-sonnet-5
✓ Connection test successful

The URL line is replaced by region and profile, because bedrock has no configured URL — the region decides the host, and a request that reached the wrong one otherwise fails as though the model ID were malformed.

Failure	What the user sees
Unresolvable profile / no AWS config	"bedrock uses the standard AWS credential chain — set AWS_PROFILE, or run aws sso login --profile NAME"
Expired session (ExpiredToken, refresh failure)	Names the profile in the aws sso login suggestion
AccessDeniedException: You don't have access to the model…	Model access is granted per account and per region in the console; an IAM policy alone does not enable it
AccessDeniedException naming bedrock:InvokeModel	"credentials resolved, so this is an authorization gap" — the IAM side
Invalid model identifier	Points at aws bedrock list-inference-profiles --region <r> and calls out the -v1:0 suffix trap
Anything else (ValidationException on max_tokens, a reset, a throttle)	Keeps the service's own wording, with the region and profile appended
Invalid API Key format	Explains that no api_key applies to bedrock and that a bearer token reached the request; if AWS_BEARER_TOKEN_BEDROCK is set, names that variable specifically

Two notes on that last row. It is checked first, because it occurs with otherwise-valid credentials and a later "expired"/"denied" branch would mislabel it. And the SDK reports the pre-middleware /v1/messages path in its error text, which makes correct path rewriting look broken — the surrounding message now says which region and profile were actually used, so the URL is not the only context available.

Every other protocol shares this client type, so the translation is gated on the bedrock flag and returns other errors untouched (TestExplainErrorLeavesNonBedrockErrorsAlone).

On the SDK bug

Per your note, the workaround stays in this patch: bedrock.WithConfig prefers bearer auth whenever cfg.BearerAuthTokenProvider is non-nil, and LoadDefaultConfig populates it from the SSO token cache — so an SSO-authenticated caller silently sends its OIDC access token and gets 403 Invalid API Key format. The provider is therefore cleared before WithConfig runs.

There is a second, smaller SDK problem worth knowing about, which I hit while reviewing my own patch. WithConfig's doc comment says the environment variable takes precedence:

Authentication is determined as follows: if the AWS_BEARER_TOKEN_BEDROCK environment variable is set, it is used for bearer token authentication. Otherwise, if cfg.BearerAuthTokenProvider is set, it is used.

The code does the opposite — it consults the variable only if cfg.BearerAuthTokenProvider == nil. My first version of this patch trusted the comment and cleared the provider only when the variable was unset, which meant an SSO user who deliberately set a Bedrock API key still sent the SSO token. Clearing unconditionally is what actually delivers the documented precedence, so that is what this does. Glad to open the anthropic-sdk-go issue for both points separately; say the word if you would rather this patch shrink once they land.

bedrock.WithLoadDefaultConfig also panics when AWS config cannot be loaded, so the config is loaded directly and the failure deferred to the first request as a sentence naming the likely fix — a CLI should not answer an expired session with a stack trace.

Verification

• Full suite passes under -race; go vet clean and gofmt -s clean.
• New tests: protocol registration, preset shape, resolution with no api_key, AWS settings reaching the client, the api_key requirement surviving for non-ambient providers, deferred-panic behaviour, config round-trip preserving the AWS fields, config set accept/reject/trim cases, the wizard skipping the key step, and each error classification.
• End-to-end against a live Bedrock account: reviews complete using SigV4 credentials from an SSO profile with no AWS variables in the environment; a -v1:0 model ID and an unresolvable profile each produce their intended message rather than a bare 400 or 403.

Rebased on current main, so it carries the finalizeResolvedEndpoint refactor, the per-run provider/model overrides from #687, and the api_key_cmd / auth_token_cmd resolution from #605. That last one overlaps this patch — both change the credential gate — and the overlap surfaced two defects a clean textual merge would have shipped:

• The site that runs api_key_cmd assumed an empty api_key implied a command was set, which held while api_key was mandatory. With ambient auth both are legitimately empty, so bedrock ran an empty command and failed with api_key_cmd for provider "bedrock" produced empty output. It is now gated on a non-empty command.
• cloneProviderEntry did not copy aws_region / aws_profile, so editing a provider through the wizard silently dropped a pinned region or profile. The reflect-based TestCloneProviderEntry_CopiesEveryField now on main caught it; the clone copies both fields and the fixture sets them.

The resolver change itself is down to one completeness condition and one gate.

[cru-Luis-Rodriguez]

Pushed a third commit (0da3406) after reviewing the first two adversarially. Four defects, each verified by execution or against SDK source rather than inferred — flagging them here so the delta is visible rather than buried in a force-push. The description above is updated to match.

1. AWS_BEARER_TOKEN_BEDROCK was unreachable for exactly the users it should serve. I had cleared cfg.BearerAuthTokenProvider only when the variable was unset, trusting WithConfig's doc comment that the variable takes precedence. The code does the opposite — it reads the variable only if cfg.BearerAuthTokenProvider == nil. So an SSO profile plus a deliberately set Bedrock API key still sent the SSO OIDC token, and the error message then blamed a token that never left the machine. Cleared unconditionally now, which is what actually delivers the documented precedence. Details in the SDK section of the description.

2. A model the account has not enabled was reported as an IAM problem. Bedrock returns AccessDeniedException for both, but You don't have access to the model with the specified model ID is fixed by enabling model access in the console, per account and per region — no IAM policy provides it. The specific wording is now matched ahead of the generic code; my clause for it had been stranded in an unreachable branch.

3. A bare ValidationException match over-triggered. Input is too long for requested model was answered with "go list your inference profiles". Only the model-identifier wording is matched now; everything else keeps the service's own message. Same treatment for the credential-expiry arm, which had matched a bare expired and so claimed x509: certificate has expired was an SSO problem.

4. --model rejected identifiers absent from the preset list, which contradicted the Q4 answer in this very description. A preset list cannot be an allowlist for Bedrock: IDs are account- and region-scoped, and an application inference profile ARN can never appear in a list compiled upstream. The list no longer gates an override for an ambient-auth provider; key-based providers keep the check.

Also dropped a cfg.URL normalization block that could not have had any effect (WithConfig is appended last and installs its own base URL), pinned AWS_CONFIG_FILE in the one test that was reading the developer's real ~/.aws/config, and fixed a column alignment in ocr llm test.

New tests cover each: the two AccessDeniedException shapes with their verbatim service wording, a request-shape ValidationException falling through to the generic message, the TLS-certificate case, and --model accepting an ARN for bedrock while still rejecting a typo for a key-based provider. Suite, go vet and gofmt clean; ocr llm test re-verified against a live account, and an identifier the preset does not list now reaches Bedrock and gets Bedrock's own verdict instead of a local rejection.

[github-actions]

🔍 OpenCodeReview found 3 issue(s) in this PR.

• ✅ Successfully posted inline: 2 comment(s)
• ❌ Failed to post inline: 1 comment(s)

---

[documentation · low]

📄 internal/llm/protocol.go (L60-L61)

⚠️ GitHub could not post this as an inline comment: Unprocessable Entity: "Line could not be resolved" - https://docs.github.com/rest/pulls/reviews#create-a-review-for-a-pull-request

The comment says "accepts the three canonical protocol names" but there are now four supported protocols (including ProtocolAnthropicBedrock). Update the comment to reflect the new count.

💡 Suggested Change

Before:

// ValidateProtocol accepts the three canonical protocol names and rejects
// everything else.

After:

// ValidateProtocol accepts the four canonical protocol names and rejects
// everything else.

[cru-Luis-Rodriguez]

Addressed all three findings from the OCR run above in 68d250c, with replies in the two inline threads.

The third one could not be posted inline ("Line could not be resolved"), so for the record: it was right. ValidateProtocol said "the three canonical protocol names" while accepting four. Fixed, and the internal/llm package comment now lists anthropic-bedrock among the supported protocols too — it had the same omission, one the bot did not flag.

Worth noting on the second finding (ValidationException too broad): that run reviewed fdd1ad2, one commit behind, and 0da3406 had already narrowed it. So the finding was accurate against the commit it saw. The first finding — ambient auth read off the preset while the protocol can be overridden per entry — was live and is the substantive fix here; it also existed on the resolver side, where it let an overridden entry resolve with no credentials at all.

Suite, go vet and gofmt clean; ocr llm test re-verified against a live Bedrock account after the change.

[lizhengfeng101]

CI / test (pull_request)
CI / test (pull_request)Failing after 39s

@cru-Luis-Rodriguez

[cru-Luis-Rodriguez]

Thanks for the ping — fixed in 273357a.

The failure was the govulncheck step, not a test. GO-2026-5764 affects github.com/aws/aws-sdk-go-v2/aws/protocol/eventstream v1.6.3, and the trace runs through this PR's own new dependency (llm.init → bedrock.init → eventstream.init), so it's on me to clear it.

aws-sdk-go-v2/config is the only direct AWS import here, so bumping it from v1.27.27 to v1.32.34 pulls the tree forward and resolves eventstream to v1.7.16, past the v1.7.8 fix. smithy-go moves to v1.27.6 as its runtime companion; internal/ini drops out and internal/v4a / service/signin come in, both internal restructuring within aws-sdk-go-v2 itself. No non-AWS dependency moves — the diff is go.mod and go.sum only.

On why the core module jumps so many minors: there's no supported way to move eventstream past v1.7.8 while holding the 2024-era pins, since MVS resolves it from the direct dependency. This is the minimal coherent bump.

Verified locally against the CI configuration (go1.26.5): go build ./..., gofmt -s, go vet, and go mod tidy are all clean, go test -race -count=1 ./... passes across all packages, and govulncheck ./... reports 0 affecting vulnerabilities.

One note for a separate PR: govulncheck still reports GO-2026-5970 (golang.org/x/text v0.37.0) and GO-2026-5942 (golang.org/x/net v0.55.0) as imported-but-not-called. Neither affects the exit code and both pre-date this PR, so I left them out to keep this diff scoped to the AWS tree — happy to open a follow-up if you'd like them bumped.

[lizhengfeng101]

@cru-Luis-Rodriguez CI is failing due to missing SPDX license headers on the two new test files:

cmd/opencodereview/bedrock_config_test.go (missing SPDX identifier)
internal/llm/bedrock_test.go (missing SPDX identifier)

The merge conflict has been resolved on our side. Could you rebase on main and run make license-add to fix the headers?

[yq314]

Just checking in, when can we get this merged?

[cru-Luis-Rodriguez]

Rebased onto current main (0c8c067) — conflicts resolved, all five commits preserved.

On make license-add: already done, back in the 8/7 rebase. Both new test files carry the SPDX identifier and copyright line, make license-check reports "All source files have valid license headers," and the license step is green in CI. Nothing outstanding there.

The conflicts this round came entirely from the retry work that landed since — retry_codes (#818), the session-key prompt-cache affinity (#332), and the retry report (#790). Those added RetryCodes, SessionKey and retryCollector to ClientConfig and moved NewLLMClient to take a collector. Most of the merge was mechanical.

One part was not, and it's the piece worth your eye. NewAnthropicBedrockClient built its own client and applied ExtraHeaders at construction time, so it would have silently skipped all three of those features — a Bedrock endpoint would have ignored retry_codes and gone missing from the retry report. It now defaults cfg.SessionKey and installs retryCodesMiddleware and newRetryObserver as middlewares, with bedrock.WithConfig still appended last so SigV4 signing wraps them. That's new behavior rather than a conflict resolution, so I'd rather name it here than have it read as a mechanical rebase.

Green across the board — test, CodeQL, and all five cross-compile targets.

[lizhengfeng101]

merge conflicts

[cru-Luis-Rodriguez]

@lizhengfeng101
Rebased onto current main (533f736) — the branch went conflicting once #605, #687 and the two new provider presets landed. What the conflicts were and how they were resolved:

• Credential gate (internal/llm/resolver.go) — feat(config): resolve api_key/auth_token from a command (#236) #605 added api_key_cmd to the same gate this patch relaxes for ambient auth. It now reads apiKey == "" && apiKeyCmd == "" && !ambientAuth, with your error wording kept verbatim.
• Completeness check — the ambient-auth condition carried over to the new finalizeResolvedEndpoint(name, ep, env) signature.
• checkAPIKeyRequirement — now takes apiKeyCmd, so a configured command satisfies the wizard the same way a static key does. That was feat(config): resolve api_key/auth_token from a command (#236) #605's behaviour at the call site this patch replaced, so it had to move into the function rather than be dropped.
• Provider list and the two byte-pinned help strings — unions, not either/or: api_key_cmd alongside aws_region, aws_profile, and anthropic-bedrock alongside the existing protocol values.

Two defects came out of the overlap with #605 rather than out of the text conflict, and both are fixed here with tests:

• The site that runs api_key_cmd assumed an empty api_key implied a command was set — true while an api_key was mandatory. Under ambient auth both are legitimately empty, so bedrock ran an empty command and failed with api_key_cmd for provider "bedrock" produced empty output. Now gated on a non-empty command.
• cloneProviderEntry did not copy aws_region / aws_profile, so editing a provider through the wizard silently dropped a pinned region or profile. Your reflect-based TestCloneProviderEntry_CopiesEveryField caught it — a good test; it found this before I did.

Verified on the rebased branch: go test -race -count=1 ./..., go vet, gofmt -s, go mod tidy (no diff), line endings, and the license / action-pin / english-only scripts all pass.

The description is updated for the new gate and for what the rebase carries.