fix/added universal Bedrock model support using Converse API. fixes #1306#1308
Conversation
Greptile Summary
Confidence Score: 5/5Safe to merge — no logic regressions on the Claude path, and the new Converse path is well-guarded with explicit error handling. All findings are P2 (non-blocking style/coverage concerns). Previously flagged P1s (dead else-branch, str(response) fallback) are resolved. Core routing logic is correct and tested. No files require special attention; the missing _invoke_anthropic dispatch test in tests/services/test_llm_client.py is the only gap worth addressing before the next refactor. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["BedrockLLMClient.invoke()"] --> B{"_is_anthropic_bedrock_model(model_id)"}
B -->|True: anthropic.claude in model_id| C["_invoke_anthropic()"]
B -->|False: all other models| D["_invoke_converse()"]
C --> E["AnthropicBedrock.messages.create()"]
E -->|retry w/ backoff| E
E --> F["LLMResponse(content)"]
D --> G["boto3 bedrock-runtime.converse()"]
G -->|retry w/ backoff| G
G --> H{"text blocks found?"}
H -->|No| I["raise RuntimeError"]
H -->|Yes| F
Reviews (2): Last reviewed commit: "docs(bedrock): align application inferen..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| converse_messages = [ | ||
| { | ||
| "role": msg["role"], | ||
| "content": [{"text": msg["content"]}] | ||
| if isinstance(msg["content"], str) | ||
| else msg["content"], | ||
| } | ||
| for msg in messages | ||
| ] |
There was a problem hiding this comment.
else branch silently passes incompatible content blocks
The else msg["content"] branch is unreachable in practice: _normalize_messages always stringifies content (str(content)), and engine.apply(str) -> str. If this branch were ever reached (e.g., by a future guardrail returning structured content), raw content would be forwarded to the Converse API, which expects [{"text": "..."}] — not Anthropic's [{"type": "text", "text": "..."}] format — causing a silent schema mismatch and API error.
Consider removing the dead branch to make intent explicit:
converse_messages = [
{
"role": msg["role"],
"content": [{"text": msg["content"]}],
}
for msg in messages
]
There was a problem hiding this comment.
_normalize_messages always produces strings, so the else branch is unreachable. Remove it to make intent explicit.
| content = "\n".join(text_parts).strip() | ||
| if not content: | ||
| content = str(response) | ||
| return LLMResponse(content=content) |
There was a problem hiding this comment.
When no "text" blocks are found in the response (e.g., the model returns only tool-use blocks, or an unexpected schema), content falls back to str(response). The boto3 Converse response dict includes stopReason, usage, ResponseMetadata, etc., so downstream callers (e.g., StructuredOutputClient._extract_json_payload) would receive raw AWS metadata as the LLM answer, causing confusing JSON parse failures.
A more explicit fallback would surface the issue clearly:
if not content:
logger.warning("Bedrock converse returned no text blocks; raw response: %s", response)
raise RuntimeError(f"Bedrock converse returned no text content: {response}")There was a problem hiding this comment.
str(response) on a boto3 dict dumps ResponseMetadata, usage, and stopReason, which will break downstream JSON parsing. Raise a clear error instead.
shubh586
changed the title
|
@shubh586 fix review comments |
| content = "\n".join(text_parts).strip() | ||
| if not content: | ||
| content = str(response) | ||
| return LLMResponse(content=content) |
There was a problem hiding this comment.
str(response) on a boto3 dict dumps ResponseMetadata, usage, and stopReason, which will break downstream JSON parsing. Raise a clear error instead.
| content = _extract_text(response) | ||
| return LLMResponse(content=content) | ||
|
|
||
| def _invoke_converse(self, prompt_or_messages: Any) -> LLMResponse: |
There was a problem hiding this comment.
Add unit tests for _invoke_converse and _is_anthropic_bedrock_model in tests/services/test_llm_client.py. The new boto3 path has zero coverage for message conversion, response parsing, and error handling.
|
Thanks for the feedback @muddlebee! I'm working on it! |
| model_lower = model_id.lower() | ||
| if "anthropic.claude" in model_lower: | ||
| return True | ||
| # Application inference profile ARNs — default to Anthropic unless overridden |
There was a problem hiding this comment.
Application inference profile ARNs are forced to Anthropic unless BEDROCK_USE_CONVERSE=1. Non-Claude profiles still fail on AnthropicBedrock. Default these ARNs to converse or resolve the profile target model before routing.
There was a problem hiding this comment.
got it! I’ll update the routing logic.
- Route application-inference-profile ARNs to Converse for non-Claude pools - Raise when Converse returns no text blocks (avoid str(response) metadata) - Build Converse messages with explicit text blocks only - Apply isort for boto3 import - Add unit tests for routing, message shape, and empty-text handling
|
@greptileai review |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Use _InactiveGuardrailEngine as the patched callable instead of wrapping construction in lambdas CodeQL flagged on the PR branch.
- Extend llm-providers Amazon Bedrock section (Claude vs Converse, profiles) - Add BEDROCK_* and bedrock to .env.example alongside other LLM providers
Resolve .env.example: combine LLM_PROVIDER comment lists (bedrock/minimax/ ollama from PR branch + opencode from main).
|
🎉 MERGED! @shubh586 just shipped something. The diff gods are pleased. 🙌
👋 Join us on Discord - OpenSRE : hang out, contribute, or hunt for features and issues. Everyone's welcome. |
|
@shubh586 thank you for efforts, but changes not as per contributor guidlines and standards. next time please take care :) |
|
Thank them for review |
3 participants
Fixes #1306
Describe the changes you have made in this PR -
Summary
This PR overhauls the
BedrockLLMClientto unlock support for all Amazon Bedrock foundation models (such as Mistral, LLaMA, and Amazon Titan) by integrating the model-agnosticboto3BedrockconverseAPI.Before and After
🔴 Before this PR:
The
BedrockLLMClientwas strictly hardcoded to use theAnthropicBedrockSDK.mistral.mistral-large-2402-v1:0), the investigation would get STUCK and fail with a generic'NoneType' object is not iterableerror.🟢 After this PR:
The client now dynamically inspects the requested model ID.
AnthropicBedrockSDK (ensuring zero regressions)._invoke_conversehandler that usesboto3.client("bedrock-runtime"). This translates OpenSRE's internal messages into Bedrock's nativeconverseschema.Steps to reproduce
LLM_PROVIDER=bedrockBEDROCK_REASONING_MODEL=mistral.mistral-large-2402-v1:0opensreinteractive shell.'NoneType'error. After: The Mistral model successfully executes the investigation.