mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-08-20 06:04:11 +00:00
9c5e90d0b1
* feat(providers): add AWS Bedrock as a built-in provider
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) <noreply@anthropic.com>
* feat(config): configure and diagnose an ambient-auth provider from the CLI
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.<name>.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) <noreply@anthropic.com>
* fix(bedrock): correct bearer-token precedence, error triage, and model gating
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) <noreply@anthropic.com>
* fix(bedrock): honour an entry-level protocol override for ambient auth
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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
* fix(bedrock): correct error triage, ambient credential handling, and the protocol's reach
Addresses the six pre-merge items from review.
- "not authorized to invoke this API operation" sat in the model-access
branch. It is IAM's own wording, so it pointed at a console toggle when
the fix is a policy change; it now falls to the AccessDenied branch,
whose message names bedrock:InvokeModel. A test case carries the phrase
without "AccessDenied" in the text, so the phrase itself is pinned to
the authorization branch rather than the exception name.
- TestExplainErrorClassifiesBedrockFailures read the ambient
AWS_BEARER_TOKEN_BEDROCK and asserted the message produced when it is
unset, so the suite failed on any machine that exports one. It pins the
variable empty. Reproduced before the fix with
AWS_BEARER_TOKEN_BEDROCK=sk-x go test -run TestExplainError.
- api_key_cmd ran for an ambient-auth provider. The output is discarded by
a signed request, and the command is typically a secret-manager read, so
a bedrock user got a real 1Password / Touch ID prompt for nothing — while
the comment above the call claimed it could not happen. Gated on
!ambientAuth; the new test proves non-execution with a sentinel file, and
fails without the gate.
- llm.protocol and OCR_LLM_PROTOCOL validated anthropic-bedrock and then
ignored it. Both strategies describe one URL and one token, have nowhere
to carry a region or a profile, and bedrock uses neither value they do
carry, so the request would have been signed and re-hosted with the rest
of the block silently dropped. Both now reject it, as does
`ocr config set llm.protocol`, at the point the value is typed.
- The custom-provider contract was split in two: the TUI never offered
bedrock, while the resolver demanded a url the bedrock client never
reads. Settled toward supporting it — a provider entry is the one place
with somewhere to put aws_region and aws_profile, which is what lets a
second region or profile have its own entry. url is now required for
every protocol except bedrock, the Custom form offers bedrock and ends at
the protocol step (there is no url, api key or auth header to collect),
and switching an existing entry to it clears the three fields the
previous protocol needed rather than leaving them as dead config. The
Manual form keeps the three-protocol list, since it writes llm.url and
llm.auth_token.
- Documented the bedrock.WithConfig append site: options wrap in order, so
appending last leaves signing innermost — closest to the wire, and re-run
on each retry rather than replaying a stale signature.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* docs(pages): document the bedrock provider in all four locales
The provider shipped with no documentation: no row in the built-in table, and
no mention of aws_region or aws_profile, so the only way to learn either field
existed was to read the resolver.
Adds to en, ja, ru and zh:
- a `bedrock` row in the built-in provider table, with the host derived from
aws_region and no API key env var, since neither applies;
- an "AWS Bedrock" section covering the two AWS fields and what each falls
back to, why model IDs are not validated against the shipped list (they are
scoped to an account and a region, and an application inference profile ARN
can never appear in a list compiled upstream), the `-v1:0` suffix trap, and
the region/profile lines `ocr llm test` prints in place of a URL;
- anthropic-bedrock in the custom-provider protocol list, with the example
that needs no url and takes the same AWS fields — the supported way to run
a second region or profile;
- a note that llm.protocol and OCR_LLM_PROTOCOL reject it, and why.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(bedrock): clear stale AWS settings on protocol switch, bound config load
applyProviderField's "protocol" case only validated and wrote the new
protocol, leaving aws_region/aws_profile behind when an entry switched
away from anthropic-bedrock. Setting the AWS fields first (while the
entry was still ambient) and only then switching protocol produced
exactly the dead config the aws_region/aws_profile write path already
guards against on the other ordering. Clear both fields when the new
protocol isn't anthropic-bedrock, with a stderr warning, mirroring the
TUI's cpAmbientProtocol() cleanup.
Also bound NewAnthropicBedrockClient's awsconfig.LoadDefaultConfig with
a context timeout instead of context.Background(), as defense in
depth against network calls region auto-detection can still make.
Credential resolution itself (SSO refresh, AssumeRole,
credential_process) is lazy and already bounded by cfg.Timeout at
request time, so this does not fix an observed hang, but removes an
unbounded context where the AWS SDK's own defaults are the only guard.
---------
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: kite <lizhengfeng.lzf@alibaba-inc.com>
|
||
|---|---|---|
| .. | ||
| opencodereview | ||