Conversation
dosubot
bot
added
the
size:M
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
|
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 (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds Amazon Bedrock as a built-in provider by extending the LLMProvider union to include "bedrock" and updating the model factory to handle a "bedrock" branch. The model factory imports and constructs an Amazon Bedrock provider, using an AWS credential provider created via fromNodeProviderChain. sdk/package.json adds two runtime dependencies: 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
sdk/package.json (1)
36-37: Duplicate build on publish.Both
prepareandprepublishOnlyinvokenpm run build. When publishing, this triggers two consecutive builds. Consider removing one or guarding against redundant work.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/package.json` around lines 36 - 37, The package.json defines both "prepare" and "prepublishOnly" scripts invoking "npm run build", causing redundant builds on publish; remove one of these keys (either "prepare" or "prepublishOnly") or change one to a no-op/guard so "npm run build" runs only once, e.g., keep "prepare": "npm run build" and delete "prepublishOnly" (or vice versa), or implement an environment-check guard around "npm run build" so the "prepare" and "prepublishOnly" scripts do not both execute the same build step.sdk/src/model-factory.ts (1)
258-287: Document Bedrock environment variable configuration.The code uses custom environment variables (
BEDROCK_ROLE_ARN,BEDROCK_SOURCE_PROFILE,BEDROCK_PROFILE,AWS_REGION,AWS_PROFILE) with a specific precedence order for credential resolution, but they are not documented anywhere in the repository. Add documentation (insdk/README.mdor a configuration guide) explaining these variables and their resolution order: temporary credentials viaBEDROCK_ROLE_ARN, then INI profile viaBEDROCK_PROFILE/AWS_PROFILE, then Node provider chain fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/model-factory.ts` around lines 258 - 287, Document the Bedrock environment variable configuration and resolution order referenced in the "bedrock" branch of sdk/src/model-factory.ts: explain BEDROCK_ROLE_ARN, BEDROCK_SOURCE_PROFILE, BEDROCK_PROFILE, AWS_REGION, and AWS_PROFILE and the precedence used by the code (use temporary credentials via BEDROCK_ROLE_ARN with fromTemporaryCredentials/master fromIni(source profile) first, then INI profile via fromIni(BEDROCK_PROFILE or AWS_PROFILE), then fallback to fromNodeProviderChain), and show how these map to the createAmazonBedrock call and region/credentialProvider behavior so users know how to configure credentials and region for createAmazonBedrock/model selection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/package.json`:
- Around line 69-77: The package.json entry for "@ai-sdk/amazon-bedrock" is
pinned to v3.0.84 and needs to be updated to the v4 major line; change the
dependency version for "@ai-sdk/amazon-bedrock" from "^3.0.84" to the current v4
range (for example "^4.0.83"), then run your package manager (npm/yarn/pnpm
install) and run the test/build to surface any breaking changes in code paths
that reference amazon-bedrock client initialization or types (search for usages
of "@ai-sdk/amazon-bedrock" and update imports or constructor calls as
required).
In `@sdk/src/model-factory.ts`:
- Around line 265-268: The role-assumption branch uses fromIni with profile:
bedrockSourceProfile ?? bedrockProfile and therefore ignores AWS_PROFILE,
causing inconsistent behavior with the non-role branch; update the source
credential selection in the bedrockRoleArn branch (where fromIni is called) to
use bedrockSourceProfile ?? bedrockProfile ?? process.env.AWS_PROFILE (or an
equivalent helper that reads AWS_PROFILE) so it falls back to AWS_PROFILE like
the non-role path, ensuring bedrockRoleArn, fromIni, bedrockSourceProfile and
bedrockProfile behave consistently.
---
Nitpick comments:
In `@sdk/package.json`:
- Around line 36-37: The package.json defines both "prepare" and
"prepublishOnly" scripts invoking "npm run build", causing redundant builds on
publish; remove one of these keys (either "prepare" or "prepublishOnly") or
change one to a no-op/guard so "npm run build" runs only once, e.g., keep
"prepare": "npm run build" and delete "prepublishOnly" (or vice versa), or
implement an environment-check guard around "npm run build" so the "prepare" and
"prepublishOnly" scripts do not both execute the same build step.
In `@sdk/src/model-factory.ts`:
- Around line 258-287: Document the Bedrock environment variable configuration
and resolution order referenced in the "bedrock" branch of
sdk/src/model-factory.ts: explain BEDROCK_ROLE_ARN, BEDROCK_SOURCE_PROFILE,
BEDROCK_PROFILE, AWS_REGION, and AWS_PROFILE and the precedence used by the code
(use temporary credentials via BEDROCK_ROLE_ARN with
fromTemporaryCredentials/master fromIni(source profile) first, then INI profile
via fromIni(BEDROCK_PROFILE or AWS_PROFILE), then fallback to
fromNodeProviderChain), and show how these map to the createAmazonBedrock call
and region/credentialProvider behavior so users know how to configure
credentials and region for createAmazonBedrock/model selection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c7a02be5-a277-453a-9e00-ee0167bf0b0e
📒 Files selected for processing (3)
sdk/package.jsonsdk/src/model-factory.tssdk/src/types.ts
sdk/src/model-factory.ts
Outdated
| if (bedrockRoleArn) { | ||
| const sourceCredentials = fromIni({ | ||
| profile: bedrockSourceProfile ?? bedrockProfile, | ||
| }); |
There was a problem hiding this comment.
Inconsistent AWS_PROFILE fallback for role assumption.
When BEDROCK_ROLE_ARN is set, source credentials derive from BEDROCK_SOURCE_PROFILE ?? BEDROCK_PROFILE, ignoring AWS_PROFILE. However, the non-role branch (line 277) does fall back to AWS_PROFILE. This asymmetry may surprise users who set BEDROCK_ROLE_ARN alongside AWS_PROFILE expecting it to be used.
🔧 Proposed fix for consistency
const sourceCredentials = fromIni({
- profile: bedrockSourceProfile ?? bedrockProfile,
+ profile: bedrockSourceProfile ?? bedrockProfile ?? process.env.AWS_PROFILE,
});📝 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.
| if (bedrockRoleArn) { | |
| const sourceCredentials = fromIni({ | |
| profile: bedrockSourceProfile ?? bedrockProfile, | |
| }); | |
| if (bedrockRoleArn) { | |
| const sourceCredentials = fromIni({ | |
| profile: bedrockSourceProfile ?? bedrockProfile ?? process.env.AWS_PROFILE, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@sdk/src/model-factory.ts` around lines 265 - 268, The role-assumption branch
uses fromIni with profile: bedrockSourceProfile ?? bedrockProfile and therefore
ignores AWS_PROFILE, causing inconsistent behavior with the non-role branch;
update the source credential selection in the bedrockRoleArn branch (where
fromIni is called) to use bedrockSourceProfile ?? bedrockProfile ??
process.env.AWS_PROFILE (or an equivalent helper that reads AWS_PROFILE) so it
falls back to AWS_PROFILE like the non-role path, ensuring bedrockRoleArn,
fromIni, bedrockSourceProfile and bedrockProfile behave consistently.
sdk/package.json
Outdated
| "@ai-sdk/mistral": "^2.0.19", | ||
| "@ai-sdk/openai": "^2.0.32", | ||
| "@ai-sdk/xai": "^2.0.29", | ||
| "@aws-sdk/credential-providers": "^3.998.0", |
There was a problem hiding this comment.
LGTM post-pin resolve
There was a problem hiding this comment.
hello, version is fixed :) thanks for your work, sdk is working great :)
dosubot
bot
added
size:S
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdk/src/model-factory.ts (1)
258-264: Consider relaxingapiKeyrequirement for Bedrock callers.This branch does not consume
options.apiKey, but the public options type still requires it. Making API-key requirements provider-specific would avoid dummy values and improve SDK ergonomics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/src/model-factory.ts` around lines 258 - 264, The public options type requires apiKey even though the "bedrock" branch (in model-factory.ts using fromNodeProviderChain() and createAmazonBedrock()) doesn't use it; change the options typing to make apiKey optional for Bedrock callers by introducing a provider-discriminated union or making apiKey optional in the generic options interface, update any factory signature that returns ProviderLanguageModel to accept the new type, and remove any forced/dummy apiKey usages so callers of createAmazonBedrock/fromNodeProviderChain can omit apiKey.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sdk/src/model-factory.ts`:
- Around line 258-264: The public options type requires apiKey even though the
"bedrock" branch (in model-factory.ts using fromNodeProviderChain() and
createAmazonBedrock()) doesn't use it; change the options typing to make apiKey
optional for Bedrock callers by introducing a provider-discriminated union or
making apiKey optional in the generic options interface, update any factory
signature that returns ProviderLanguageModel to accept the new type, and remove
any forced/dummy apiKey usages so callers of
createAmazonBedrock/fromNodeProviderChain can omit apiKey.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4784a0e8-a04d-4c95-a2ab-d601c129c54d
⛔ Files ignored due to path filters (1)
sdk/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
sdk/package.jsonsdk/src/model-factory.ts
…l lib to ProviderV3 if bump to 4.0.83
3 participants
No description provided.