Uh oh!
There was an error while loading. Please reload this page.
feat(bedrock): wire Converse API for all publishers + Anthropic streaming (#302 Phase G Step 3 / D7.2.b/D7.3/D7.4) - #389
Merged
Merged
Conversation
…ming (#302 Phase G Step 3 / D7.2.b / D7.3 / D7.4) Replaces per-publisher /invoke dispatch with the unified Converse API path for everything except Anthropic non-stream (which keeps its existing /invoke path for backward compat with operator deployments + the e2e test fixtures pinned in #320). All other dispatch — Meta / Mistral / Cohere / Amazon Titan / Amazon Nova / AI21 — and ALL chat_stream() requests — including Anthropic — now flow through the SDK's `.converse()` / `.converse_stream()`. This closes D7.2.b (Anthropic streaming), D7.3 (Meta dispatch), and D7.4 (Mistral / Amazon / Cohere / AI21 dispatch) per #302 Phase G's audit-corrected roadmap. ## Why Converse (vs. extending the per-publisher /invoke path) AWS introduced the Converse API exactly to solve the per-publisher body-shaping problem: one JSON envelope (`messages[].content[].text` + `system[]` + `inferenceConfig`), one URL pattern (`/model/<id>/converse[-stream]`), one response shape, works for every Bedrock publisher. The legacy /invoke path requires a per-publisher body translator on the gateway side (Anthropic's Messages, Meta's Llama prompt template, Cohere's command shape, etc.) — multiplying maintenance burden as AWS adds publishers. ## Eventstream framing `/converse-stream` emits `application/vnd.amazon.eventstream` binary frames. The aws-sdk-bedrockruntime SDK owns the frame decoder — we don't need our own (compare with the mock-bedrock fixture I wrote in api7/AISIX-Cloud#480 which hand-rolled the encoder for the test mock). The bridge layer just walks the typed `ConverseStreamOutput` event sequence and maps each event to zero, one, or two `ChatChunk`s: - `MessageStart` → role chunk (first emission only) - `ContentBlockDelta` (text) → content chunk - `MessageStop` → finish_reason chunk - `Metadata` → usage chunk - `ContentBlockStart` / `ContentBlockStop` / tool-use deltas → no chunk ## Error classification The new `map_aws_sdk_error_generic` helper preserves the existing 4xx-vs-5xx classification: ServiceError → `UpstreamStatus` carrying the HTTP code; TimeoutError or deadline-elapsed → `Timeout`; everything else → `Transport`. Mirrors the pattern in the legacy `map_sdk_error` + the audit-corrected Vertex/Azure token-mint classifications (#387, #388). ## Test changes Tests deleted (obsolete after Converse wiring): - chat_rejects_non_anthropic_publishers_with_publisher_named - chat_stream_returns_clear_not_implemented_error - chat_stream_anthropic_returns_d7_2_b_specific_error - chat_stream_non_anthropic_publisher_returns_d7_3_specific_error - chat_publisher_not_implemented_error_includes_model_id_and_publisher_name Tests updated: - chat_ignores_req_model_and_uses_ctx_model_name — re-anchored to the publisher-unknown error path (only publisher-resolution failures still produce Config errors before dispatch) Tests added (5 new for the Converse path): - chat_meta_publisher_dispatches_via_converse_url — `/model/meta.*/converse` path regex + Bedrock-shape response decode - chat_amazon_nova_publisher_dispatches_via_converse_url — same dispatch contract, different publisher prefix (regression guard against a future change that hard-codes per-publisher routing inside chat_converse) - chat_stream_for_anthropic_dispatches_via_converse_stream_url — `/converse-stream` URL pattern pin for Anthropic streaming (legacy /invoke had no stream variant) - chat_stream_for_meta_dispatches_via_converse_stream_url — same pattern for non-Anthropic streaming - chat_converse_request_body_uses_text_content_block_shape — pins outbound body shape: system messages lifted to top-level `system[]`, user messages emit typed `content: [{text: "..."}]` blocks (NOT Anthropic-style flat string), no top-level `model` field `cargo test -p aisix-provider-bedrock` → 41/41 PASS (was 36 with 5 deletions + 1 update + 5 additions). `cargo clippy -p aisix-provider-bedrock --all-targets -- -D warnings` clean. `cargo fmt --all` applied. ## References (CLAUDE.md §7) - Converse API spec: https://docs.aws.amazon.com/bedrock/latest/APIReference/API_runtime_Converse.html - Converse stream events: https://docs.aws.amazon.com/bedrock/latest/APIReference/API_runtime_ConverseStream.html - aws-sdk-bedrockruntime 1.130.0 operation modules: https://docs.rs/aws-sdk-bedrockruntime/1.130.0/aws_sdk_bedrockruntime/operation/ - mock-bedrock fixture (api7/AISIX-Cloud#480) uses the same `vnd.amazon.eventstream` frame format for its encoder; the bridge here consumes the SDK-decoded events rather than re-implementing the decoder ## Out of scope (deferred follow-ups) - D7.2.a Anthropic /invoke → Converse migration. Keeping legacy path until cp-api / dashboard test fixtures are updated to assert the Converse outbound body shape instead of the Anthropic Messages shape. The customer-visible ChatResponse is identical on both paths. - Tool-use / image / document `ContentBlock` variants — chat scenarios only carry `Text` blocks today; the `Role::Tool` arm in `build_converse_inputs` is a deliberate no-op pending a structured tool-use surface on `ChatFormat`. ## Unblocks AC.10 in api7/AISIX-Cloud#302: "Bedrock 上 Claude / Llama 都能 stream". Both halves are now done — Claude streaming via Converse, Llama via Converse (chat + stream). Live e2e against mock-bedrock (#480) is the next sub-step, separate PR.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR completes Bedrock Phase G by unifying chat and streaming dispatch through the Bedrock Converse API for all non-Anthropic publishers, while preserving Anthropic non-streaming on the legacy invoke path. New translation, streaming, and error-mapping helpers support the Converse integration, with test coverage validating the correct routing and request body shapes. ChangesBedrock Phase G Converse Unification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. Comment |
There was a problem hiding this comment.
Pull request overview
This PR switches aisix-provider-bedrock from per-publisher /invoke dispatch to the unified Bedrock Converse API for all non-Anthropic chat() calls and for allchat_stream() calls (including Anthropic), while keeping Anthropic non-streaming on the legacy /invoke path for backwards compatibility.
Changes:
- Route non-Anthropic
chat()requests viaPOST /model/<id>/converseand implement response translation toChatResponse. - Implement
chat_stream()for all publishers viaPOST /model/<id>/converse-stream, mapping typed SDK events intoChatChunks. - Update unit tests and add wiremock-based dispatch/body-shape guards; add
async-streamdependency.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| crates/aisix-provider-bedrock/src/lib.rs | Updates crate-level docs to reflect Converse-based dispatch and streaming support. |
| crates/aisix-provider-bedrock/src/bridge.rs | Implements Converse + ConverseStream dispatch paths, request/response translation, and associated tests. |
| crates/aisix-provider-bedrock/Cargo.toml | Adds async-stream and updates comments to match the new dispatch split. |
| Cargo.lock | Records the new async-stream dependency in the lockfile. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+785
to
+792
| // Tool-result messages are part of Anthropic's tool-use | ||
| // protocol; Bedrock Converse supports them via | ||
| // `ContentBlock::ToolResult` but the gateway's ChatMessage | ||
| // surface doesn't carry the structured tool_use_id + | ||
| // content shape needed to round-trip cleanly. Skip | ||
| // silently for now; a tool-use-aware follow-up PR can | ||
| // wire this when the upstream ChatFormat extends to | ||
| // carry the structured payload. |
Comment on lines
+936
to
+945
| /// Map the SDK's `ConverseError` SdkError variant to BridgeError. | ||
| /// Same classification rules as `map_sdk_error` (UpstreamStatus for | ||
| /// retryable upstream failures, Timeout on dispatch timeout). | ||
| fn map_converse_sdk_error( | ||
| e: SdkError<ConverseError, aws_smithy_runtime_api::http::Response>, | ||
| started: Instant, | ||
| deadline: Option<Duration>, | ||
| ) -> BridgeError { | ||
| map_aws_sdk_error_generic(e.to_string(), &e, started, deadline) | ||
| } |
Comment on lines
+979
to
+988
| match err { | ||
| SdkError::ServiceError(svc) => { | ||
| let status = svc.raw().status().as_u16(); | ||
| BridgeError::upstream_status(status, msg) | ||
| } | ||
| SdkError::TimeoutError(_) => BridgeError::Timeout { | ||
| elapsed_ms: started.elapsed().as_millis() as u64, | ||
| }, | ||
| _ => BridgeError::Transport(msg), | ||
| } |
| // unset by default so the upstream model's own defaults apply. | ||
| // A follow-up override-pipeline PR can wire temperature / | ||
| // max_tokens from RequestOverrides. | ||
| let _ = InferenceConfiguration::builder(); |
Comment on lines
+802
to
+820
| fn converse_output_into_chat_response( | ||
| resp: aws_sdk_bedrockruntime::operation::converse::ConverseOutput, | ||
| upstream_id: &str, | ||
| ) -> ChatResponse { | ||
| let (text, finish) = match resp.output() { | ||
| Some(aws_sdk_bedrockruntime::types::ConverseOutput::Message(msg)) => { | ||
| let text: String = msg | ||
| .content() | ||
| .iter() | ||
| .filter_map(|cb| match cb { | ||
| ContentBlock::Text(t) => Some(t.as_str()), | ||
| _ => None, | ||
| }) | ||
| .collect::<Vec<_>>() | ||
| .join(""); | ||
| (text, map_stop_reason(resp.stop_reason())) | ||
| } | ||
| _ => (String::new(), FinishReason::Stop), | ||
| }; |
…audit HIGH/MEDIUM on #389) Independent audit-aigw-389-bedrock-converse flagged 2 HIGH + 2 MEDIUM bugs in the original Converse wiring. All fixed in this commit; 45/45 tests pass. ## HIGH-1 + HIGH-2 — `map_aws_sdk_error_generic` silently regressed the legacy /invoke path's error envelope hardening The original helper collapsed all ServiceError responses to `BridgeError::upstream_status(status, msg)`, a convenience constructor that sets `wire: Unknown` + `parsed: None` + `retry_after: None`. The legacy `chat_anthropic` /invoke path uses `map_service_error` which builds a fully-shaped `BridgeError::UpstreamStatus` with: - wire: Bedrock — required by error_translate to render Bedrock- shape errors back to OpenAI/Anthropic-shape clients - retry_after: parsed from upstream Retry-After header — required by the cooldown layer to honour AWS throttle hints - parsed.kind: extracted via .meta().code() — distinguishes a ThrottlingException 429 from a different throttle source Without this fix every non-Anthropic publisher + all streaming silently shipped degraded errors vs. the legacy path — broke the PR #323 audit (MEDIUM-2) hardening it had just restored. Fix: introduce `bedrock_service_error_to_upstream_status` (generic over `E: ProvideErrorMetadata`) that mirrors `map_service_error`'s field-by-field construction. Both Converse + ConverseStream errors now flow through it. ## MEDIUM-1 — `e.to_string()` produced opaque "service error" strings as the customer-visible message Same root cause; resolved by the HIGH fix above. The customer- visible `message` now uses the legacy path's canned status-keyed phrase ("upstream rate limited", "upstream authentication failed", etc.) — preserving the operator-ARN redaction the legacy path also enforces. ## MEDIUM-2 — `chat_converse` silently dropped ChatFormat's temperature/max_tokens/top_p The original `chat_converse` constructed an `InferenceConfiguration::builder()` and immediately discarded it with `let _ =`. Every non-Anthropic Bedrock customer using these knobs would see them vanish — real behavioural regression vs. the legacy chat_anthropic path which forwards them via build_request → AnthropicRequest. Fix: new `build_inference_config(req)` helper extracts temperature / max_tokens / top_p when set, returns None when all absent (so we don't emit an empty `inferenceConfig: {}` which 400s on some Bedrock publishers). Wired into both chat_converse and chat_converse_stream. ## Tests added (5 new for the audit fixes) - chat_converse_maps_upstream_429_with_retry_after_and_bedrock_wire pins the HIGH-1+2 fix: 429 + Retry-After: 42 → UpstreamStatus with status=429, message="upstream rate limited", wire=Bedrock, retry_after=Some(42s), parsed.kind="ThrottlingException" - chat_converse_maps_upstream_4xx_to_canned_message_with_bedrock_wire pins HIGH-1 for the non-throttle path: 403 with leaky ARN body → canned "upstream authentication failed" (NO ARN leak), wire=Bedrock, parsed.kind="AccessDeniedException" - chat_converse_wires_temperature_max_tokens_top_p_into_inference_config pins MEDIUM-2: ChatFormat knobs reach Bedrock's body as camelCase {temperature, maxTokens, topP} - chat_converse_omits_inference_config_when_no_knobs_set companion: empty knobs → no inferenceConfig field on the wire `cargo test -p aisix-provider-bedrock` → 45/45 PASS (was 41; +4 new audit-regression tests). `cargo clippy -p aisix-provider-bedrock --all-targets -- -D warnings` clean. ## LOWs (non-blocking, deferred per CLAUDE.md §8) - LOW-1: Role::Tool messages silently dropped on Converse path — deliberate no-op pending a structured tool-use surface on ChatFormat. Documented in `build_converse_inputs`. - LOW-2: Per-publisher dispatch test matrix coverage (Mistral / Cohere / AmazonTitan / AI21 not individually pinned). The match arm in chat() is structurally uniform (`_ => chat_converse`) so routing is correct by construction; the two existing Meta + Amazon Nova tests already cover the non-Anthropic dispatch contract.
moonming
commented
May 25, 2026
MemberAuthor
Audit response — all HIGH/MEDIUM addressed in commit `b97829c`Independent audit-aigw-389-bedrock-converse returned 2 HIGH + 2 MEDIUM + 2 LOW. All HIGH/MEDIUM fixed in this commit; LOWs explicitly deferred per CLAUDE.md §8. HIGH-1 + HIGH-2 ✅ fixedThe original `map_aws_sdk_error_generic` collapsed all ServiceError responses to `BridgeError::upstream_status(status, msg)` — a convenience constructor that sets `wire: Unknown`, `parsed: None`, `retry_after: None`. The legacy `/invoke` path's `map_service_error` (hardened by PR #323's MEDIUM-2 audit) builds full UpstreamStatus with `wire: Bedrock`, parsed AWS error code, and parsed Retry-After. The Converse path silently regressed this hardening for every non-Anthropic publisher + all streaming. Fix: new `bedrock_service_error_to_upstream_status` helper (generic over `E: ProvideErrorMetadata`) mirrors `map_service_error` field-by-field. Both Converse + ConverseStream errors flow through it. MEDIUM-1 ✅ fixedSame root cause as HIGH-1; resolved by the same change. Customer-visible messages now use the canned status-keyed phrases ("upstream rate limited", "upstream authentication failed") — preserving the operator-ARN redaction the legacy path enforces. MEDIUM-2 ✅ fixed`chat_converse` silently dropped `ChatFormat.temperature` / `.max_tokens` / `.top_p` — real behavioural regression for non-Anthropic Bedrock customers. New `build_inference_config` helper extracts the knobs when set (returns None when all absent to avoid emitting empty `inferenceConfig: {}` which 400s on some publishers). Wired into both chat_converse and chat_converse_stream. Test coverage added (4 new tests pinning the audit fixes)
LOWs (deferred)
Result`cargo test -p aisix-provider-bedrock` → 45/45 PASS (was 41; +4 new audit-regression tests). All HIGH/MEDIUM findings addressed. Awaiting fresh CI green. |
Uh oh!
There was an error while loading. Please reload this page.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Replaces per-publisher `/invoke` dispatch with the unified Converse API path for everything except Anthropic non-stream (which keeps its existing `/invoke` path for backward compat with operator deployments + e2e test fixtures pinned in #320). All other dispatch — Meta / Mistral / Cohere / Amazon Titan / Amazon Nova / AI21 — and all `chat_stream()` requests (including Anthropic) — now flow through the SDK's `.converse()` / `.converse_stream()`.
Closes D7.2.b (Anthropic streaming), D7.3 (Meta dispatch), and D7.4 (Mistral / Amazon Titan/Nova / Cohere / AI21 dispatch) per #302 Phase G's audit-corrected roadmap.
Why Converse vs. extending per-publisher `/invoke`
AWS introduced the Converse API exactly to solve the per-publisher body-shaping problem: one JSON envelope (`messages[].content[].text` + `system[]` + `inferenceConfig`), one URL pattern, one response shape — works for every Bedrock publisher. The legacy `/invoke` path requires a per-publisher body translator on the gateway side (Anthropic Messages, Meta Llama prompt template, Cohere command shape, etc.) — that multiplies maintenance burden as AWS adds publishers.
Eventstream framing
`/converse-stream` emits `application/vnd.amazon.eventstream` binary frames. The aws-sdk-bedrockruntime SDK owns the frame decoder — we don't need our own (compare with the mock-bedrock fixture in #480 which hand-rolled the encoder for the test mock). The bridge layer just walks the typed `ConverseStreamOutput` event sequence and maps each event to zero, one, or two `ChatChunk`s:
Dispatch shape
```rust
// chat() — Anthropic legacy /invoke for backward compat; everyone else Converse
match publisher {
BedrockPublisher::Anthropic => self.chat_anthropic(req, ctx, upstream_id).await,
_ => self.chat_converse(req, ctx, upstream_id).await,
}
// chat_stream() — all publishers through Converse (legacy /invoke had no stream variant)
self.chat_converse_stream(req, ctx, upstream_id).await
```
Test changes
Deleted (5 tests, obsolete after Converse wiring):
Updated (1 test):
Added (5 tests):
Test plan
References (CLAUDE.md §7)
Out of scope (deferred follow-ups)
Unblocks
AC.10 in api7/AISIX-Cloud#302: "Bedrock 上 Claude / Llama 都能 stream". Both halves now done — Claude streaming via Converse, Llama via Converse (chat + stream). Live e2e against mock-bedrock (#480) is the next sub-step, separate PR.
Summary by CodeRabbit
New Features
Improvements
Documentation
Tests