feat(bedrock): 新增 AWS Profile 认证方式#1748
Merged
Merged
Conversation
Add a new authentication mode for AWS Bedrock provider that allows users to authenticate using named AWS profiles (~/.aws/credentials) instead of static access keys. Uses fromNodeProviderChain for full AWS credential chain resolution (SSO, process credentials, etc).
…n model listing - Change checkStrategy from generate-text to fetch-models so verification no longer depends on a specific model ID - Support profile auth mode in fetchBedrockModels() via fromNodeProviderChain
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds profile-based AWS Bedrock authentication: extends credential schema/types with ChangesAWS Bedrock Profile-Based Authentication
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/renderer/src/i18n/de-DE/settings.json (1)
1146-1146:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGerman verification hint no longer matches actual behavior.
Line 1146 still claims verification uses Claude 3.5 Sonnet. With
fetch-modelsverification, this text is stale and can confuse troubleshooting.Suggested fix
- "bedrockVerifyTip": "DeepChat verwendet Claude 3.5 Sonnet zur Prüfung. Wenn Sie keine Aufrufberechtigung für dieses Modell haben, schlägt die Prüfung fehl. Dies wirkt sich nicht auf die Nutzung anderer Modelle aus.", + "bedrockVerifyTip": "DeepChat prüft die Konfiguration, indem die verfügbare Modellliste von AWS Bedrock abgerufen wird. Wenn die Prüfung fehlschlägt, prüfen Sie Zugangsdaten, Region und Berechtigungen.",🤖 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 `@src/renderer/src/i18n/de-DE/settings.json` at line 1146, The "bedrockVerifyTip" localization string is outdated (mentions "Claude 3.5 Sonnet") and should be updated to reflect the current "fetch-models" verification behavior; locate the "bedrockVerifyTip" entry in the German i18n file and replace the message with a concise statement explaining that verification uses the fetch-models method and that lacking model access will cause verification to fail without affecting other models, keeping tone and phrasing consistent with surrounding entries.src/renderer/src/i18n/da-DK/settings.json (1)
1004-1004:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
bedrockVerifyTipis outdated after strategy change.Line 1004 says verification uses Claude 3.5 Sonnet, but this PR switched verification to model fetching. The message will mislead users about failure causes.
Suggested fix
- "bedrockVerifyTip": "DeepChat bruger Claude 3.5 Sonnet til verifikation. Hvis du ikke har adgang, mislykkes testen – dette påvirker ikke andre modeller.", + "bedrockVerifyTip": "DeepChat verificerer konfigurationen ved at hente den tilgængelige modelliste fra AWS Bedrock. Hvis forespørgslen mislykkes, skal du kontrollere legitimationsoplysninger, region og tilladelser.",🤖 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 `@src/renderer/src/i18n/da-DK/settings.json` at line 1004, The translation key bedrockVerifyTip is outdated (mentions "Claude 3.5 Sonnet") and should be updated to reflect that verification now uses model fetching; locate the bedrockVerifyTip entry in src/renderer/src/i18n/da-DK/settings.json and replace the message text so it accurately states that verification attempts to fetch or probe the configured Bedrock model and that failures indicate model access/fetch issues (not specifically Claude 3.5 Sonnet), keeping the Danish phrasing consistent with surrounding entries.src/main/presenter/llmProviderPresenter/providers/aiSdkProvider.ts (1)
1745-1817:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBedrock verification now succeeds on fallback data.
fetchBedrockModels()swallows missing/invalid Bedrock auth by returning provider-db Claude models, and thefetch-modelsbranch incheck()treats any returned list as a successful verification. After this PR’s strategy change, "Verify" can report success for a Bedrock config that still cannot authenticate.Suggested direction
- private async fetchBedrockModels(): Promise<MODEL_META[]> { + private async fetchBedrockModels(options?: { allowFallback?: boolean }): Promise<MODEL_META[]> { + const allowFallback = options?.allowFallback !== false const provider = this.provider as AWS_BEDROCK_PROVIDER const credential = provider.credential const region = credential?.region || process.env.BEDROCK_REGION const useProfile = credential?.authMode === 'profile' && credential?.profile if (!useProfile) { const accessKeyId = credential?.accessKeyId || process.env.BEDROCK_ACCESS_KEY_ID const secretAccessKey = credential?.secretAccessKey || process.env.BEDROCK_SECRET_ACCESS_KEY if (!accessKeyId || !secretAccessKey || !region) { - return this.mapConfigDbModels(this.definition.providerDbSourceId).filter((model) => - model.id.startsWith('anthropic.') - ) + if (allowFallback) { + return this.mapConfigDbModels(this.definition.providerDbSourceId).filter((model) => + model.id.startsWith('anthropic.') + ) + } + throw new Error('Missing AWS Bedrock credentials') } } if (!region) { - return this.mapConfigDbModels(this.definition.providerDbSourceId).filter((model) => - model.id.startsWith('anthropic.') - ) + if (allowFallback) { + return this.mapConfigDbModels(this.definition.providerDbSourceId).filter((model) => + model.id.startsWith('anthropic.') + ) + } + throw new Error('Missing AWS region') } try { // ... } catch (error) { console.error('获取AWS Bedrock Anthropic模型列表出错:', error) - return this.mapConfigDbModels(this.definition.providerDbSourceId).filter((model) => - model.id.startsWith('anthropic.') - ) + if (allowFallback) { + return this.mapConfigDbModels(this.definition.providerDbSourceId).filter((model) => + model.id.startsWith('anthropic.') + ) + } + throw error } }case 'fetch-models': default: try { - await this.fetchProviderModels() + if (this.definition.modelSource === 'bedrock') { + await this.fetchBedrockModels({ allowFallback: false }) + } else { + await this.fetchProviderModels() + } return { isOk: true, errorMsg: null } } catch (error) {Also applies to: 2285-2294
🤖 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 `@src/main/presenter/llmProviderPresenter/providers/aiSdkProvider.ts` around lines 1745 - 1817, fetchBedrockModels currently masks missing/invalid Bedrock credentials by returning provider-db Claude models, letting check() treat that as a successful verification; change fetchBedrockModels (and the duplicate block later) to throw a descriptive error when required Bedrock auth/region is missing or invalid instead of returning the fallback mapConfigDbModels(...) list so authentication failures surface. Locate the checks around credential/accessKeyId/secretAccessKey/region in fetchBedrockModels and replace the early-return fallback with: throw new Error('Missing Bedrock credentials or region') (or similar), and ensure any calling check() code is ready to catch this error and mark verification as failed.
🤖 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 `@src/main/presenter/llmProviderPresenter/providers/aiSdkProvider.ts`:
- Around line 1747-1754: The code that builds Bedrock credentials (variables
region, accessKeyId, secretAccessKey, and the useProfile branch in
aiSdkProvider.ts) only checks BEDROCK_* env vars, causing mismatch with runtime
which uses AWS_* vars; update the credential resolution to fall back to the
AWS_* equivalents (e.g., process.env.AWS_REGION, process.env.AWS_ACCESS_KEY_ID,
process.env.AWS_SECRET_ACCESS_KEY) when the BEDROCK_* vars are absent, and make
the same change in the other Bedrock credential blocks referenced (the blocks
around the useProfile/accessKeyId/secretAccessKey logic and the later repeated
sections) so validation/model refresh uses the same env var names as
providerFactory.
In `@src/renderer/settings/components/BedrockProviderSettingsDetail.vue`:
- Around line 301-304: The handler handleAuthModeChange currently force-casts
any string into authMode and persists it via
providerStore.updateAwsBedrockProviderConfig; change it to explicitly validate
that the incoming value is one of the allowed literals ('accessKeys' or
'profile') before assigning authMode.value and calling
providerStore.updateAwsBedrockProviderConfig(props.provider.id, ...); if the
value is not one of those two, bail out (or set a safe default) and do not
persist, optionally logging or emitting an error—this ensures only valid
authMode values are stored and used by auth branching.
---
Outside diff comments:
In `@src/main/presenter/llmProviderPresenter/providers/aiSdkProvider.ts`:
- Around line 1745-1817: fetchBedrockModels currently masks missing/invalid
Bedrock credentials by returning provider-db Claude models, letting check()
treat that as a successful verification; change fetchBedrockModels (and the
duplicate block later) to throw a descriptive error when required Bedrock
auth/region is missing or invalid instead of returning the fallback
mapConfigDbModels(...) list so authentication failures surface. Locate the
checks around credential/accessKeyId/secretAccessKey/region in
fetchBedrockModels and replace the early-return fallback with: throw new
Error('Missing Bedrock credentials or region') (or similar), and ensure any
calling check() code is ready to catch this error and mark verification as
failed.
In `@src/renderer/src/i18n/da-DK/settings.json`:
- Line 1004: The translation key bedrockVerifyTip is outdated (mentions "Claude
3.5 Sonnet") and should be updated to reflect that verification now uses model
fetching; locate the bedrockVerifyTip entry in
src/renderer/src/i18n/da-DK/settings.json and replace the message text so it
accurately states that verification attempts to fetch or probe the configured
Bedrock model and that failures indicate model access/fetch issues (not
specifically Claude 3.5 Sonnet), keeping the Danish phrasing consistent with
surrounding entries.
In `@src/renderer/src/i18n/de-DE/settings.json`:
- Line 1146: The "bedrockVerifyTip" localization string is outdated (mentions
"Claude 3.5 Sonnet") and should be updated to reflect the current "fetch-models"
verification behavior; locate the "bedrockVerifyTip" entry in the German i18n
file and replace the message with a concise statement explaining that
verification uses the fetch-models method and that lacking model access will
cause verification to fail without affecting other models, keeping tone and
phrasing consistent with surrounding entries.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ec849fb6-087c-4d9d-9207-757765a65390
📒 Files selected for processing (28)
package.jsonsrc/main/presenter/llmProviderPresenter/aiSdk/providerFactory.tssrc/main/presenter/llmProviderPresenter/providerRegistry.tssrc/main/presenter/llmProviderPresenter/providers/aiSdkProvider.tssrc/renderer/settings/components/BedrockProviderSettingsDetail.vuesrc/renderer/src/i18n/da-DK/settings.jsonsrc/renderer/src/i18n/de-DE/settings.jsonsrc/renderer/src/i18n/en-US/settings.jsonsrc/renderer/src/i18n/es-ES/settings.jsonsrc/renderer/src/i18n/fa-IR/settings.jsonsrc/renderer/src/i18n/fr-FR/settings.jsonsrc/renderer/src/i18n/he-IL/settings.jsonsrc/renderer/src/i18n/id-ID/settings.jsonsrc/renderer/src/i18n/it-IT/settings.jsonsrc/renderer/src/i18n/ja-JP/settings.jsonsrc/renderer/src/i18n/ko-KR/settings.jsonsrc/renderer/src/i18n/ms-MY/settings.jsonsrc/renderer/src/i18n/pl-PL/settings.jsonsrc/renderer/src/i18n/pt-BR/settings.jsonsrc/renderer/src/i18n/ru-RU/settings.jsonsrc/renderer/src/i18n/tr-TR/settings.jsonsrc/renderer/src/i18n/vi-VN/settings.jsonsrc/renderer/src/i18n/zh-CN/settings.jsonsrc/renderer/src/i18n/zh-HK/settings.jsonsrc/renderer/src/i18n/zh-TW/settings.jsonsrc/shared/contracts/domainSchemas.tssrc/shared/types/presenters/legacy.presenters.d.tssrc/shared/types/presenters/llmprovider.presenter.d.ts
- Unify env var names: use AWS_REGION/AWS_ACCESS_KEY_ID/AWS_SECRET_ACCESS_KEY consistently (matches providerFactory.ts) - Validate authMode value before assignment in settings UI
Collaborator
|
LGTM |
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
2 participants
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 / 概述
Add AWS Profile authentication support for the Bedrock provider. Users can now choose between static Access Keys or a named AWS Profile (
~/.aws/credentials) for authentication.为 AWS Bedrock provider 新增 Profile 认证模式。用户无需手动输入 Access Key，可以直接使用本地配置的 AWS Profile 进行认证。
Screenshots / 截图
authentication by profile
AWS Profile Mode configuration
Verification / 验证
Design / 设计
Authentication Modes / 认证模式
Technical Approach / 技术方案
fromNodeProviderChainfrom@aws-sdk/credential-providers— resolves the full AWS credential chain (SSO, env vars, process credentials, IMDSv2, etc.)generate-texttofetch-models(ListFoundationModels API) — no longer depends on a specific model ID being availableUI Design / UI 设计
Radio selector at the top of Bedrock settings:
Changes / 变更
package.json@aws-sdk/credential-providerssrc/shared/types/presenters/*.d.tsauthMode,profiletoAwsBedrockCredentialsrc/shared/contracts/domainSchemas.tssrc/main/.../providerFactory.tsfromNodeProviderChainfor profile modesrc/main/.../aiSdkProvider.tsfetchBedrockModelsprofile supportsrc/main/.../providerRegistry.tscheckStrategy: 'fetch-models'BedrockProviderSettingsDetail.vueTest Plan / 测试
pnpm run typecheck— passpnpm run lint— passpnpm run i18n— no missing keysada credentials update --profile bedrock-gwSummary by CodeRabbit
New Features
Chores