Conversation
Summary of ChangesHello @cgorski, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant new feature by integrating AWS Bedrock as a first-class LLM provider. This allows users to leverage Bedrock's capabilities, including its native Converse API and diverse model offerings, within the application. The implementation focuses on providing a robust and flexible authentication experience, accommodating various AWS credential management strategies, and ensuring a smooth setup process through updated configuration and wizard flows. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces support for AWS Bedrock as an LLM provider, including updates to configuration, documentation, and the setup wizard. The core implementation in src/llm/bedrock.rs is well-structured and includes comprehensive tests. I've provided a few suggestions to improve robustness and maintainability, including consolidating duplicated logic as per repository guidelines. Notably, there's a syntax issue in the setup wizard that might cause build failures on stable Rust. Overall, this is a great addition.
serrrfirat
left a comment
There was a problem hiding this comment.
Summary
Substantial PR adding AWS Bedrock support via native Converse API. The core implementation — message conversion (with strict user/assistant alternation), tool call handling, Document<->JSON conversion, response extraction, and error mapping — is solid and has good unit test coverage (20+ tests). However, the bearer token (API key) auth path is completely non-functional: AWS_BEARER_TOKEN_BEDROCK is collected and stored but never wired to the AWS SDK client, which does not recognize this env var in the Rust SDK (unlike Python/JS SDKs). This is the auth method advertised as 'simplest' in the wizard. IAM and SSO auth paths work correctly. Additionally, the wizard doesn't persist AWS_PROFILE for named profile auth (inconsistent with other settings), and there's an unsafe u32→i32 cast for max_tokens. Recommend blocking on f-1 (bearer token) and f-2 (profile persistence) before merge.
General findings
[NIT] Settings doc comment doesn't mention bedrock as a valid backend (src/settings.rs:51)
The doc comment on llm_backend field says: 'LLM backend: "nearai", "anthropic", "openai", "ollama", "openai_compatible"' — missing 'bedrock' and 'tinfoil'.
Suggested fix:
Update the comment to: 'LLM backend: "nearai", "anthropic", "openai", "ollama", "openai_compatible", "tinfoil", "bedrock"'.
zmanian
left a comment
There was a problem hiding this comment.
Review: AWS Bedrock LLM Provider
The core bedrock.rs implementation is well-structured and handles the Bedrock Converse API correctly. However, there are two blockers preventing merge.
Blocker
-
Bearer token auth is non-functional — The
AWS_BEARER_TOKEN_BEDROCKenvironment variable is read and aTokencredential is constructed, but the AWS Rust SDK's Bedrock client doesn't recognize this credential type for the Converse API. Bearer token auth only works with specific AWS services (like CodeCatalyst), not Bedrock. This auth path will silently fail at runtime. Either remove it and document the supported auth methods (env creds, profile, IMDS), or verify it actually works with a test. -
36 new crate dependencies with no feature gate — This PR adds 36 new transitive dependencies including
aws-lc-syswhich requires cmake and a C compiler to build. This significantly increases build times and system requirements for all users, even those not using Bedrock. This must be behind abedrockfeature flag (like the existinglibsqlfeature pattern) so it's opt-in.
High Priority
-
Unsafe
u32 as i32cast — Token usage values are cast withas i32which silently truncates on overflow. Usei32::try_from(val).unwrap_or(i32::MAX)or similar safe conversion. -
Fragile error detection — Error handling uses
contains("Error:")string matching which is brittle. Match on the actual error variants from the SDK types instead.
Medium Priority
-
Setup wizard integration — The wizard step for Bedrock should only appear when the
bedrockfeature is enabled, consistent with how other optional backends work. -
Missing region validation —
AWS_REGIONdefaults tous-east-1silently. Consider at least logging when the default is used, since Bedrock model availability varies significantly by region.
What's Good
- Clean separation of Bedrock-specific types
- Proper streaming support via Converse Stream API
- Tool use / function calling implementation looks correct
- Good error type mapping
Recommendation
Add a bedrock feature flag to gate the AWS dependencies, remove or fix the bearer token auth path, and this will be close to mergeable. The core implementation quality is solid.
…tle substring matching
- Safe u32→i32 cast for max_tokens using try_from with clamp - Remove brittle string-based error detection fallback for tool results - Validate BEDROCK_CROSS_REGION against allowed values (us/eu/apac/global) - Validate message list is non-empty before Converse API call - Log when using default us-east-1 region - Update llm_backend doc comment to list all backends - Add tests for build_inference_config and empty message handling
The wizard collected the profile name but only printed a hint to set it manually. Now it saves to settings and writes AWS_PROFILE to the bootstrap .env, consistent with how BEDROCK_REGION and other Bedrock settings are persisted.
The AWS SDK dependencies (aws-config, aws-sdk-bedrockruntime, aws-smithy-types) require cmake and a C compiler to build aws-lc-sys. Gate them behind an opt-in `bedrock` feature flag so default builds are unaffected. Build with: cargo build --features bedrock All config, settings, and wizard code stays unconditional (no AWS deps) so users can configure Bedrock even without the feature compiled — they get a clear error at startup directing them to rebuild.
|
All PR comments addressed, ready for merge consideration |
…rchitecture (takeover #345) - Resolve merge conflicts with main's registry-based provider system - Add missing cache_creation_input_tokens/cache_read_input_tokens fields - Add missing content_parts field in test ChatMessage - Fix string literal type mismatches in wizard env_vars (.to_string()) - Remove non-functional bearer token auth (AWS_BEARER_TOKEN_BEDROCK) from wizard and documentation per reviewer feedback from @zmanian and @serrrfirat - Remove stale BEDROCK_ACCESS_KEY proxy entry from provider table - Update Bedrock provider to use is_bedrock string check (LlmBackend enum removed) - Add bedrock_profile fallback from settings in config resolution [skip-regression-check] Co-Authored-By: cgorski <cgorski@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Thanks for the work on this, @cgorski! I've picked up your changes and continued them in #713. The new PR includes your original work plus:
You're credited as co-author on all commits. Feel free to review the new PR! |
|
Thanks for contribution! Taking over in #713 to update to the new format and resolve left over comments. |
* feat: add AWS Bedrock LLM provider via native Converse API * fix: use JSON parsing for tool result error detection instead of brittle substring matching * refactor: extract duplicated inference config builder into helper function * fix: address review feedback — safe casts, input validation, and tests - Safe u32→i32 cast for max_tokens using try_from with clamp - Remove brittle string-based error detection fallback for tool results - Validate BEDROCK_CROSS_REGION against allowed values (us/eu/apac/global) - Validate message list is non-empty before Converse API call - Log when using default us-east-1 region - Update llm_backend doc comment to list all backends - Add tests for build_inference_config and empty message handling * fix: persist AWS_PROFILE for Bedrock named profile auth The wizard collected the profile name but only printed a hint to set it manually. Now it saves to settings and writes AWS_PROFILE to the bootstrap .env, consistent with how BEDROCK_REGION and other Bedrock settings are persisted. * feat: gate AWS Bedrock behind optional `bedrock` feature flag The AWS SDK dependencies (aws-config, aws-sdk-bedrockruntime, aws-smithy-types) require cmake and a C compiler to build aws-lc-sys. Gate them behind an opt-in `bedrock` feature flag so default builds are unaffected. Build with: cargo build --features bedrock All config, settings, and wizard code stays unconditional (no AWS deps) so users can configure Bedrock even without the feature compiled — they get a clear error at startup directing them to rebuild. * fix: address review feedback and adapt Bedrock provider to registry architecture (takeover #345) - Resolve merge conflicts with main's registry-based provider system - Add missing cache_creation_input_tokens/cache_read_input_tokens fields - Add missing content_parts field in test ChatMessage - Fix string literal type mismatches in wizard env_vars (.to_string()) - Remove non-functional bearer token auth (AWS_BEARER_TOKEN_BEDROCK) from wizard and documentation per reviewer feedback from @zmanian and @serrrfirat - Remove stale BEDROCK_ACCESS_KEY proxy entry from provider table - Update Bedrock provider to use is_bedrock string check (LlmBackend enum removed) - Add bedrock_profile fallback from settings in config resolution [skip-regression-check] Co-Authored-By: cgorski <cgorski@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use main's Cargo.lock as base to preserve dependency versions Regenerating Cargo.lock from scratch caused transitive dependency version drift that broke the html_to_markdown fixture test in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: bedrock config bugs — spurious warning, alias normalization, profile fallback - Move is_bedrock check before unknown-backend warning to prevent spurious "unknown backend" log for bedrock users - Normalize backend aliases ("aws", "aws_bedrock") to "bedrock" so the provider factory matches correctly - Add settings.bedrock_profile fallback for AWS_PROFILE, consistent with region and cross_region resolution [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address Copilot review feedback — bearer token cleanup, stop_sequences, model dedup - Remove stale bearer token refs from setup README and CHANGELOG - Remove dead bedrock_api_key secret injection mapping - Pass stop_sequences through to Bedrock InferenceConfiguration - Remove "API key" from wizard menu description (bearer token removed) - Skip duplicate LLM_MODEL write for bedrock backend in wizard - Fix cargo fmt formatting [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address review feedback — async new(), remove LiteLLM entry, wizard fixes - Remove dead LiteLLM-based bedrock entry from providers.json (native Converse API intercepts before registry lookup) - Make BedrockProvider::new() async to avoid block_in_place panic in current_thread runtimes; propagate async to create_llm_provider, build_provider_chain, and init_llm - Document CMake build prerequisite in docs/LLM_PROVIDERS.md - Clear bedrock_profile when user selects "default credentials" in wizard - Fix selected_model clearing to match established pattern (conditional on provider switch, not unconditional) - Add regression tests for bedrock model preservation and profile clearing Addresses review feedback from @zmanian on PR #713. Streaming support tracked in #741. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address remaining review comments — CLAUDE.md backends, wizard UX - Add `bedrock` to CLAUDE.md inline backend list (#10) - Skip full setup re-run when keeping existing Bedrock config (#11) - Clear stale bedrock_profile on empty named-profile input (#12) - Add regression test for empty profile clearing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Chris Gorski <cgorski@cgorski.org> Co-authored-by: cgorski <cgorski@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* feat: add AWS Bedrock LLM provider via native Converse API * fix: use JSON parsing for tool result error detection instead of brittle substring matching * refactor: extract duplicated inference config builder into helper function * fix: address review feedback — safe casts, input validation, and tests - Safe u32→i32 cast for max_tokens using try_from with clamp - Remove brittle string-based error detection fallback for tool results - Validate BEDROCK_CROSS_REGION against allowed values (us/eu/apac/global) - Validate message list is non-empty before Converse API call - Log when using default us-east-1 region - Update llm_backend doc comment to list all backends - Add tests for build_inference_config and empty message handling * fix: persist AWS_PROFILE for Bedrock named profile auth The wizard collected the profile name but only printed a hint to set it manually. Now it saves to settings and writes AWS_PROFILE to the bootstrap .env, consistent with how BEDROCK_REGION and other Bedrock settings are persisted. * feat: gate AWS Bedrock behind optional `bedrock` feature flag The AWS SDK dependencies (aws-config, aws-sdk-bedrockruntime, aws-smithy-types) require cmake and a C compiler to build aws-lc-sys. Gate them behind an opt-in `bedrock` feature flag so default builds are unaffected. Build with: cargo build --features bedrock All config, settings, and wizard code stays unconditional (no AWS deps) so users can configure Bedrock even without the feature compiled — they get a clear error at startup directing them to rebuild. * fix: address review feedback and adapt Bedrock provider to registry architecture (takeover nearai#345) - Resolve merge conflicts with main's registry-based provider system - Add missing cache_creation_input_tokens/cache_read_input_tokens fields - Add missing content_parts field in test ChatMessage - Fix string literal type mismatches in wizard env_vars (.to_string()) - Remove non-functional bearer token auth (AWS_BEARER_TOKEN_BEDROCK) from wizard and documentation per reviewer feedback from @zmanian and @serrrfirat - Remove stale BEDROCK_ACCESS_KEY proxy entry from provider table - Update Bedrock provider to use is_bedrock string check (LlmBackend enum removed) - Add bedrock_profile fallback from settings in config resolution [skip-regression-check] Co-Authored-By: cgorski <cgorski@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use main's Cargo.lock as base to preserve dependency versions Regenerating Cargo.lock from scratch caused transitive dependency version drift that broke the html_to_markdown fixture test in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: bedrock config bugs — spurious warning, alias normalization, profile fallback - Move is_bedrock check before unknown-backend warning to prevent spurious "unknown backend" log for bedrock users - Normalize backend aliases ("aws", "aws_bedrock") to "bedrock" so the provider factory matches correctly - Add settings.bedrock_profile fallback for AWS_PROFILE, consistent with region and cross_region resolution [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address Copilot review feedback — bearer token cleanup, stop_sequences, model dedup - Remove stale bearer token refs from setup README and CHANGELOG - Remove dead bedrock_api_key secret injection mapping - Pass stop_sequences through to Bedrock InferenceConfiguration - Remove "API key" from wizard menu description (bearer token removed) - Skip duplicate LLM_MODEL write for bedrock backend in wizard - Fix cargo fmt formatting [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address review feedback — async new(), remove LiteLLM entry, wizard fixes - Remove dead LiteLLM-based bedrock entry from providers.json (native Converse API intercepts before registry lookup) - Make BedrockProvider::new() async to avoid block_in_place panic in current_thread runtimes; propagate async to create_llm_provider, build_provider_chain, and init_llm - Document CMake build prerequisite in docs/LLM_PROVIDERS.md - Clear bedrock_profile when user selects "default credentials" in wizard - Fix selected_model clearing to match established pattern (conditional on provider switch, not unconditional) - Add regression tests for bedrock model preservation and profile clearing Addresses review feedback from @zmanian on PR nearai#713. Streaming support tracked in nearai#741. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address remaining review comments — CLAUDE.md backends, wizard UX - Add `bedrock` to CLAUDE.md inline backend list (nearai#10) - Skip full setup re-run when keeping existing Bedrock config (nearai#11) - Clear stale bedrock_profile on empty named-profile input (nearai#12) - Add regression test for empty profile clearing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Chris Gorski <cgorski@cgorski.org> Co-authored-by: cgorski <cgorski@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
* feat: add AWS Bedrock LLM provider via native Converse API * fix: use JSON parsing for tool result error detection instead of brittle substring matching * refactor: extract duplicated inference config builder into helper function * fix: address review feedback — safe casts, input validation, and tests - Safe u32→i32 cast for max_tokens using try_from with clamp - Remove brittle string-based error detection fallback for tool results - Validate BEDROCK_CROSS_REGION against allowed values (us/eu/apac/global) - Validate message list is non-empty before Converse API call - Log when using default us-east-1 region - Update llm_backend doc comment to list all backends - Add tests for build_inference_config and empty message handling * fix: persist AWS_PROFILE for Bedrock named profile auth The wizard collected the profile name but only printed a hint to set it manually. Now it saves to settings and writes AWS_PROFILE to the bootstrap .env, consistent with how BEDROCK_REGION and other Bedrock settings are persisted. * feat: gate AWS Bedrock behind optional `bedrock` feature flag The AWS SDK dependencies (aws-config, aws-sdk-bedrockruntime, aws-smithy-types) require cmake and a C compiler to build aws-lc-sys. Gate them behind an opt-in `bedrock` feature flag so default builds are unaffected. Build with: cargo build --features bedrock All config, settings, and wizard code stays unconditional (no AWS deps) so users can configure Bedrock even without the feature compiled — they get a clear error at startup directing them to rebuild. * fix: address review feedback and adapt Bedrock provider to registry architecture (takeover nearai#345) - Resolve merge conflicts with main's registry-based provider system - Add missing cache_creation_input_tokens/cache_read_input_tokens fields - Add missing content_parts field in test ChatMessage - Fix string literal type mismatches in wizard env_vars (.to_string()) - Remove non-functional bearer token auth (AWS_BEARER_TOKEN_BEDROCK) from wizard and documentation per reviewer feedback from @zmanian and @serrrfirat - Remove stale BEDROCK_ACCESS_KEY proxy entry from provider table - Update Bedrock provider to use is_bedrock string check (LlmBackend enum removed) - Add bedrock_profile fallback from settings in config resolution [skip-regression-check] Co-Authored-By: cgorski <cgorski@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use main's Cargo.lock as base to preserve dependency versions Regenerating Cargo.lock from scratch caused transitive dependency version drift that broke the html_to_markdown fixture test in CI. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: bedrock config bugs — spurious warning, alias normalization, profile fallback - Move is_bedrock check before unknown-backend warning to prevent spurious "unknown backend" log for bedrock users - Normalize backend aliases ("aws", "aws_bedrock") to "bedrock" so the provider factory matches correctly - Add settings.bedrock_profile fallback for AWS_PROFILE, consistent with region and cross_region resolution [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address Copilot review feedback — bearer token cleanup, stop_sequences, model dedup - Remove stale bearer token refs from setup README and CHANGELOG - Remove dead bedrock_api_key secret injection mapping - Pass stop_sequences through to Bedrock InferenceConfiguration - Remove "API key" from wizard menu description (bearer token removed) - Skip duplicate LLM_MODEL write for bedrock backend in wizard - Fix cargo fmt formatting [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address review feedback — async new(), remove LiteLLM entry, wizard fixes - Remove dead LiteLLM-based bedrock entry from providers.json (native Converse API intercepts before registry lookup) - Make BedrockProvider::new() async to avoid block_in_place panic in current_thread runtimes; propagate async to create_llm_provider, build_provider_chain, and init_llm - Document CMake build prerequisite in docs/LLM_PROVIDERS.md - Clear bedrock_profile when user selects "default credentials" in wizard - Fix selected_model clearing to match established pattern (conditional on provider switch, not unconditional) - Add regression tests for bedrock model preservation and profile clearing Addresses review feedback from @zmanian on PR nearai#713. Streaming support tracked in nearai#741. [skip-regression-check] Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address remaining review comments — CLAUDE.md backends, wizard UX - Add `bedrock` to CLAUDE.md inline backend list (nearai#10) - Skip full setup re-run when keeping existing Bedrock config (nearai#11) - Clear stale bedrock_profile on empty named-profile input (nearai#12) - Add regression test for empty profile clearing Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Chris Gorski <cgorski@cgorski.org> Co-authored-by: cgorski <cgorski@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
No description provided.