feat(guardrail): add aws bedrock#101
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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughImplements a Bedrock guardrail runtime: adds dependencies, exposes guardrails module, defines Bedrock config/runtime types and wire formats, SigV4-signs requests, sends them via reqwest to Bedrock apply, maps responses to GuardrailOutcome, and adds unit tests. ChangesBedrock Guardrail Runtime
Sequence DiagramsequenceDiagram
participant Runtime as BedrockGuardrailRuntime
participant Signer as aws-sigv4 Signer
participant Client as reqwest::Client
participant Bedrock as Bedrock Apply API
Runtime->>Signer: build headers and SigV4 sign (POST /model/.../apply)
Signer-->>Runtime: signed headers + signed url
Runtime->>Client: POST signed URL with JSON body (content blocks)
Client->>Bedrock: HTTP POST (signed, JSON)
Bedrock-->>Client: HTTP response (JSON action + outputs)
Client-->>Runtime: response body + status
Runtime->>Runtime: map action -> GuardrailOutcome (Allow/Rewrite/Block) or BedrockError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ 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
🧹 Nitpick comments (1)
crates/aisix-guardrail/src/guardrails/bedrock.rs (1)
22-270: 🏗️ Heavy liftDocument newly exposed public API items with
///comments.This file introduces multiple public constants/types/constructors without doc comments, which makes the crate API harder to consume and maintain.
As per coding guidelines: "Use /// for doc comments on public items in Rust".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-guardrail/src/guardrails/bedrock.rs` around lines 22 - 270, Several newly exported items lack doc comments; add /// documentation for each public constant, struct, method, and enum introduced here (so the crate API is documented). Specifically, add concise /// comments for IDENTIFIER, DEFAULT_OUTPUT_SCOPE, DEFAULT_RUNTIME_HOST_PREFIX, DEFAULT_RUNTIME_HOST_SUFFIX, the BedrockGuardrailConfig and BedrockRuntimeConfig structs (documenting fields and purpose), BedrockGuardrailMeta, BedrockGuardrailRuntime and its public constructors new and with_client, and the BedrockError enum (documenting error variants semantics). Keep comments short and descriptive, mention expected usage/semantics for constructors (e.g., what BedrockRuntimeConfig fields mean and optional endpoint/session_token), and document what each BedrockError variant represents.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aisix-guardrail/src/guardrails/bedrock.rs`:
- Around line 379-415: The match on response.action should handle the
"GUARDRAIL_BLOCKED" action explicitly instead of falling through to the
UnsupportedPayload error; update the match in the function that maps bedrock
responses (the match on response.action.as_str()) to add a "GUARDRAIL_BLOCKED"
arm which returns Ok(GuardrailOutcome::Block { reason:
response.action_reason.unwrap_or_else(|| "bedrock guardrail blocked".into()) })
so valid blocked responses produce GuardrailOutcome::Block, leaving the existing
"GUARDRAIL_INTERVENED" and fallback arms unchanged.
- Around line 236-242: The Bedrock guardrail HTTP call currently uses
self.client.post(signed.url)...body(body).send().await which relies on the
client's default timeout; modify the RequestBuilder to set an explicit
per-request timeout by calling the RequestBuilder.timeout(...) with a
std::time::Duration (e.g., Duration::from_secs(5) or other acceptable bound)
before .send(), and add the necessary Duration import; update the call sequence
around self.client.post(...) to include this timeout so the await is bounded.
---
Nitpick comments:
In `@crates/aisix-guardrail/src/guardrails/bedrock.rs`:
- Around line 22-270: Several newly exported items lack doc comments; add ///
documentation for each public constant, struct, method, and enum introduced here
(so the crate API is documented). Specifically, add concise /// comments for
IDENTIFIER, DEFAULT_OUTPUT_SCOPE, DEFAULT_RUNTIME_HOST_PREFIX,
DEFAULT_RUNTIME_HOST_SUFFIX, the BedrockGuardrailConfig and BedrockRuntimeConfig
structs (documenting fields and purpose), BedrockGuardrailMeta,
BedrockGuardrailRuntime and its public constructors new and with_client, and the
BedrockError enum (documenting error variants semantics). Keep comments short
and descriptive, mention expected usage/semantics for constructors (e.g., what
BedrockRuntimeConfig fields mean and optional endpoint/session_token), and
document what each BedrockError variant represents.
🪄 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: d524fcef-501d-4457-b3b2-68dd7f311617
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/aisix-guardrail/Cargo.tomlcrates/aisix-guardrail/src/guardrails/bedrock.rscrates/aisix-guardrail/src/guardrails/mod.rscrates/aisix-guardrail/src/lib.rs
| let response = self | ||
| .client | ||
| .post(signed.url) | ||
| .headers(signed.headers) | ||
| .body(body) | ||
| .send() | ||
| .await?; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In reqwest (current stable), does Client::new() include a default request timeout, and is per-request .timeout(...) the recommended way to enforce bounded wait time?
💡 Result:
Yes—reqwest’s stable behavior includes a default overall request timeout when using Client::new (through ClientBuilder defaults), and it is 30 seconds. Reasoning and details: 1) ClientBuilder::timeout documents that it “Set[s] a timeout for connect, read and write operations of a Client” and that “Default is 30 seconds,” with “Pass None to disable timeout.” [1] 2) The reqwest issue tracker also reports that with Client::new (and ClientBuilder::new.build) “requests will time out after 30 seconds,” despite some docs claiming “Default is no timeout.” [2] Is per-request.timeout(...) the recommended way to enforce a bounded wait time? - The per-request reqwest::RequestBuilder::timeout(Duration) is explicitly described as “applied … from when the request starts connecting until the response body has finished,” and it “affects only this request and overrides the timeout configured using ClientBuilder::timeout.” [3] - Therefore, if you want different bounded wait times per request, using per-request.timeout(...) is appropriate (it overrides the client default) and is the direct mechanism to do so. [3] - If you want a single consistent bound for most requests, setting the timeout on the Client via ClientBuilder::timeout(...) (or.timeout(...) on a blocking/async ClientBuilder) is the mechanism; per-request.timeout(...) is then for exceptions because it overrides the client’s timeout. [1][3] Example pattern (per-request override): client.get(url).timeout(Duration::from_secs(10)).send.await
Citations:
- 1: https://docs.rs/reqwest/0.10.10/reqwest/blocking/struct.ClientBuilder.html
- 2: https://docs.rs/reqwest/0.10.0/reqwest/struct.Client.html
- 3: Default timeouts seanmonstar/reqwest#181
🏁 Script executed:
# Check the broader context around the Bedrock HTTP request to understand if there's specific timeout requirements
cat -n crates/aisix-guardrail/src/guardrails/bedrock.rs | head -260 | tail -40Repository: api7/aisix
Length of output: 1717
🏁 Script executed:
# Search for any existing timeout configurations elsewhere in the codebase for Bedrock or similar HTTP requests
rg "timeout" crates/aisix-guardrail/src/guardrails/ -A 2 -B 2Repository: api7/aisix
Length of output: 36
🏁 Script executed:
# Check if there are any tests or documentation about Bedrock request expectations
rg -i "bedrock" crates/aisix-guardrail/ --type rs -lRepository: api7/aisix
Length of output: 81
🏁 Script executed:
# Search for BedrockGuardrailRuntime struct definition and client field initialization
rg "struct BedrockGuardrailRuntime" crates/aisix-guardrail/src/guardrails/bedrock.rs -A 20Repository: api7/aisix
Length of output: 612
🏁 Script executed:
# Search for impl blocks that initialize the client (new, from_config, etc.)
rg "impl.*BedrockGuardrailRuntime" crates/aisix-guardrail/src/guardrails/bedrock.rs -A 30Repository: api7/aisix
Length of output: 2339
🏁 Script executed:
# Look for any timeout-related constants or configuration in the file
rg -i "timeout|duration" crates/aisix-guardrail/src/guardrails/bedrock.rsRepository: api7/aisix
Length of output: 162
Add an explicit per-request timeout for the Bedrock HTTP call.
The .send().await call relies on reqwest's default 30-second timeout (from Client::new()). Since this guardrail check is latency-sensitive, add an explicit per-request timeout override to ensure bounded wait time:
Suggested fix
let response = self
.client
.post(signed.url)
.headers(signed.headers)
+ .timeout(std::time::Duration::from_secs(10))
.body(body)
.send()
.await?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let response = self | |
| .client | |
| .post(signed.url) | |
| .headers(signed.headers) | |
| .body(body) | |
| .send() | |
| .await?; | |
| let response = self | |
| .client | |
| .post(signed.url) | |
| .headers(signed.headers) | |
| .timeout(std::time::Duration::from_secs(10)) | |
| .body(body) | |
| .send() | |
| .await?; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/aisix-guardrail/src/guardrails/bedrock.rs` around lines 236 - 242, The
Bedrock guardrail HTTP call currently uses
self.client.post(signed.url)...body(body).send().await which relies on the
client's default timeout; modify the RequestBuilder to set an explicit
per-request timeout by calling the RequestBuilder.timeout(...) with a
std::time::Duration (e.g., Duration::from_secs(5) or other acceptable bound)
before .send(), and add the necessary Duration import; update the call sequence
around self.client.post(...) to include this timeout so the await is bounded.
There was a problem hiding this comment.
Create an issue for this.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
gh issue create \
--repo api7/aisix \
--title "feat(guardrail): add explicit per-request timeout for Bedrock HTTP call" \
--body "## Summary
The Bedrock guardrail HTTP call in \`crates/aisix-guardrail/src/guardrails/bedrock.rs\` currently relies on reqwest's default timeout from \`Client::new()\` without an explicit per-request timeout override. Since this guardrail check is latency-sensitive, an explicit timeout should be configured to ensure bounded wait time.
## Suggested Fix
Add a per-request \`.timeout(...)\` call before \`.send().await\`:
\`\`\`rust
let response = self
.client
.post(signed.url)
.headers(signed.headers)
.timeout(std::time::Duration::from_secs(10))
.body(body)
.send()
.await?;
\`\`\`
The specific timeout value (e.g., 5s, 10s) should be decided based on acceptable latency bounds for guardrail checks.
## References
- PR: https://github.com/api7/aisix/pull/101
- Review comment: https://github.com/api7/aisix/pull/101#discussion_r3224587569
Reported by `@bzp2010`" \
--assignee bzp2010
Length of output: 41
@bzp2010, I've created the follow-up issue: #102 — assigned to you for tracking the explicit per-request timeout on the Bedrock HTTP call.
🧠 Learnings used
Learnt from: bzp2010
Repo: api7/aisix PR: 34
File: src/proxy/hooks2/authorization/mod.rs:1-1
Timestamp: 2026-04-10T09:35:05.745Z
Learning: In Rust code using the `anyhow` crate, treat `use anyhow::Result;` as potentially used even if the code uses non-default error types like `Result<T, SomeOtherError>` (i.e., `Result<T, E>` where `E` is not `anyhow::Error`). Because `anyhow::Result` is an alias for `std::result::Result<T, E=anyhow::Error>`, the import is referenced by the `Result<..., ...>` syntax. Do not flag `use anyhow::Result;` as unused solely due to the presence of `Result<T, SomeOtherError>` usages.
1 participant
Summary by CodeRabbit
New Features
Tests
Chores