feat(provider): add aws bedrock#54
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds Amazon Bedrock support: new Bedrock types, provider implementation with AWS SigV4 signing and AwsStatic credentials, AWS EventStream reader, OpenAI↔Bedrock transform logic, PreparedRequest/prepare_request hooks, config/schema updates, and extensive tests/fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway as Proxy (Gateway)
participant Provider as BedrockDef
participant Signer as AWS SigV4 Signer
participant Bedrock as Bedrock Upstream
participant StreamReader as AWS EventStream Reader
participant Transformer as ChatTransform (BedrockDef)
Client->>Gateway: POST /v1/chat/completions (OpenAI JSON)
Gateway->>Gateway: build PreparedRequest(method,url,headers,body,stream)
Gateway->>Provider: prepare_request(PreparedRequest, AwsStaticCredentials)
activate Provider
Provider->>Signer: generate SigV4 signature (method,url,headers,body,creds)
Signer-->>Provider: signed headers
Provider->>Provider: rewrite URL to /converse-stream if stream
Provider-->>Gateway: PreparedRequest (signed)
deactivate Provider
Gateway->>Bedrock: send signed request
Bedrock-->>Gateway: EventStream (stream) / JSON (non-stream)
Gateway->>StreamReader: aws_event_stream_reader(stream)
StreamReader-->>Gateway: JSON event objects / frames
Gateway->>Transformer: parse_bedrock_stream_to_openai/frame → chunks OR transform_response_with_request
Transformer-->>Gateway: ChatCompletionChunk(s) / ChatCompletionResponse
Gateway-->>Client: SSE/OpenAI-formatted streamed chunks or final JSON response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/gateway/providers/mod.rs (2)
34-41:⚠️ Potential issue | 🟡 MinorAdd
#[fastrace::trace]annotation to public function.The function
default_provider_registryat line 34 is public insrc/gateway/providers/mod.rsand must include the#[fastrace::trace]decorator for distributed tracing, per coding guidelines forsrc/**/*.rs.🔧 Suggested fix
+#[fastrace::trace] pub fn default_provider_registry() -> Result<ProviderRegistry> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/providers/mod.rs` around lines 34 - 41, The public function default_provider_registry needs the #[fastrace::trace] attribute for distributed tracing; add the #[fastrace::trace] annotation immediately above the pub fn default_provider_registry() -> Result<ProviderRegistry> declaration so the function is instrumented (no other behavior changes required).
48-57:⚠️ Potential issue | 🟡 MinorUse
pretty_assertions::assert_eqin thissrctest module.The test currently uses plain
assert_eq!macros instead ofpretty_assertions::assert_eqfor better assertion output, as required by the coding guidelines.🔧 Suggested fix
#[cfg(test)] mod tests { use super::default_provider_registry; + use pretty_assertions::assert_eq;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/providers/mod.rs` around lines 48 - 57, The test default_provider_registry_registers_builtin_providers uses the standard assert_eq! macros; import and use the pretty_assertions assertion macro instead to get prettier diffs: add a use/import of pretty_assertions::assert_eq (or otherwise bring the pretty_assertions macro into scope) in the test module and keep the existing equality checks (in function default_provider_registry_registers_builtin_providers) so they call the pretty_assertions version rather than the std one.src/proxy/provider.rs (1)
78-96:⚠️ Potential issue | 🟠 MajorBedrock endpoint overrides should not allow arbitrary cleartext HTTP.
This helper now gates Bedrock
endpointtoo, sohttp://...lets SigV4 auth andx-amz-security-tokentravel in cleartext to non-local hosts. Please requirehttpsfor normal endpoints and only keephttpfor loopback/test hosts.🔒 Suggested tightening
fn parse_base_url(api_base: Option<&str>) -> Result<Option<Url>> { match api_base { Some(api_base) => { let parsed = Url::parse(api_base).map_err(|error| { GatewayError::Internal(format!("invalid provider api_base {}: {}", api_base, error)) })?; - if !matches!(parsed.scheme(), "http" | "https") { + let is_loopback = matches!(parsed.host_str(), Some("localhost" | "127.0.0.1" | "::1")); + if parsed.scheme() != "https" && !(parsed.scheme() == "http" && is_loopback) { return Err(GatewayError::Internal(format!( - "invalid provider api_base {}: unsupported scheme {}", + "invalid provider api_base {}: unsupported scheme {}", api_base, parsed.scheme() ))); } Ok(Some(parsed)) } None => Ok(None), } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proxy/provider.rs` around lines 78 - 96, The parse_base_url function currently allows "http" for any host; change it to require "https" for non-local endpoints by rejecting "http" unless the parsed host is a loopback/test address (e.g., "localhost", "127.0.0.1", "::1" or a loopback IP). Update the scheme validation in parse_base_url to: if scheme == "https" accept, if scheme == "http" only accept when parsed.host() indicates a loopback/local address, otherwise return a GatewayError::Internal describing unsupported cleartext scheme for non-local hosts; keep existing error formatting and use parsed.scheme()/parsed.host() to construct the message.
🧹 Nitpick comments (6)
src/gateway/streams/reader/aws_event_stream.rs (1)
26-36: Add tracing to the new public reader entrypoint.
aws_event_stream_readeris public and sits directly on the Bedrock stream path, so it should be traced like the other public gateway entrypoints.📈 Minimal change
/// `aws_event_stream_reader` decodes AWS EventStream frames into normalized JSON lines. /// /// Each event frame is emitted as a JSON object with `type` and `payload` fields so /// provider-specific transforms can distinguish Bedrock `ConverseStream` event kinds /// without widening the shared streaming interface. Exception frames are surfaced as /// `GatewayError::Stream`, and truncated frames at EOF fail closed instead of emitting /// partial data. +#[fastrace::trace] pub fn aws_event_stream_reader<S>(stream: S) -> Pin<Box<dyn Stream<Item = Result<String>> + Send>> where S: Stream<Item = std::result::Result<Bytes, reqwest::Error>> + Send + 'static, {As per coding guidelines
src/**/*.rs: Use#[fastrace::trace]decorator for distributed tracing on public functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/streams/reader/aws_event_stream.rs` around lines 26 - 36, Add distributed tracing to the public entrypoint by annotating the aws_event_stream_reader function with the #[fastrace::trace] attribute (place it immediately above the pub fn aws_event_stream_reader declaration) and ensure the fastrace crate is available in scope (add the fastrace dependency and/or use/import as needed). This keeps the tracing consistent with other public gateway entrypoints and requires no behavior changes to the function body.tests/proxy/bedrock-chat-completions.test.ts (1)
52-54: Replace the fixed propagation sleep with a readiness poll.A blind 500ms delay makes this suite timing-dependent: slow runs can still race, and fast runs always wait longer than needed. Poll until the model/key is actually visible through the proxy instead of sleeping.
src/config/entities/models.rs (1)
283-288: Consider usingpretty_assertionsfor better test output.As per coding guidelines for
{tests,src}/**/*.rs, tests should usepretty_assertions::assert_eqfor better diff output on failures.Suggested import addition
#[cfg(test)] mod tests { + use pretty_assertions::assert_eq; use serde_json::json; use super::{SCHEMA, SCHEMA_VALIDATOR, format_evaluation_error};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config/entities/models.rs` around lines 283 - 288, Add pretty_assertions::assert_eq to the test imports and use it in place of std's assert_eq in the tests module to get improved diff output on failures; specifically, update the tests module in models.rs (the #[cfg(test)] mod tests block that currently imports serde_json::json, SCHEMA, SCHEMA_VALIDATOR, format_evaluation_error, and MODELS_PATTERN) to include use pretty_assertions::assert_eq and change any existing assert_eq calls in those tests to the imported version.src/gateway/provider_instance.rs (2)
33-36: Remove unused variable_has_session_token.The variable
_has_session_tokenis computed but never used. This appears to be leftover debug code or an incomplete feature.Suggested fix
Self::AwsStatic(credentials) => { - let _has_session_token = credentials.session_token.is_some(); f.write_str("AwsStatic(REDACTED)") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/provider_instance.rs` around lines 33 - 36, Remove the unused temporary variable by deleting the line that creates _has_session_token inside the AwsStatic match arm; keep the existing f.write_str("AwsStatic(REDACTED)") behavior in the Display/formatting code (the AwsStatic variant is the target symbol to edit).
160-173: Consider usingpretty_assertionsfor better test output.As per coding guidelines for
{tests,src}/**/*.rs, tests should usepretty_assertions::assert_eqfor better diff output on failures.Suggested import addition
#[cfg(test)] mod tests { use std::{borrow::Cow, sync::Arc}; use http::{ HeaderMap, HeaderValue, header::{AUTHORIZATION, HeaderName}, }; + use pretty_assertions::assert_eq; use super::{AwsStaticCredentials, ProviderAuth, ProviderInstance, ProviderRegistry};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/provider_instance.rs` around lines 160 - 173, Add the pretty_assertions import to the test module so test failures show improved diffs: in the #[cfg(test)] mod tests import block add use pretty_assertions::assert_eq; and then use that assert_eq in existing tests (in the tests module that references AwsStaticCredentials, ProviderAuth, ProviderInstance, ProviderRegistry, GatewayError/Result, ChatTransform, ProviderCapabilities, ProviderMeta, StreamReaderKind) so assertions produce prettier output on failure.src/gateway/providers/bedrock/transform.rs (1)
602-612: Consider usingpretty_assertionsfor better test output.As per coding guidelines for
{tests,src}/**/*.rs, tests should usepretty_assertions::assert_eqfor better diff output on failures.Suggested import addition
#[cfg(test)] mod tests { + use pretty_assertions::assert_eq; use serde_json::json; use super::{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/providers/bedrock/transform.rs` around lines 602 - 612, The tests module at the bottom (mod tests) should import and use pretty_assertions::assert_eq to get nicer diffs on failures; add a use pretty_assertions::assert_eq; inside the tests module (near the other use statements that import bedrock_to_openai_response, openai_to_bedrock_request, parse_bedrock_stream_to_openai and the crate types BedrockEmptyObject/BedrockToolChoice/ChatCompletionRequest/MessageContent) so existing test assertions use assert_eq from pretty_assertions instead of the std one.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/utils/admin.ts`:
- Around line 31-53: The helpers adminUrl, adminGet, and adminPost still use the
hardcoded base 127.0.0.1:3001 and therefore ignore the dynamic adminPort passed
to startIsolatedAdminApp; update those helpers to accept (or derive) the admin
port from the TestAppPorts returned/used by startIsolatedAdminApp (or accept an
optional adminPort argument) and build the base URL using
`127.0.0.1:${adminPort}` instead of `127.0.0.1:3001`; ensure any call sites that
call adminGet/adminPost in tests pass the adminPort or the full adminUrl so
admin requests go to the correct instance.
---
Outside diff comments:
In `@src/gateway/providers/mod.rs`:
- Around line 34-41: The public function default_provider_registry needs the
#[fastrace::trace] attribute for distributed tracing; add the #[fastrace::trace]
annotation immediately above the pub fn default_provider_registry() ->
Result<ProviderRegistry> declaration so the function is instrumented (no other
behavior changes required).
- Around line 48-57: The test
default_provider_registry_registers_builtin_providers uses the standard
assert_eq! macros; import and use the pretty_assertions assertion macro instead
to get prettier diffs: add a use/import of pretty_assertions::assert_eq (or
otherwise bring the pretty_assertions macro into scope) in the test module and
keep the existing equality checks (in function
default_provider_registry_registers_builtin_providers) so they call the
pretty_assertions version rather than the std one.
In `@src/proxy/provider.rs`:
- Around line 78-96: The parse_base_url function currently allows "http" for any
host; change it to require "https" for non-local endpoints by rejecting "http"
unless the parsed host is a loopback/test address (e.g., "localhost",
"127.0.0.1", "::1" or a loopback IP). Update the scheme validation in
parse_base_url to: if scheme == "https" accept, if scheme == "http" only accept
when parsed.host() indicates a loopback/local address, otherwise return a
GatewayError::Internal describing unsupported cleartext scheme for non-local
hosts; keep existing error formatting and use parsed.scheme()/parsed.host() to
construct the message.
---
Nitpick comments:
In `@src/config/entities/models.rs`:
- Around line 283-288: Add pretty_assertions::assert_eq to the test imports and
use it in place of std's assert_eq in the tests module to get improved diff
output on failures; specifically, update the tests module in models.rs (the
#[cfg(test)] mod tests block that currently imports serde_json::json, SCHEMA,
SCHEMA_VALIDATOR, format_evaluation_error, and MODELS_PATTERN) to include use
pretty_assertions::assert_eq and change any existing assert_eq calls in those
tests to the imported version.
In `@src/gateway/provider_instance.rs`:
- Around line 33-36: Remove the unused temporary variable by deleting the line
that creates _has_session_token inside the AwsStatic match arm; keep the
existing f.write_str("AwsStatic(REDACTED)") behavior in the Display/formatting
code (the AwsStatic variant is the target symbol to edit).
- Around line 160-173: Add the pretty_assertions import to the test module so
test failures show improved diffs: in the #[cfg(test)] mod tests import block
add use pretty_assertions::assert_eq; and then use that assert_eq in existing
tests (in the tests module that references AwsStaticCredentials, ProviderAuth,
ProviderInstance, ProviderRegistry, GatewayError/Result, ChatTransform,
ProviderCapabilities, ProviderMeta, StreamReaderKind) so assertions produce
prettier output on failure.
In `@src/gateway/providers/bedrock/transform.rs`:
- Around line 602-612: The tests module at the bottom (mod tests) should import
and use pretty_assertions::assert_eq to get nicer diffs on failures; add a use
pretty_assertions::assert_eq; inside the tests module (near the other use
statements that import bedrock_to_openai_response, openai_to_bedrock_request,
parse_bedrock_stream_to_openai and the crate types
BedrockEmptyObject/BedrockToolChoice/ChatCompletionRequest/MessageContent) so
existing test assertions use assert_eq from pretty_assertions instead of the std
one.
In `@src/gateway/streams/reader/aws_event_stream.rs`:
- Around line 26-36: Add distributed tracing to the public entrypoint by
annotating the aws_event_stream_reader function with the #[fastrace::trace]
attribute (place it immediately above the pub fn aws_event_stream_reader
declaration) and ensure the fastrace crate is available in scope (add the
fastrace dependency and/or use/import as needed). This keeps the tracing
consistent with other public gateway entrypoints and requires no behavior
changes to the function body.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 237bdabc-5c9f-43b0-ad7a-f1cafd75f13c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (23)
Cargo.tomlbuild.rssrc/config/entities/models.rssrc/gateway/gateway.rssrc/gateway/provider_instance.rssrc/gateway/providers/bedrock.rssrc/gateway/providers/bedrock/transform.rssrc/gateway/providers/mod.rssrc/gateway/streams/mod.rssrc/gateway/streams/reader/aws_event_stream.rssrc/gateway/streams/reader/mod.rssrc/gateway/streams/reader/sse.rssrc/gateway/traits/mod.rssrc/gateway/traits/provider.rssrc/gateway/types/bedrock.rssrc/gateway/types/mod.rssrc/lib.rssrc/proxy/provider.rstests/fixtures/bedrock-mock-upstream.tstests/proxy/bedrock-chat-completions.test.tstests/proxy/hook/bedrock-rate-limit.test.tstests/utils/admin.tstests/utils/bedrock-mock-upstream.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/proxy/bedrock-chat-completions.test.ts (2)
29-31: Replace fixed propagation sleep with readiness pollingLine 29-Line 31 uses a hardcoded 500ms wait, which can make this test intermittently fail in slower environments. Prefer polling with a timeout until model/key config is actually ready.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/proxy/bedrock-chat-completions.test.ts` around lines 29 - 31, Replace the fixed 500ms sleep in the waitConfigPropagation helper with a polling loop that repeatedly checks for actual readiness of the model/key configuration until a configurable timeout is reached; update the waitConfigPropagation function to call the service or endpoint that confirms config readiness (or use an existing readiness helper) in short intervals (e.g., 50–100ms) and resolve when it returns success, otherwise reject/throw after the timeout so tests fail fast and deterministically. Use the function name waitConfigPropagation to locate and replace the setTimeout-based implementation and ensure tests that rely on it still await the returned promise.
142-144: Add stream-path assertion forx-amz-security-tokenFor parity with the non-stream test, also assert the session token header on the
/converse-streamrequest. It tightens signing coverage for the streaming code path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/proxy/bedrock-chat-completions.test.ts` around lines 142 - 144, Add an assertion that the `/converse-stream` request includes the AWS session token header: after the existing checks on recorded[0]?.url and recorded[0]?.headers.authorization, add an assertion that recorded[0]?.headers['x-amz-security-token'] equals the same session token used in the non-stream test (or at minimum is defined). This uses the same recorded array and EXPECTED_ENCODED_PATH with the '/converse-stream' entry so the streaming code path's signing is verified.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/proxy/bedrock-chat-completions.test.ts`:
- Line 53: The test is currently including the full admin response in the
assertion message (expect(modelResp.status,
JSON.stringify(modelResp.data)).toBe(201)), which can leak sensitive data;
update the assertions in bedrock-chat-completions.test.ts to avoid serializing
modelResp.data (and the similar assertion at the other location) by using only
the status or a redacted message as the failure context (e.g., use
modelResp.status or a short string like "response status != 201") so references
to modelResp in the expect call no longer stringify the full payload.
---
Nitpick comments:
In `@tests/proxy/bedrock-chat-completions.test.ts`:
- Around line 29-31: Replace the fixed 500ms sleep in the waitConfigPropagation
helper with a polling loop that repeatedly checks for actual readiness of the
model/key configuration until a configurable timeout is reached; update the
waitConfigPropagation function to call the service or endpoint that confirms
config readiness (or use an existing readiness helper) in short intervals (e.g.,
50–100ms) and resolve when it returns success, otherwise reject/throw after the
timeout so tests fail fast and deterministically. Use the function name
waitConfigPropagation to locate and replace the setTimeout-based implementation
and ensure tests that rely on it still await the returned promise.
- Around line 142-144: Add an assertion that the `/converse-stream` request
includes the AWS session token header: after the existing checks on
recorded[0]?.url and recorded[0]?.headers.authorization, add an assertion that
recorded[0]?.headers['x-amz-security-token'] equals the same session token used
in the non-stream test (or at minimum is defined). This uses the same recorded
array and EXPECTED_ENCODED_PATH with the '/converse-stream' entry so the
streaming code path's signing is verified.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b6982b0-50d2-426b-ad7e-7746d7780c1c
📒 Files selected for processing (4)
src/gateway/providers/bedrock.rstests/proxy/bedrock-chat-completions.test.tstests/proxy/hook/bedrock-rate-limit.test.tstests/utils/admin.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/proxy/hook/bedrock-rate-limit.test.ts
- tests/utils/admin.ts
- src/gateway/providers/bedrock.rs
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/gateway/providers/bedrock.rs (3)
25-42: Add///docs to the public Bedrock API surface.
IDENTIFIER,BedrockDef, andBedrockProviderConfigare all public items, so they should carry Rust doc comments in this module. As per coding guidelines "**/*.rs: Use /// for doc comments on public items in Rust".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/providers/bedrock.rs` around lines 25 - 42, Add Rust doc comments (using ///) to the public Bedrock API surface: add brief docs for the IDENTIFIER constant, the BedrockDef struct, and the BedrockProviderConfig struct (including short descriptions for its fields like region, access_key_id, secret_access_key, session_token, and endpoint). Update the declarations for IDENTIFIER, BedrockDef, and BedrockProviderConfig to include /// comments that explain their purpose and usage so the module follows the project's public-item documentation guideline.
70-72: Keep the full Bedrock chat route in one place.Line 70 reports
/model, but Lines 165-182 actually send/model/{model}/converse. Splitting that logic acrosschat_endpoint_path()and a custombuild_url()makes the provider metadata inaccurate and bypassesProviderMeta::build_url_for_endpoint()'s overlap handling. I’d move the full route shape intochat_endpoint_path(model)and rely on the defaultbuild_url()implementation.Also applies to: 165-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/providers/bedrock.rs` around lines 70 - 72, The chat path is split: chat_endpoint_path currently returns "/model" while a custom build_url constructs "/model/{model}/converse", causing inaccurate provider metadata and bypassing ProviderMeta::build_url_for_endpoint() overlap handling. Update chat_endpoint_path(model: &str) to return the full route shape (e.g. "/model/{model}/converse") using the model parameter, and remove or revert the custom build_url implementation so the default build_url()/ProviderMeta::build_url_for_endpoint() is used for URL composition and overlap handling.
230-349: Use the repo-standard assertion helpers in this test module.These tests still use plain
assert_eq!andmatches!. Please switch topretty_assertions::assert_eqandassert_matches::assert_matchesso failures stay consistent with the rest of the Rust test suite. As per coding guidelines "{tests,src}/**/*.rs: Use pretty_assertions::assert_eq and assert_matches::assert_matches for better test output".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/gateway/providers/bedrock.rs` around lines 230 - 349, Replace plain assert macros in the bedrock provider tests with the repo-standard helpers: add imports for pretty_assertions::assert_eq and assert_matches::assert_matches at the top of the test module, change all assert_eq! calls (e.g., in bedrock_provider_config_deserializes_static_credentials, bedrock_provider_config_debug_redacts_credentials, build_url_encodes_model_ids_with_slashes, prepare_request_rewrites_stream_url_before_signing) to use assert_eq, and replace the matches! usage in prepare_request_rejects_non_aws_auth with assert_matches (targeting GatewayError::Validation and the message predicate referencing ProviderAuth::AwsStatic). Ensure the new helpers are used consistently for the tests referencing BedrockProviderConfig, BedrockDef, prepare_request, and related assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/gateway/providers/bedrock.rs`:
- Around line 25-42: Add Rust doc comments (using ///) to the public Bedrock API
surface: add brief docs for the IDENTIFIER constant, the BedrockDef struct, and
the BedrockProviderConfig struct (including short descriptions for its fields
like region, access_key_id, secret_access_key, session_token, and endpoint).
Update the declarations for IDENTIFIER, BedrockDef, and BedrockProviderConfig to
include /// comments that explain their purpose and usage so the module follows
the project's public-item documentation guideline.
- Around line 70-72: The chat path is split: chat_endpoint_path currently
returns "/model" while a custom build_url constructs "/model/{model}/converse",
causing inaccurate provider metadata and bypassing
ProviderMeta::build_url_for_endpoint() overlap handling. Update
chat_endpoint_path(model: &str) to return the full route shape (e.g.
"/model/{model}/converse") using the model parameter, and remove or revert the
custom build_url implementation so the default
build_url()/ProviderMeta::build_url_for_endpoint() is used for URL composition
and overlap handling.
- Around line 230-349: Replace plain assert macros in the bedrock provider tests
with the repo-standard helpers: add imports for pretty_assertions::assert_eq and
assert_matches::assert_matches at the top of the test module, change all
assert_eq! calls (e.g., in
bedrock_provider_config_deserializes_static_credentials,
bedrock_provider_config_debug_redacts_credentials,
build_url_encodes_model_ids_with_slashes,
prepare_request_rewrites_stream_url_before_signing) to use assert_eq, and
replace the matches! usage in prepare_request_rejects_non_aws_auth with
assert_matches (targeting GatewayError::Validation and the message predicate
referencing ProviderAuth::AwsStatic). Ensure the new helpers are used
consistently for the tests referencing BedrockProviderConfig, BedrockDef,
prepare_request, and related assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9278c03f-8c0f-462e-b39e-6be1b04df242
📒 Files selected for processing (1)
src/gateway/providers/bedrock.rs
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/gateway/providers/bedrock.rs (1)
33-52:⚠️ Potential issue | 🟠 MajorDon't derive raw JSON serialization for credential-bearing config.
Debugis redacted, but derivedSerializewill still emitaccess_key_id,secret_access_key, andsession_tokenverbatim. Any admin/API path that serializesBedrockProviderConfigwill leak credentials unless this goes through a separate redacted view or a custom serializer.
🧹 Nitpick comments (2)
tests/proxy/bedrock-chat-completions.test.ts (2)
140-144: Cover the session token on the streaming path too.
buildBedrockProviderConfig()always supplies asession_token, but this test only assertsx-amz-security-tokenon/converse. Adding the same check here would keep auth coverage on/converse-streamas well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/proxy/bedrock-chat-completions.test.ts` around lines 140 - 144, The test currently asserts the session token header only for the /converse path; update the assertions for the streaming request (where recorded[0]?.url equals `${EXPECTED_ENCODED_PATH}/converse-stream`) to also check that recorded[0]?.headers['x-amz-security-token'] or recorded[0]?.headers.authorization includes the session token; this keeps coverage in the test that buildBedrockProviderConfig()'s session_token is present on the converse-stream request as well.
29-31: Replace the fixed propagation sleep with readiness polling.This hard-coded 500ms delay makes the test timing-sensitive in CI. Poll until the model/key is actually visible instead of sleeping for a fixed interval.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/proxy/bedrock-chat-completions.test.ts` around lines 29 - 31, The helper waitConfigPropagation currently uses a fixed 500ms sleep which is flaky; replace it with a readiness poll that repeatedly checks the proxy/config endpoint until the new model/key is visible or a reasonable timeout is reached. Update the waitConfigPropagation function to call the same visibility check the tests rely on (e.g., fetch the proxy's model list or key lookup endpoint) in a loop with short intervals (e.g., 50–200ms) and a total timeout (e.g., 5–10s), returning as soon as the resource is observed and throwing/failed assertion if the timeout elapses; keep the function name waitConfigPropagation so callers remain unchanged and ensure the polling uses the existing HTTP helper used elsewhere in the tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/proxy/bedrock-chat-completions.test.ts`:
- Around line 140-144: The test currently asserts the session token header only
for the /converse path; update the assertions for the streaming request (where
recorded[0]?.url equals `${EXPECTED_ENCODED_PATH}/converse-stream`) to also
check that recorded[0]?.headers['x-amz-security-token'] or
recorded[0]?.headers.authorization includes the session token; this keeps
coverage in the test that buildBedrockProviderConfig()'s session_token is
present on the converse-stream request as well.
- Around line 29-31: The helper waitConfigPropagation currently uses a fixed
500ms sleep which is flaky; replace it with a readiness poll that repeatedly
checks the proxy/config endpoint until the new model/key is visible or a
reasonable timeout is reached. Update the waitConfigPropagation function to call
the same visibility check the tests rely on (e.g., fetch the proxy's model list
or key lookup endpoint) in a loop with short intervals (e.g., 50–200ms) and a
total timeout (e.g., 5–10s), returning as soon as the resource is observed and
throwing/failed assertion if the timeout elapses; keep the function name
waitConfigPropagation so callers remain unchanged and ensure the polling uses
the existing HTTP helper used elsewhere in the tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5c7f26b-070b-4a84-8521-f4f90c6c4880
📒 Files selected for processing (2)
src/gateway/providers/bedrock.rstests/proxy/bedrock-chat-completions.test.ts
This was referenced Apr 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Tests