Open
Conversation
Contributor
📝 WalkthroughWalkthroughAdds AWS Bedrock authentication: types, events, and credentials; state machine flows and validation service; Bedrock SDK integration for providers; UI credential forms and buttons; and dependency addition for Bedrock SDK. Changes
Sequence DiagramsequenceDiagram
actor User
participant LoggedOutWindow
participant AIStateMachine
participant ValidatorService
participant BedrockSDK
participant AuthStore
User->>LoggedOutWindow: Click "Enter AWS Bedrock credentials"
LoggedOutWindow->>AIStateMachine: Dispatch AUTH_WITH_AWS_BEDROCK
AIStateMachine->>AIStateMachine: Transition to awsBedrockFlow
AIStateMachine->>User: Show credential form
User->>AIStateMachine: Submit SUBMIT_AWS_CREDENTIALS
AIStateMachine->>AIStateMachine: Transition to validatingAwsCredentials
AIStateMachine->>ValidatorService: validateAwsCredentials(credentials)
ValidatorService->>BedrockSDK: Init client with accessKeyId/secret/region
BedrockSDK-->>ValidatorService: Success / Error
alt Validation Success
ValidatorService->>AuthStore: Store AwsBedrockSecrets
AuthStore-->>ValidatorService: Stored (token-like value)
ValidatorService-->>AIStateMachine: Validation success
AIStateMachine->>AIStateMachine: Transition to Authenticated
AIStateMachine->>User: Initialize Bedrock provider for features
else Validation Failure
ValidatorService-->>AIStateMachine: Error
AIStateMachine->>AIStateMachine: Transition back to awsBedrockFlow
AIStateMachine->>User: Display error, allow retry
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 3
🧹 Nitpick comments (4)
workspaces/mi/mi-extension/src/rpc-managers/ai-features/rpc-manager.ts (1)
481-486: Method name is now misleading for its broadened behavior.Line 483 still says
hasAnthropicApiKey, but Line 485 now includes AWS Bedrock credentials too. Consider introducing a neutral method name and keeping the old one as a compatibility alias.♻️ Suggested refactor
- async hasAnthropicApiKey(): Promise<boolean | undefined> { - const loginMethod = await getLoginMethod(); - return loginMethod === LoginMethod.ANTHROPIC_KEY || loginMethod === LoginMethod.AWS_BEDROCK; - } + async hasExternalCredentials(): Promise<boolean> { + const loginMethod = await getLoginMethod(); + return loginMethod === LoginMethod.ANTHROPIC_KEY || loginMethod === LoginMethod.AWS_BEDROCK; + } + + // Backward-compatible alias; remove after API migration. + async hasAnthropicApiKey(): Promise<boolean> { + return this.hasExternalCredentials(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/rpc-managers/ai-features/rpc-manager.ts` around lines 481 - 486, Rename the misleading method hasAnthropicApiKey to a neutral name like usesAnthropicOrBedrockCredentials (or isUsingAnthropicOrBedrockCredentials) to reflect it returns true for both LoginMethod.ANTHROPIC_KEY and LoginMethod.AWS_BEDROCK; update the method signature and internal implementation in rpc-manager.ts accordingly, then add a short compatibility alias named hasAnthropicApiKey that simply calls the new usesAnthropicOrBedrockCredentials method (keeping the same Promise<boolean | undefined> return type) and mark the alias as deprecated in a comment; finally search for and update any internal references to call the new method name (or leave callers working via the alias until you opt to refactor them).workspaces/mi/mi-visualizer/src/views/AIPanel/index.tsx (1)
60-60: You can merge duplicated auth-form branches here.Lines 63-78 render the same component for both API-key and Bedrock flows; consolidating this keeps future changes from drifting.
♻️ Suggested simplification
- if (authenticatingState === 'apiKeyFlow' || authenticatingState === 'validatingApiKey') { - setViewComponent( - <WaitingForLoginSection - loginMethod={loginMethod} - isValidating={isValidating} - errorMessage={errorMessage} - /> - ); - } else if (authenticatingState === 'awsBedrockFlow' || authenticatingState === 'validatingAwsCredentials') { + if ( + authenticatingState === 'apiKeyFlow' || + authenticatingState === 'validatingApiKey' || + authenticatingState === 'awsBedrockFlow' || + authenticatingState === 'validatingAwsCredentials' + ) { setViewComponent( <WaitingForLoginSection loginMethod={loginMethod} isValidating={isValidating} errorMessage={errorMessage} /> ); } else {Also applies to: 63-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-visualizer/src/views/AIPanel/index.tsx` at line 60, The auth-form rendering is duplicated for the API-key and Bedrock branches; collapse those branches by using the existing isValidating boolean (computed from authenticatingState === 'validatingApiKey' || authenticatingState === 'validatingAwsCredentials') to render the shared auth form once. Replace the two separate conditional renderings with a single conditional that checks isValidating (or the inverse if appropriate) and returns the same auth-form JSX/component, leaving other branch-specific logic outside or above that shared render. Ensure you update any props or handlers used in both branches so the single auth-form instance receives the correct values.workspaces/mi/mi-extension/src/ai-features/auth.ts (1)
485-490: ReuseAwsBedrockSecretsinstead of redefining the credential shape.This avoids type drift between validation input and stored/retrieved Bedrock secrets.
♻️ Proposed refactor
-export const validateAwsCredentials = async (credentials: { - accessKeyId: string; - secretAccessKey: string; - region: string; - sessionToken?: string; -}): Promise<AIUserToken> => { +export const validateAwsCredentials = async (credentials: AwsBedrockSecrets): Promise<AIUserToken> => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/ai-features/auth.ts` around lines 485 - 490, The function validateAwsCredentials currently defines its own credential shape; replace that ad-hoc type with the existing AwsBedrockSecrets type to avoid type drift — update the validateAwsCredentials signature to accept credentials: AwsBedrockSecrets (keeping sessionToken optional), import AwsBedrockSecrets where needed, and adjust any call sites to pass the stored secret object directly so the validator and the stored/retrieved Bedrock secrets remain consistent.workspaces/mi/mi-extension/src/ai-features/connection.ts (1)
56-57: Align Bedrock provider implementation with its caching contract.
getBedrockProvidercurrently recreates the client every time despite claiming cached behavior.♻️ Proposed refactor
let cachedAnthropic: ReturnType<typeof createAnthropic> | null = null; let cachedBedrock: ReturnType<typeof createAmazonBedrock> | null = null; +let cachedBedrockKey: string | null = null; let cachedAuthMethod: LoginMethod | null = null; @@ const getBedrockProvider = async (): Promise<ReturnType<typeof createAmazonBedrock>> => { const credentials = await getAwsBedrockCredentials(); if (!credentials) { throw new Error("Authentication failed: Unable to get AWS Bedrock credentials"); } - // Always recreate to ensure fresh credentials - cachedBedrock = createAmazonBedrock({ - region: credentials.region, - accessKeyId: credentials.accessKeyId, - secretAccessKey: credentials.secretAccessKey, - sessionToken: credentials.sessionToken, - }); + const cacheKey = `${credentials.region}:${credentials.accessKeyId}:${credentials.sessionToken ?? ''}`; + if (!cachedBedrock || cachedBedrockKey !== cacheKey) { + cachedBedrock = createAmazonBedrock({ + region: credentials.region, + accessKeyId: credentials.accessKeyId, + secretAccessKey: credentials.secretAccessKey, + sessionToken: credentials.sessionToken, + }); + cachedBedrockKey = cacheKey; + } return cachedBedrock; };Also applies to: 275-293
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/ai-features/connection.ts` around lines 56 - 57, getBedrockProvider is recreating the Bedrock client on every call despite the cachedBedrock/cachedAuthMethod variables; update getBedrockProvider to return cachedBedrock when it exists and cachedAuthMethod matches the current LoginMethod, otherwise create a new client via createAmazonBedrock, assign both cachedBedrock and cachedAuthMethod, and return it. Also apply the same caching check/assignment pattern to the other Bedrock-creation block (the similar logic around createAmazonBedrock) so the client is only recreated when the auth method changes or cache is null.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workspaces/mi/mi-extension/src/ai-features/aiMachine.ts`:
- Around line 472-476: The AWS_BEDROCK branch currently treats presence of
accessKeyId alone as sufficient to mark a session authenticated; update the
checks in the AWS_BEDROCK handling (the block that reads
credentials?.loginMethod === LoginMethod.AWS_BEDROCK and sets tokenData) to
require the full Bedrock credential set (accessKeyId, secretAccessKey, and
region) before setting tokenData and entering Authenticated; do the same fix for
the other AWS_BEDROCK handling later in the file (the second block around the
tokenData assignment at lines ~548-554) so both branches validate all three
fields from credentials.secrets before proceeding.
In `@workspaces/mi/mi-extension/src/ai-features/connection.ts`:
- Around line 44-53: The helper getBedrockRegionalPrefix currently defaults
unknown region families to "us", which can produce incorrect Bedrock model IDs;
update getBedrockRegionalPrefix to fail fast: detect when region does not start
with "us-", "eu-", or "ap-" and throw a descriptive Error (including the
original region string) instead of returning "us", so callers immediately know
the region is unsupported and can handle it.
In
`@workspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsx`:
- Around line 371-378: The EyeButton instances (e.g., the one that toggles
showAwsAccessKey using setShowAwsAccessKey and renders Codicon) currently only
use a generic title; update each icon-only toggle (EyeButton components at the
three locations) to include a field-specific aria-label (e.g., "Show AWS access
key" / "Hide AWS access key" for showAwsAccessKey) and add
aria-pressed={showAwsAccessKey} so screen readers can identify which credential
field is being toggled and its state; ensure the aria-label text corresponds to
the same state logic used for title and Codicon name and apply the same pattern
for the other toggles referenced in the diff.
---
Nitpick comments:
In `@workspaces/mi/mi-extension/src/ai-features/auth.ts`:
- Around line 485-490: The function validateAwsCredentials currently defines its
own credential shape; replace that ad-hoc type with the existing
AwsBedrockSecrets type to avoid type drift — update the validateAwsCredentials
signature to accept credentials: AwsBedrockSecrets (keeping sessionToken
optional), import AwsBedrockSecrets where needed, and adjust any call sites to
pass the stored secret object directly so the validator and the stored/retrieved
Bedrock secrets remain consistent.
In `@workspaces/mi/mi-extension/src/ai-features/connection.ts`:
- Around line 56-57: getBedrockProvider is recreating the Bedrock client on
every call despite the cachedBedrock/cachedAuthMethod variables; update
getBedrockProvider to return cachedBedrock when it exists and cachedAuthMethod
matches the current LoginMethod, otherwise create a new client via
createAmazonBedrock, assign both cachedBedrock and cachedAuthMethod, and return
it. Also apply the same caching check/assignment pattern to the other
Bedrock-creation block (the similar logic around createAmazonBedrock) so the
client is only recreated when the auth method changes or cache is null.
In `@workspaces/mi/mi-extension/src/rpc-managers/ai-features/rpc-manager.ts`:
- Around line 481-486: Rename the misleading method hasAnthropicApiKey to a
neutral name like usesAnthropicOrBedrockCredentials (or
isUsingAnthropicOrBedrockCredentials) to reflect it returns true for both
LoginMethod.ANTHROPIC_KEY and LoginMethod.AWS_BEDROCK; update the method
signature and internal implementation in rpc-manager.ts accordingly, then add a
short compatibility alias named hasAnthropicApiKey that simply calls the new
usesAnthropicOrBedrockCredentials method (keeping the same Promise<boolean |
undefined> return type) and mark the alias as deprecated in a comment; finally
search for and update any internal references to call the new method name (or
leave callers working via the alias until you opt to refactor them).
In `@workspaces/mi/mi-visualizer/src/views/AIPanel/index.tsx`:
- Line 60: The auth-form rendering is duplicated for the API-key and Bedrock
branches; collapse those branches by using the existing isValidating boolean
(computed from authenticatingState === 'validatingApiKey' || authenticatingState
=== 'validatingAwsCredentials') to render the shared auth form once. Replace the
two separate conditional renderings with a single conditional that checks
isValidating (or the inverse if appropriate) and returns the same auth-form
JSX/component, leaving other branch-specific logic outside or above that shared
render. Ensure you update any props or handlers used in both branches so the
single auth-form instance receives the correct values.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
common/config/rush/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (10)
workspaces/mi/mi-core/src/state-machine-types.tsworkspaces/mi/mi-extension/package.jsonworkspaces/mi/mi-extension/src/ai-features/aiMachine.tsworkspaces/mi/mi-extension/src/ai-features/auth.tsworkspaces/mi/mi-extension/src/ai-features/connection.tsworkspaces/mi/mi-extension/src/rpc-managers/ai-features/rpc-manager.tsworkspaces/mi/mi-visualizer/src/views/AIPanel/component/AIChatHeader.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsxworkspaces/mi/mi-visualizer/src/views/AIPanel/index.tsxworkspaces/mi/mi-visualizer/src/views/LoggedOutWindow/index.tsx
workspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsx
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds AWS Bedrock as a new authentication method for the MI Copilot, enabling users to authenticate using AWS credentials as an alternative to the existing Anthropic API key and MI Intel SSO methods.
Changes:
- Added AWS Bedrock login flow with credential input UI and validation
- Integrated AWS Bedrock SDK for model access with regional prefix support
- Extended state machine to handle AWS authentication events and states
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| workspaces/mi/mi-visualizer/src/views/LoggedOutWindow/index.tsx | Added button to trigger AWS Bedrock authentication flow |
| workspaces/mi/mi-visualizer/src/views/AIPanel/index.tsx | Extended authentication state checks to include AWS Bedrock validation |
| workspaces/mi/mi-visualizer/src/views/AIPanel/component/WaitingForLoginSection.tsx | Implemented AWS credential input form with validation and secure field visibility toggles |
| workspaces/mi/mi-visualizer/src/views/AIPanel/component/AIChatHeader.tsx | Updated header to display AWS Bedrock authentication status |
| workspaces/mi/mi-extension/src/rpc-managers/ai-features/rpc-manager.ts | Modified API key check to include AWS Bedrock credentials |
| workspaces/mi/mi-extension/src/ai-features/connection.ts | Added Bedrock provider initialization, model mapping, and regional prefix logic |
| workspaces/mi/mi-extension/src/ai-features/auth.ts | Implemented AWS credential validation, storage, and retrieval functions |
| workspaces/mi/mi-extension/src/ai-features/aiMachine.ts | Extended state machine with AWS Bedrock authentication states and transitions |
| workspaces/mi/mi-extension/package.json | Added @ai-sdk/amazon-bedrock dependency |
| workspaces/mi/mi-core/src/state-machine-types.ts | Added AWS Bedrock types, events, and login method enum |
Files not reviewed (1)
- common/config/rush/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
workspaces/mi/mi-extension/src/ai-features/connection.ts (1)
44-57:⚠️ Potential issue | 🟠 MajorDo not silently map unknown Bedrock regions to
us.Line 56 currently falls back to
us, which can hide bad config and produce incorrect model IDs. This should fail fast with a descriptive error.🐛 Proposed fix
export const getBedrockRegionalPrefix = (region: string): string => { const prefix = region.split('-')[0]; switch (prefix) { case 'us': case 'eu': case 'ap': case 'ca': case 'sa': case 'me': case 'af': return prefix; default: - return 'us'; + throw new Error(`Unsupported AWS Bedrock region prefix: ${region}`); } };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@workspaces/mi/mi-extension/src/ai-features/connection.ts` around lines 44 - 57, The getBedrockRegionalPrefix function currently defaults unknown region prefixes to 'us', which can hide misconfiguration; change it to throw an Error instead: when the prefix extracted in getBedrockRegionalPrefix is not one of the allowed values ('us','eu','ap','ca','sa','me','af'), throw a clear, descriptive error that includes the original region string (e.g., "Unsupported Bedrock region: <region>") so callers fail fast and can correct their config.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@workspaces/mi/mi-extension/src/ai-features/connection.ts`:
- Around line 283-317: The code calls getAwsBedrockCredentials() twice which can
yield different snapshots and cause mismatched region/model IDs; fix by
obtaining credentials once inside getAnthropicClient and passing that same
snapshot into getBedrockProvider (or change getBedrockProvider to accept a
credentials parameter) so both provider creation and getBedrockRegionalPrefix
use the identical credentials; update references to cachedBedrock creation in
createAmazonBedrock to use the passed credentials and remove the second
getAwsBedrockCredentials() call.
---
Duplicate comments:
In `@workspaces/mi/mi-extension/src/ai-features/connection.ts`:
- Around line 44-57: The getBedrockRegionalPrefix function currently defaults
unknown region prefixes to 'us', which can hide misconfiguration; change it to
throw an Error instead: when the prefix extracted in getBedrockRegionalPrefix is
not one of the allowed values ('us','eu','ap','ca','sa','me','af'), throw a
clear, descriptive error that includes the original region string (e.g.,
"Unsupported Bedrock region: <region>") so callers fail fast and can correct
their config.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
workspaces/mi/mi-extension/src/ai-features/aiMachine.tsworkspaces/mi/mi-extension/src/ai-features/connection.ts
Comment on lines
+283
to
+317
| const getBedrockProvider = async (): Promise<ReturnType<typeof createAmazonBedrock>> => { | ||
| const credentials = await getAwsBedrockCredentials(); | ||
| if (!credentials) { | ||
| throw new Error("Authentication failed: Unable to get AWS Bedrock credentials"); | ||
| } | ||
|
|
||
| // Always recreate to ensure fresh credentials | ||
| cachedBedrock = createAmazonBedrock({ | ||
| region: credentials.region, | ||
| accessKeyId: credentials.accessKeyId, | ||
| secretAccessKey: credentials.secretAccessKey, | ||
| sessionToken: credentials.sessionToken, | ||
| }); | ||
|
|
||
| return cachedBedrock; | ||
| }; | ||
|
|
||
| export const getAnthropicClient = async (model: AnthropicModel): Promise<any> => { | ||
| const loginMethod = await getLoginMethod(); | ||
|
|
||
| if (loginMethod === LoginMethod.AWS_BEDROCK) { | ||
| const bedrockProvider = await getBedrockProvider(); | ||
| const credentials = await getAwsBedrockCredentials(); | ||
| if (!credentials) { | ||
| throw new Error("Authentication failed: Unable to get AWS Bedrock credentials"); | ||
| } | ||
| const regionalPrefix = getBedrockRegionalPrefix(credentials.region); | ||
| const bedrockModelId = BEDROCK_MODEL_MAP[model]; | ||
| if (!bedrockModelId) { | ||
| throw new Error(`No Bedrock model mapping found for: ${model}`); | ||
| } | ||
| const fullModelId = `${regionalPrefix}.${bedrockModelId}`; | ||
| logDebug(`Using Bedrock model: ${fullModelId}`); | ||
| return bedrockProvider(fullModelId); | ||
| } |
Contributor
There was a problem hiding this comment.
Use a single credential snapshot for Bedrock provider creation and model ID derivation.
getAwsBedrockCredentials() is called twice (Line 284 and Line 305). If credentials change between those reads, provider region and model prefix can diverge in one request path.
🛠️ Proposed fix
-const getBedrockProvider = async (): Promise<ReturnType<typeof createAmazonBedrock>> => {
- const credentials = await getAwsBedrockCredentials();
- if (!credentials) {
- throw new Error("Authentication failed: Unable to get AWS Bedrock credentials");
- }
-
- // Always recreate to ensure fresh credentials
- cachedBedrock = createAmazonBedrock({
- region: credentials.region,
- accessKeyId: credentials.accessKeyId,
- secretAccessKey: credentials.secretAccessKey,
- sessionToken: credentials.sessionToken,
- });
-
- return cachedBedrock;
-};
+const getBedrockProvider = (credentials: {
+ region: string;
+ accessKeyId: string;
+ secretAccessKey: string;
+ sessionToken?: string;
+}): ReturnType<typeof createAmazonBedrock> => {
+ return createAmazonBedrock({
+ region: credentials.region,
+ accessKeyId: credentials.accessKeyId,
+ secretAccessKey: credentials.secretAccessKey,
+ sessionToken: credentials.sessionToken,
+ });
+};
export const getAnthropicClient = async (model: AnthropicModel): Promise<any> => {
const loginMethod = await getLoginMethod();
if (loginMethod === LoginMethod.AWS_BEDROCK) {
- const bedrockProvider = await getBedrockProvider();
const credentials = await getAwsBedrockCredentials();
if (!credentials) {
throw new Error("Authentication failed: Unable to get AWS Bedrock credentials");
}
+ const bedrockProvider = getBedrockProvider(credentials);
const regionalPrefix = getBedrockRegionalPrefix(credentials.region);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@workspaces/mi/mi-extension/src/ai-features/connection.ts` around lines 283 -
317, The code calls getAwsBedrockCredentials() twice which can yield different
snapshots and cause mismatched region/model IDs; fix by obtaining credentials
once inside getAnthropicClient and passing that same snapshot into
getBedrockProvider (or change getBedrockProvider to accept a credentials
parameter) so both provider creation and getBedrockRegionalPrefix use the
identical credentials; update references to cachedBedrock creation in
createAmazonBedrock to use the passed credentials and remove the second
getAwsBedrockCredentials() call.
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
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 by CodeRabbit