[codex] wire native Bedrock provider#581
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
|
@codex review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a native AWS Bedrock provider, replacing the previous OpenAI-compatible implementation. It features a sophisticated model ID parser for ARNs and inference profiles, implements SigV4 signing, and updates configuration to use standard AWS environment variables. Review feedback suggests optimizing performance by avoiding a redundant request clone and improving the model family identification logic to correctly handle region-prefixed IDs.
| let mut execution_request = request.clone(); | ||
| execution_request.model = execution_model_id(&request.model); |
There was a problem hiding this comment.
Cloning the entire ChatRequest here is redundant and potentially expensive for large requests. The downstream functions execute_converse and execute_invoke (specifically seen in invoke.rs) already perform model ID parsing on the provided request. You can pass the original request by reference directly.
match model_config.api_type {
BedrockApiType::Converse | BedrockApiType::ConverseStream => {
converse::execute_converse(client, request).await
}
BedrockApiType::Invoke | BedrockApiType::InvokeStream => {
invoke::execute_invoke(client, request).await
}
}There was a problem hiding this comment.
I am leaving this one unchanged in this PR. The Invoke path already parses the execution model ID, but the Converse path currently sends request.model directly as the Bedrock modelId. Removing the cloned execution request without moving that parsing into converse.rs would regress regional/profile IDs. VibeGuard blocks edits to converse.rs because it is already over the file-size limit, so this should be handled with the Converse split/refactor rather than in this landing PR.
| let family_hint = metadata_lookup_ids | ||
| .iter() | ||
| .find_map(|id| id.split_once('.').map(|(family, _)| family.to_string())); |
There was a problem hiding this comment.
The current logic for family_hint will return geo or region prefixes (like us or us-east-1) as the family name if they are present in the model ID. Iterating in reverse starting from the canonical ID (which has prefixes stripped) will provide a more accurate hint for the actual model provider (e.g., anthropic).
| let family_hint = metadata_lookup_ids | |
| .iter() | |
| .find_map(|id| id.split_once('.').map(|(family, _)| family.to_string())); | |
| let family_hint = metadata_lookup_ids | |
| .iter() | |
| .rev() | |
| .find_map(|id| id.split_once('.').map(|(family, _)| family.to_string())); |
There was a problem hiding this comment.
Fixed in 071f7fb by deriving the hint from the canonical metadata fallback first and adding assertions for regional, global, and ARN profile IDs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2a0dda913d
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 071f7fba38
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .find_map(|key| env::var(key).ok()) | ||
| .filter(|value| !value.trim().is_empty()) |
There was a problem hiding this comment.
env_str_any uses find_map before filtering empties, so if AWS_REGION is defined as an empty string the lookup stops there and never checks AWS_DEFAULT_REGION; build_bedrock_config_from_factory then falls back to us-east-1. In deployments where AWS_REGION is exported but blank (common with templated env injection), Bedrock requests get signed for the wrong region and can fail with model/endpoint errors even though a valid AWS_DEFAULT_REGION is available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ec1e0bbc3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4638028159
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45e1e527ef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| &["aws_access_key_id", "aws_access_key", "access_key"], | ||
| ) | ||
| .map(str::to_string) | ||
| .or_else(|| env_str_any(&["AWS_ACCESS_KEY_ID"])) | ||
| .ok_or_else(|| { |
There was a problem hiding this comment.
This lookup prefers config values even when they are empty strings, so a blank aws_access_key_id/aws_secret_access_key in settings prevents fallback to AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY. In templated deployments where empty strings are emitted for unset config values, provider initialization fails with missing-credential errors even though valid environment credentials are present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc1d2accd8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1ab85e8840
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 96182a46e0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| let aws_session_token = config_str_any(config, &["aws_session_token", "session_token"]) | ||
| .map(str::to_string) | ||
| .or_else(|| env_str_any(&["AWS_SESSION_TOKEN"])); |
There was a problem hiding this comment.
The Bedrock config builder prefers settings.aws_session_token even when it is an empty string, so templated configs that emit "aws_session_token": "" suppress fallback to AWS_SESSION_TOKEN. With temporary AWS credentials this produces an empty x-amz-security-token header instead of the real token from env, causing SigV4-authenticated Bedrock requests to fail.
Useful? React with 👍 / 👎.
Signed-off-by: majiayu000 <1835304752@qq.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c9781a285
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Signed-off-by: majiayu000 <1835304752@qq.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0324d2e032
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| matches!( | ||
| prefix, | ||
| "global" | "us" | "eu" | "ap" | "apac" | "sa" | "ca" | "me" | "af" | ||
| ) |
There was a problem hiding this comment.
is_geo_prefix omits jp and au, so valid geography-scoped IDs like jp.<model> or au.<model> are not canonicalized to the base model ID during parse_bedrock_model_id. In the new native routing path, get_model_config_for_model_id depends on that canonicalization for metadata lookup, so these model IDs can now fail with Model ... not supported before dispatch even though Bedrock accepts them as geo inference profile IDs.
Useful? React with 👍 / 👎.
…al chain - chat/invoke.rs: route_chat_request already normalizes request.model to the execution ID before calling execute_invoke, so the second parse_bedrock_model_id call inside invoke.rs is dead work. Use the pre-normalized model directly and document the invariant. - factory/builder.rs: stop hard-erroring when aws_access_key_id / aws_secret_access_key are missing from both config and env. Pass them through as empty strings so the AWS SigV4 credential chain (IAM instance profile, IRSA, ~/.aws/credentials) can resolve at request time. Region is now the only mandatory field.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e408f072da
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| .or_else(|| env_str_any(&["AWS_ACCESS_KEY_ID"])) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
This builder now uses unwrap_or_default() for missing AWS credentials, but the newly wired native Bedrock path validates credentials eagerly (BedrockConfig::validate/AwsAuth::validate) and rejects empty values, so setups that rely on IAM role/credential-chain resolution (no static keys in config/env) fail during provider initialization before any request is attempted.
Useful? React with 👍 / 👎.
| .ok_or_else(|| { | ||
| ProviderError::configuration( | ||
| "bedrock", | ||
| "aws_region (or AWS_REGION / AWS_DEFAULT_REGION) is required", | ||
| ) |
There was a problem hiding this comment.
The builder hard-fails when neither aws_region nor region env vars are set, which causes startup errors for otherwise valid Bedrock configs that omit region; this is inconsistent with the existing Bedrock default region behavior (us-east-1) and turns an optional setting into a required one after the native-provider wiring.
Useful? React with 👍 / 👎.
1 participant
Summary
ProviderType::Bedrockto the native SigV4BedrockProviderinstead of the OpenAI-like compatibility path.bedrock/is only treated as a local selector while AWS execution IDs preserveus.*,global.*, region-like IDs, and ARNs.openai_compatible, with separate native/proxy provider docs.Why
Issue #6 started as a missing
bedrock/...routing example, but the long-term design in PR #560 split native AWS Bedrock from OpenAI-compatible Bedrock proxies. This PR lands the foundation pieces from that design without mixing proxy semantics into native Bedrock.Closes #574.
Closes #575.
Closes #578.
Refs #6.
Refs #576.
Refs #577.
Refs #579.
Validation
cargo fmtgit diff --checkcargo check --libcargo test core::providers::bedrock::model_id --libcargo test normalize_bedrock_model_id --libcargo test bedrock --libcargo test factory --libcargo test --test lib provider_factory -- --nocapturecargo test