feat:Add Amazon Bedrock to support the leading large language models such as Amazon Nova, Claude, Llama, Mistral, and others. #5746
Conversation
修改： app/api/auth.ts 新文件： app/api/bedrock.ts 新文件： app/api/bedrock/models.ts 新文件： app/api/bedrock/utils.ts 修改： app/client/api.ts 新文件： app/client/platforms/bedrock.ts 修改： app/components/settings.tsx 修改： app/config/server.ts 修改： app/constant.ts 修改： app/locales/cn.ts 修改： app/locales/en.ts 修改： app/store/access.ts 修改： app/utils.ts 修改： package.json
修改： app/api/bedrock/models.ts 修改： app/api/bedrock/utils.ts 修改： app/client/api.ts 修改： app/client/platforms/bedrock.ts 新文件： app/components/chat-actions.tsx 修改： app/components/chat.module.scss 修改： app/components/chat.tsx 修改： app/constant.ts 新文件： app/icons/document.svg 修改： app/locales/cn.ts 修改： app/locales/en.ts
|
@glayyiyi is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces extensive changes to integrate support for the Bedrock API across various components of the application. Key modifications include the addition of new handlers, validation logic, and utility functions for AWS credentials. The routing logic is updated to accommodate Bedrock-specific requests, while the authentication and configuration components are enhanced to manage AWS credentials securely. Additionally, localization files are modified to support Bedrock service settings, and new constants are defined to facilitate API interactions. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🔇 Additional comments (5)app/constant.ts (5)
The Bedrock provider is properly added to all relevant enums ( Also applies to: 123-123, 148-148
The Nova models are correctly added to the
Consider enhancing AWS configuration management While the current implementation provides basic functionality, there are a few improvements to consider:
Consider this enhanced implementation: export const Bedrock = {
ChatPath: "model",
ApiVersion: "2023-11-01",
- getEndpoint: (region: string = "us-west-2") =>
- `https://bedrock-runtime.${region}.amazonaws.com`,
+ getEndpoint: (config: { region?: string; customEndpoint?: string }) => {
+ if (config.customEndpoint) return config.customEndpoint;
+ const region = config.region || "us-west-2";
+ return `https://bedrock-runtime.${region}.amazonaws.com`;
+ },
};
-// Get the region from access store for BEDROCK_BASE_URL
-export const BEDROCK_BASE_URL = Bedrock.getEndpoint();
+export const BEDROCK_BASE_URL = Bedrock.getEndpoint({});
The Bedrock models are correctly integrated into the Let's verify the model integration: ✅ Verification successfulBedrock models integration verified successfully The integration is correctly implemented with:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Check for any potential duplicate model names across providers
rg -g '*.ts' -A 5 'DEFAULT_MODELS.*map.*name' | grep -E '(name|provider)' | sort | uniq -c | sort -nr
Length of output: 103 Script: #!/bin/bash
# Let's first check the content of the constant.ts file to understand the model integration
rg -g 'constant.ts' -B 5 -A 10 'bedrockModels.*map'
# Also check for any references to bedrockModels to see its definition
rg -g '*.ts' 'const.*bedrockModels'
Length of output: 666 Script: #!/bin/bash
# Let's see the actual bedrock models array definition
rg -g 'constant.ts' -B 2 -A 5 'const bedrockModels = \['
# Also check if there are any duplicate model entries in DEFAULT_MODELS
rg -g 'constant.ts' 'DEFAULT_MODELS\s*=' -A 50
Length of output: 2272 Script: #!/bin/bash
# Let's see the complete bedrock models list and the rest of DEFAULT_MODELS
rg -g 'constant.ts' -A 10 'const bedrockModels = \['
# Also check if there are any other usages of these bedrock models
rg -g '*.ts' 'bedrock.*nova|anthropic\.claude'
Length of output: 1166
The Bedrock models array includes various models with specific version numbers (e.g., Let's verify the model versions: Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 37
🧹 Outside diff range and nitpick comments (12)
app/api/auth.ts (1)
57-59: Consider moving header names to constants.The header names should be defined as constants at the top of the file for better maintainability and reusability.
// Add at the top of the file const BEDROCK_HEADERS = { REGION: 'X-Region', ACCESS_KEY: 'X-Access-Key', SECRET_KEY: 'X-Secret-Key', } as const;app/config/server.ts (1)
146-146: Consider regrouping cloud provider checksThe
isBedrockcheck should be grouped with other cloud providers (like Azure) rather than being placed betweenisStabilityandisAzurefor better code organization and maintainability.const isStability = !!process.env.STABILITY_API_KEY; - const isBedrock = !!process.env.BEDROCK_API_KEY; const isAzure = !!process.env.AZURE_URL; const isGoogle = !!process.env.GOOGLE_API_KEY; const isAnthropic = !!process.env.ANTHROPIC_API_KEY; + const isBedrock = !!process.env.BEDROCK_API_KEY; const isTencent = !!process.env.TENCENT_API_KEY;app/constant.ts (1)
312-324: Consider adding model capabilities documentationWhile the models are well-organized by provider, adding comments about each model's capabilities (context window, specialized features, etc.) would help developers choose the appropriate model for their use case.
Example enhancement:
const bedrockModels = [ // Claude Models + // anthropic.claude-3-haiku-20240307-v1:0 - Fast, efficient model with 200K context "anthropic.claude-3-haiku-20240307-v1:0", + // anthropic.claude-3-sonnet-20240229-v1:0 - Balanced performance with 200K context "anthropic.claude-3-sonnet-20240229-v1:0", // ... add descriptions for other modelsapp/locales/en.ts (1)
350-375: Add default endpoint value in the subtitle.The Bedrock endpoint subtitle mentions "Default: " but doesn't specify the actual default value. This information would be helpful for users.
Apply this change:
Title: "AWS Bedrock Endpoint", - SubTitle: "Custom endpoint for AWS Bedrock API. Default: ", + SubTitle: "Custom endpoint for AWS Bedrock API. Default: https://bedrock-runtime.[region].amazonaws.com",app/components/chat.tsx (2)
624-626: Remove commented code.The commented code for storing document content should be either implemented or removed to maintain code cleanliness.
555-635: Consider streaming large files.For better memory management with large files, consider implementing streaming for file processing instead of loading the entire file into memory.
app/components/chat-actions.tsx (1)
183-183: Localize the button text 'Upload Document'The text 'Upload Document' is hard-coded and may not be localized, affecting internationalization. Use localization functions to ensure proper translation.
Apply this diff:
- <div className="text">Upload Document</div> + <div className="text">{locale.uploadDocument}</div>Ensure that the key
uploadDocumentis added to your localization files.app/api/bedrock.ts (3)
68-68: Make the default AWS region configurableHardcoding the default region to
"us-west-2"may not suit all deployments. Consider making the default region configurable through environment variables or application settings.Apply this diff to use an environment variable for the default region:
- const region = req.headers.get("X-Region") || "us-west-2"; + const DEFAULT_REGION = process.env.AWS_DEFAULT_REGION || "us-west-2"; + const region = req.headers.get("X-Region") || DEFAULT_REGION;
95-97: Validate request body fieldsEnsure that all required fields in the request body are present and valid before proceeding. This prevents runtime errors and improves robustness.
Apply this diff to add validation for
modelId:const body = (await req.json()) as ConverseRequest; + if (!body.modelId) { + throw new Error("modelId is required in the request body"); + } const { modelId } = body;
44-50: Add unit tests for authentication and request handlingImplementing unit tests for the
authfunction and request handlers ensures that authentication logic works correctly and helps prevent future regressions.Would you like assistance in generating unit tests for these functions?
app/api/bedrock/models.ts (1)
200-216: Handle empty content arrays appropriatelyIn the
convertContentToAWSfunction, when no valid content blocks are found, a default text block with an empty string is returned (lines 211-213). Consider whether returning an empty array or handling this case differently would be more appropriate to avoid unintended empty messages.Suggested change:
// If no valid blocks, provide a default text block if (blocks.length === 0) { - return [{ text: "" }]; + return []; }Ensure that downstream functions can handle an empty content array without errors.
app/client/platforms/bedrock.ts (1)
171-174: Simplify Response Text ConstructionThe current logic for constructing
responseTextis unnecessarily complex, involving an array for a single message.Simplify the code as follows:
- let responseText = ""; - const responseTexts = [responseText]; - responseTexts.push(Locale.Error.Unauthorized); - responseText = responseTexts.join("\n\n"); + const responseText = Locale.Error.Unauthorized;This streamlines the code by directly assigning the error message to
responseText.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/icons/document.svgis excluded by!**/*.svg
📒 Files selected for processing (18)
app/api/[provider]/[...path]/route.ts(2 hunks)app/api/auth.ts(1 hunks)app/api/bedrock.ts(1 hunks)app/api/bedrock/models.ts(1 hunks)app/api/bedrock/utils.ts(1 hunks)app/client/api.ts(8 hunks)app/client/platforms/bedrock.ts(1 hunks)app/components/chat-actions.tsx(1 hunks)app/components/chat.module.scss(2 hunks)app/components/chat.tsx(3 hunks)app/components/settings.tsx(2 hunks)app/config/server.ts(3 hunks)app/constant.ts(6 hunks)app/locales/cn.ts(2 hunks)app/locales/en.ts(2 hunks)app/store/access.ts(5 hunks)app/utils.ts(1 hunks)package.json(1 hunks)
🧰 Additional context used
📓 Learnings (2)
app/components/chat-actions.tsx (1)
Learnt from: Dakai
PR: ChatGPTNextWeb/ChatGPT-Next-Web#5585
File: app/components/chat.tsx:1482-1528
Timestamp: 2024-10-06T07:59:31.943Z
Learning: In `app/components/chat.tsx`, maintain similarity between `uploadDocument()` and `uploadImage()` functions to aid future maintenance.
app/components/chat.tsx (1)
Learnt from: Dakai
PR: ChatGPTNextWeb/ChatGPT-Next-Web#5585
File: app/components/chat.tsx:1482-1528
Timestamp: 2024-10-06T07:59:31.943Z
Learning: In `app/components/chat.tsx`, maintain similarity between `uploadDocument()` and `uploadImage()` functions to aid future maintenance.
🪛 Biome
app/client/platforms/bedrock.ts
[error] 345-345: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
app/components/settings.tsx
[error] 980-980: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 996-996: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1012-1012: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1028-1028: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (23)
app/api/[provider]/[...path]/route.ts (3)
4-4: LGTM: Import statement follows established patterns.
The import for the Bedrock handler follows the consistent pattern used for other providers.
24-25: LGTM: Switch case implementation is consistent.
The Bedrock case follows the established pattern of other providers, correctly passing through the request and params.
Also applies to: 32-32
Line range hint 53-71: Verify AWS credentials handling and region compatibility.
Since this is running on Edge Runtime and Bedrock requires AWS credentials:
- Ensure AWS credentials are properly handled in the Edge environment
- Verify that the preferred regions align with Bedrock service availability
✅ Verification successful
AWS credentials and region handling are properly implemented for Edge Runtime
The verification shows that:
- AWS credentials are securely handled via request headers (X-Region, X-Access-Key, X-Secret-Key) and properly validated in the auth middleware
- Region handling is implemented correctly with a default fallback to "us-west-2"
- The BedrockRuntimeClient is properly instantiated with the credentials and region in the Edge environment
- Comprehensive error handling is in place for credential and region-related issues
The current implementation aligns with Edge Runtime requirements and AWS Bedrock service constraints. No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for AWS credential handling and region-related code
# Check for AWS credential handling in the codebase
echo "Checking AWS credential handling..."
rg -l "AWS" --type ts
# Look for region configurations
echo "Checking region configurations..."
rg "region|AWS_REGION" --type ts
# Check for environment variable handling
echo "Checking environment variable handling..."
rg "process\.env\.AWS|getEnvVar.*AWS" --type ts
Length of output: 1793
Script:
#!/bin/bash
# Let's examine the AWS credential handling in Bedrock-related files more closely
# Check Bedrock client implementation
echo "Checking Bedrock client implementation..."
rg -A 10 "BedrockClient" app/client/platforms/bedrock.ts
# Check AWS credential handling in auth.ts
echo "Checking auth implementation..."
rg -A 10 "export async function" app/api/auth.ts
# Check Bedrock API implementation
echo "Checking Bedrock API implementation..."
rg -A 10 "export async function" app/api/bedrock.ts
# Check available Bedrock regions
echo "Checking Bedrock regions configuration..."
rg -A 10 "availableRegions|bedrockRegions"
Length of output: 846
Script:
#!/bin/bash
# Let's check the actual implementation details
# Check how credentials are handled in the Bedrock API
echo "Checking Bedrock API file content..."
cat app/api/bedrock.ts
# Check auth implementation
echo "Checking auth file content..."
cat app/api/auth.ts
# Check if there's any Edge-specific credential handling
echo "Checking Edge runtime configurations..."
rg -A 5 "runtime.*=.*edge"
Length of output: 13839
package.json (2)
54-54: LGTM!
The zustand dependency is properly pinned to version 4.3.8.
55-55: 🛠️ Refactor suggestion
Consider AWS SDK versioning and dependencies.
A few recommendations regarding the AWS SDK integration:
- Consider pinning the version instead of using caret (^) to prevent potential breaking changes from minor version updates.
- Version 3.679.0 is quite old. Consider updating to the latest version for security fixes and features.
- Based on the PR objectives mentioning AWS authentication and document uploads, you might need additional AWS SDK packages:
@aws-sdk/credential-providersfor authentication@aws-sdk/client-s3for document uploads
Consider updating to:
- "@aws-sdk/client-bedrock-runtime": "^3.679.0"
+ "@aws-sdk/client-bedrock-runtime": "3.499.0",
+ "@aws-sdk/credential-providers": "3.499.0",
+ "@aws-sdk/client-s3": "3.499.0"app/api/auth.ts (2)
61-65: LGTM! Proper credential masking in logs.
The implementation correctly masks sensitive AWS credentials in logs, which is a good security practice.
55-76: Verify Bedrock authentication integration.
Let's verify that this authentication block is properly integrated with the rest of the Bedrock-related changes mentioned in the PR.
✅ Verification successful
Let me gather more information about the Bedrock handler integration to ensure proper authentication flow.
Let me check one final aspect to verify the complete authentication flow.
Based on the comprehensive verification results, I can now provide the final response:
✓ Bedrock authentication is properly integrated
The authentication implementation is correctly integrated with the Bedrock handler and shows consistent header usage across the codebase:
- Auth check is properly performed in
bedrock.tsusingauth(req, ModelProvider.Bedrock) - AWS credential headers (X-Region, X-Access-Key, X-Secret-Key) are consistently used in:
- Frontend:
app/client/api.tsandapp/client/platforms/bedrock.ts - Backend:
app/api/auth.tsandapp/api/bedrock.ts
- Frontend:
- The handler includes proper error handling and credential validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Bedrock authentication integration
# Test 1: Check if Bedrock handler uses this auth function
echo "Checking Bedrock handler integration..."
rg -l "auth.*ModelProvider\.Bedrock" app/api/
# Test 2: Check for consistent header usage across files
echo "Checking header usage consistency..."
rg "X-(Region|Access-Key|Secret-Key)" app/
Length of output: 1199
Script:
#!/bin/bash
# Check the implementation of Bedrock handler to verify auth integration
echo "Checking Bedrock handler implementation..."
rg -A 5 "export.*bedrock" app/api/bedrock.ts
# Check auth function usage in bedrock.ts
echo "Checking auth function usage..."
rg "auth.*\(" app/api/bedrock.ts
# Check ModelProvider enum definition
echo "Checking ModelProvider definition..."
rg "enum ModelProvider" app/
Length of output: 462
Script:
#!/bin/bash
# Check the ModelProvider enum values and Bedrock handler implementation
echo "Checking ModelProvider enum values..."
rg -A 10 "enum ModelProvider" app/constant.ts
# Check the complete bedrock.ts implementation
echo "Checking complete Bedrock handler..."
cat app/api/bedrock.ts
# Check auth function implementation
echo "Checking auth function implementation..."
rg -A 10 "auth.*=.*async" app/api/auth.ts
Length of output: 8734
app/store/access.ts (2)
7-7: LGTM! Constants follow established patterns.
The addition of Bedrock-related constants follows the same pattern as other providers, maintaining consistency in the codebase.
Also applies to: 30-30
209-209: LGTM! Authorization check properly integrated.
The Bedrock validation is correctly integrated into the authorization flow, maintaining consistency with other providers.
app/config/server.ts (1)
15-18: LGTM: Environment variables properly declared
The Bedrock-specific environment variables are correctly declared as optional strings and follow the established pattern in the file.
app/utils.ts (1)
288-290: LGTM! The Bedrock plugin visibility logic aligns with Anthropic's implementation.
The added condition correctly mirrors Anthropic's plugin visibility rules for Claude models, maintaining consistent behavior across providers.
Let's verify the consistent handling of Claude models across providers:
✅ Verification successful
Plugin visibility logic is correctly implemented for Claude models
The verification confirms that both Anthropic and Bedrock providers have identical conditions for handling Claude models in the showPlugins function:
- Both check for
!model.includes("claude-2") - Both return
truewhen the condition is met - The implementation maintains consistent behavior across providers
The code in app/constant.ts also shows proper model definitions for various Claude versions, supporting this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent handling of Claude models across Anthropic and Bedrock providers
# Expected: Both providers should have similar conditions for Claude models
# Search for Claude model handling across the codebase
rg -A 5 "claude-2" --type ts
Length of output: 1764
app/components/chat.module.scss (2)
78-87: LGTM: Well-structured icon styles!
The new .icon class implementation follows best practices:
- Uses flexbox for proper centering
- Maintains consistent dimensions with parent container
- Follows the existing BEM methodology
408-408: LGTM: Proper structural fix
The closing brace is correctly placed, maintaining proper SCSS nesting structure.
app/constant.ts (2)
121-121: LGTM!
The Bedrock provider is correctly added to both enums, maintaining consistency with other providers.
Also applies to: 135-135
524-534: LGTM!
The Bedrock models are correctly integrated into DEFAULT_MODELS with appropriate provider configuration and consistent sorting.
app/locales/cn.ts (1)
84-84: LGTM!
The translation for document upload functionality is properly implemented.
app/locales/en.ts (1)
85-85: LGTM: Document upload translation added.
The translation for document upload functionality is clear and consistent with existing entries.
app/components/settings.tsx (1)
1753-1753: LGTM: Clean integration of Bedrock configuration.
The Bedrock configuration component is properly integrated into the provider list, maintaining consistency with other provider configurations.
app/components/chat.tsx (2)
551-552: LGTM: Clean provider flag implementation.
The flag is well-named and follows single responsibility principle.
668-675: LGTM: Clean UI implementation for document upload.
The document upload button implementation follows existing patterns and maintains consistency with the image upload feature.
app/api/bedrock/utils.ts (1)
357-400: LGTM
The convertBedrockResponseToMultimodal function effectively translates the Bedrock response into the expected MultimodalContent format.
app/api/bedrock/models.ts (1)
243-245: Verify the logic that skips messages with the same role
At lines 243-245, the code skips messages if they have the same role as the previous message. This could lead to loss of important messages, particularly if multiple consecutive messages from the same role are expected. Ensure that this behavior aligns with the application requirements.
Explanation:
// Skip if same role as previous message
if (idx > 0 && curr.role === nonSystemMessages[idx - 1].role) {
return acc;
}Consider whether messages from the same role should be merged or all messages should be included. If merging is intended, you might concatenate the content of messages with the same role.
app/client/platforms/bedrock.ts (1)
271-273: Verify Parameter Naming Consistency
The inferenceConfig object uses camelCase for maxTokens, whereas modelConfig uses snake_case max_tokens. This inconsistency might cause misalignment with the API expectations.
Check if the Bedrock API expects parameters in camelCase or snake_case. If camelCase is required, ensure consistency:
- maxTokens: modelConfig.max_tokens,
+ maxTokens: modelConfig.maxTokens,Adjust the naming in modelConfig or map the properties accordingly to match the API specification.
|
1.一个pr不要包含太多不同的功能 |
|
删除： app/api/bedrock/models.ts 删除： app/api/bedrock/utils.ts 修改： app/client/platforms/bedrock.ts 删除： app/components/chat-actions.tsx 修改： app/components/chat.tsx 修改： app/constant.ts 删除： app/icons/document.svg
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
app/api/bedrock.ts (2)
125-227: Split complex content conversion logic into smaller functionsThe
convertContentToAWSBlockfunction is complex with multiple nested conditions. Consider splitting it into smaller, focused functions for each content type.+function convertImageContent(imageUrl: string): ContentBlock | null { + const base64Match = imageUrl.match(/^data:image\/([a-zA-Z]*);base64,([^"]*)/); + if (!base64Match) return null; + + const format = base64Match[1].toLowerCase(); + if (!["png", "jpeg", "gif", "webp"].includes(format)) return null; + + validateImageSize(base64Match[2]); + return { + image: { + format: format as "png" | "jpeg" | "gif" | "webp", + source: { + bytes: Uint8Array.from(Buffer.from(base64Match[2], "base64")), + }, + }, + }; +}
444-538: Consider implementing stream backpressure handlingThe
ReadableStreamimplementation doesn't handle backpressure, which could lead to memory issues with large responses. Consider implementing thepullmethod to handle backpressure.app/client/platforms/bedrock.ts (4)
27-27: Remove console log to prevent exposing sensitive informationLogging the entire response object may inadvertently expose sensitive information. Consider removing or restricting this log statement in production code.
Apply this diff:
- console.log("[Response] bedrock response: ", res);
100-100: Remove unused variable 'format'The variable
formatis assigned but never used, which can be removed to clean up the code.Apply this diff:
- const format = base64Match[1].toLowerCase();
278-305: Avoid logging request bodies containing sensitive informationEven though binary data is masked, logging the entire request body may still expose sensitive information. Consider removing or sanitizing this log statement to avoid potential security risks.
Apply this diff:
- console.log( - "[Bedrock] Request body:", - JSON.stringify( - { - ...requestBody, - messages: requestBody.messages.map((msg) => ({ - ...msg, - content: Array.isArray(msg.content) - ? msg.content.map((c) => ({ - type: c.type, - ...(c.document - ? { - document: { - format: c.document.format, - name: c.document.name, - }, - } - : {}), - ...(c.image_url ? { image_url: { url: "[BINARY]" } } : {}), - ...(c.text ? { text: c.text } : {}), - })) - : msg.content, - })), - }, - null, - 2, - ), - );
158-403: Refactor 'chat' method to improve maintainabilityThe
chatmethod is quite lengthy and contains complex nested logic, which can make it difficult to read and maintain. Consider breaking it down into smaller, reusable helper functions to enhance code clarity and reusability.🧰 Tools
🪛 Biome
[error] 345-345: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/api/bedrock.ts(1 hunks)app/client/platforms/bedrock.ts(1 hunks)app/constant.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/constant.ts
🧰 Additional context used
🪛 Biome
app/client/platforms/bedrock.ts
[error] 345-345: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (2)
app/api/bedrock.ts (2)
387-396: Previous review comment about not exposing internal error details is still valid.
542-545: Previous review comment about adding security headers is still valid.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
app/client/platforms/bedrock.ts (2)
95-101: Avoid using 'as any' to maintain type safetyAssigning an array to
messages[i]and casting it toanycan compromise type safety and may lead to runtime errors or unexpected behavior.Instead of replacing
messages[i]with an array, consider inserting the fake assistant message directly into themessagesarray. Here's how you can refactor the code:for (let i = 0; i < messages.length - 1; i++) { const message = messages[i]; const nextMessage = messages[i + 1]; if (keys.includes(message.role) && keys.includes(nextMessage.role)) { - messages[i] = [ - message, - { - role: "assistant", - content: ";", - }, - ] as any; + messages.splice(i + 1, 0, { + role: "assistant", + content: ";", + }); + i++; // Increment i to skip the inserted assistant message } }This modification maintains the array's type integrity and avoids unnecessary type casting.
54-54: Limit logging of sensitive informationLogging entire responses or error objects may expose sensitive data, which is a security risk, especially in production environments.
Modify the logging statements to provide necessary information without revealing sensitive details:
// Line 54 -console.log("[Response] bedrock response: ", res); +console.log("[Bedrock] Response received"); // Line 301 -console.error("[Bedrock] Chat error:", e); +console.error("[Bedrock] Chat error:", e.message);This approach maintains useful logging while protecting sensitive data.
Also applies to: 301-301
修改： app/api/auth.ts 修改： app/api/bedrock.ts 修改： app/client/api.ts 修改： app/client/platforms/bedrock.ts 修改： app/components/settings.tsx 修改： app/config/server.ts 修改： app/constant.t
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (3)
app/client/platforms/bedrock.ts (2)
20-28: Consider adding validation for media typesThe
MultiBlockContenttype allows any string formedia_type. Consider using a union type to restrict it to supported MIME types for better type safety.export type MultiBlockContent = { type: "image" | "text"; source?: { type: string; - media_type: string; + media_type: "image/jpeg" | "image/png" | "image/gif" | "image/webp"; data: string; }; text?: string; };
35-39: Document system message handlingThe
ClaudeMappermaps 'system' role to 'user' which might be non-intuitive. Consider adding a comment explaining why this mapping is necessary for Claude compatibility.const ClaudeMapper = { assistant: "assistant", user: "user", + // Claude doesn't support system messages directly, so we map them to user messages system: "user", } as const;app/constant.ts (1)
233-235: Add ExampleEndpoint to Bedrock constant object.Other provider objects include an ExampleEndpoint property. Consider adding it for consistency:
export const Bedrock = { ChatPath: "converse", + ExampleEndpoint: BEDROCK_BASE_URL, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
.env.template(1 hunks)app/api/auth.ts(1 hunks)app/api/bedrock.ts(1 hunks)app/client/api.ts(8 hunks)app/client/platforms/bedrock.ts(1 hunks)app/components/settings.tsx(2 hunks)app/config/server.ts(3 hunks)app/constant.ts(7 hunks)app/store/access.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/api/auth.ts
🧰 Additional context used
🪛 Biome
app/components/settings.tsx
[error] 980-980: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 996-996: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1012-1012: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1028-1028: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (10)
.env.template (1)
69-70: LGTM! Improved formatting.
The added newline enhances readability and maintains consistent spacing between sections.
app/store/access.ts (1)
7-7: LGTM: URL configuration follows established patterns.
The Bedrock URL configuration follows the same pattern as other providers, correctly handling both app and API path scenarios.
Also applies to: 30-30
app/config/server.ts (1)
147-147: LGTM!
The Bedrock flag follows the same pattern as other provider flags.
app/client/api.ts (4)
15-15: LGTM: BedrockApi import
The import is correctly placed alongside other platform-specific API imports.
141-143: LGTM: Bedrock provider initialization
The Bedrock provider case is properly integrated into the switch statement, following the established pattern.
355-356: LGTM: Bedrock client API initialization
The Bedrock provider case is properly integrated into the switch statement, maintaining consistency with other providers.
324-347: Verify error handling for AWS credentials
Ensure proper error handling is in place when AWS credentials are invalid or missing.
✅ Verification successful
Let me gather more information about error handling by searching for validation and error handling patterns in the codebase.
Let me search for more general error handling patterns and validation checks.
Based on the analysis of the codebase, I can now provide a final response:
AWS credentials validation is properly handled
The codebase has sufficient error handling for AWS credentials:
- Client-side validation through
ensure()function inapp/store/access.tschecks for required AWS credentials (region, access key, secret key) - Server-side validation in
app/api/bedrock.tsreturns 401 error when credentials are missing - Error handling for Bedrock API calls with proper error messages and status codes
- Stream error handling with detailed error information including original status codes and messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for AWS credentials validation and error handling
# Test: Search for AWS credentials validation
rg -A 5 "awsAccessKey|awsSecretKey|awsRegion" --type ts
# Test: Search for error handling related to AWS credentials
ast-grep --pattern 'try {
$$$
aws$$$
$$$
} catch (error) {
$$$
}'
Length of output: 4725
Script:
#!/bin/bash
# Search for validation of AWS credentials
ast-grep --pattern 'ensure($_, ["awsAccessKey", "awsSecretKey", "awsRegion"])'
# Search for error handling in Bedrock API calls
rg -B 3 -A 5 "catch.*error.*Bedrock" --type ts
# Search for error messages related to AWS credentials
rg "Error.*aws|aws.*Error" --type ts
Length of output: 267
Script:
#!/bin/bash
# Search for error handling in Bedrock-related files
rg -B 3 -A 5 "catch|throw|error" app/api/bedrock.ts
# Search for validation functions
ast-grep --pattern 'function $_($$_) {
$$$
if (!$_) {
$$$
}
$$$
}'
# Check for error messages in locale files
rg "bedrock.*error|aws.*error" --type json
Length of output: 52025
app/constant.ts (2)
316-330: LGTM! Well-organized model definitions.
The Bedrock models are well-organized by model families (Claude, Meta Llama, Mistral) with clear version numbers and helpful comments.
530-540: LGTM! Consistent provider configuration.
The Bedrock provider configuration in DEFAULT_MODELS follows the established pattern and maintains consistency with other providers.
app/components/settings.tsx (1)
1753-1753: LGTM: Bedrock component integration.
The Bedrock configuration component is properly integrated into the provider list, maintaining consistency with other provider implementations.
修改： app/client/platforms/bedrock.ts
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
app/api/bedrock.ts (3)
13-13: Consider using an enum for allowed pathsUsing a Set with string literals reduces type safety. Consider using an enum to ensure type safety and maintainability.
-const ALLOWED_PATH = new Set(["converse"]); +enum AllowedPath { + Converse = "converse" +} +const ALLOWED_PATH = new Set(Object.values(AllowedPath));
24-24: Add validation for modelId formatThe modelId field should be validated to ensure it matches AWS Bedrock model formats.
+const VALID_MODEL_PREFIXES = ['anthropic.', 'amazon.', 'meta.']; + +function isValidModelId(modelId: string): boolean { + return VALID_MODEL_PREFIXES.some(prefix => modelId.startsWith(prefix)); +}
95-239: Add rate limiting and timeout handlingThe handler function lacks important production safeguards:
- No rate limiting to prevent abuse
- No timeout handling for long-running requests
Consider implementing:
- Rate limiting middleware
- Request timeout handling
Example implementation:
import { rateLimit } from 'express-rate-limit'; import { setTimeout } from 'timers/promises'; // Rate limiting middleware const limiter = rateLimit({ windowMs: 15 * 60 * 1000, // 15 minutes max: 100 // limit each IP to 100 requests per windowMs }); // Timeout wrapper const withTimeout = async (promise: Promise<any>, timeoutMs: number) => { const timeout = setTimeout(timeoutMs); try { return await Promise.race([promise, timeout.then(() => { throw new Error('Request timeout'); })]); } finally { timeout.clear(); } };app/client/platforms/bedrock.ts (3)
21-34: Add JSDoc documentation for type definitionsThe
MultiBlockContentandAnthropicMessagetypes would benefit from JSDoc documentation explaining their purpose and usage.Add documentation like this:
+/** + * Represents a block of content that can be either text or image. + * Used for multimodal messages in the Bedrock API. + */ export type MultiBlockContent = { type: "image" | "text"; source?: { type: string; media_type: string; data: string; }; text?: string; }; +/** + * Represents a message in the Claude model format. + * Maps internal roles to Claude-specific roles. + */ export type AnthropicMessage = { role: (typeof ClaudeMapper)[keyof typeof ClaudeMapper]; content: string | MultiBlockContent[]; };
43-45: Enhance error messages for unimplemented methodsThe error messages for unimplemented methods should provide more context about the current limitations and potential alternatives.
speech(options: SpeechOptions): Promise<ArrayBuffer> { - throw new Error("Speech not implemented for Bedrock."); + throw new Error("Speech synthesis is not supported by the Bedrock API. Consider using Amazon Polly for text-to-speech functionality."); } usage(): Promise<LLMUsage> { - throw new Error("Method not implemented."); + throw new Error("Usage tracking is not currently supported for Bedrock. Use AWS CloudWatch for monitoring Bedrock API usage."); } models(): Promise<LLMModel[]> { - throw new Error("Method not implemented."); + throw new Error("Dynamic model listing is not supported. Available models are defined in the Bedrock console."); }Also applies to: 292-297
58-64: Simplify model configuration initializationThe current configuration merge could be simplified using object destructuring.
-const modelConfig = { - ...useAppConfig.getState().modelConfig, - ...useChatStore.getState().currentSession().mask.modelConfig, - ...{ - model: options.config.model, - }, -}; +const { modelConfig: baseConfig } = useAppConfig.getState(); +const { modelConfig: maskConfig } = useChatStore.getState().currentSession().mask; +const modelConfig = { + ...baseConfig, + ...maskConfig, + model: options.config.model, +};
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/client/platforms/bedrock.ts (2)
24-33: Enhance type safety of ClaudeMapper and AnthropicMessageThe type definitions could be more robust with literal types and proper type guards.
Consider this improvement:
-const ClaudeMapper = { +const ClaudeMapper = { assistant: "assistant", user: "user", system: "user", -} as const; +} as const satisfies Record<string, "assistant" | "user">; -export type AnthropicMessage = { +export type AnthropicMessage = { - role: (typeof ClaudeMapper)[keyof typeof ClaudeMapper]; + role: typeof ClaudeMapper[keyof typeof ClaudeMapper]; content: string | MultiBlockContent[]; + // Add runtime type guard + isAnthropicMessage: (message: unknown) => message is AnthropicMessage; };
285-293: Mark unimplemented methods appropriatelyThe usage and models methods should clearly indicate they're not implemented, similar to the speech method.
Consider this improvement:
async usage() { - return { - used: 0, - total: 0, - }; + throw new Error("Usage tracking not implemented for Bedrock."); } async models() { - return []; + throw new Error("Model listing not implemented for Bedrock."); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/client/platforms/bedrock.ts(1 hunks)
🔇 Additional comments (1)
app/client/platforms/bedrock.ts (1)
36-38: LGTM: Appropriate error handling for unimplemented method
The speech method correctly throws an error indicating the functionality is not implemented.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
app/api/bedrock.ts (1)
118-121: Add rate limiting to protect the APIThe handler lacks rate limiting, which could lead to excessive API usage.
Consider implementing rate limiting using a middleware or a service like Redis. Example implementation:
import rateLimit from 'express-rate-limit' const limiter = rateLimit({ windowMs: 15 * 60 * 1000, // 15 minutes max: 100 // limit each IP to 100 requests per windowMs }); // Apply before the main handler export const bedrockHandler = limiter(async (req, params) => { return handle(req, params); });
修改： app/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
app/api/bedrock.ts (2)
43-43: Translate comment to EnglishComments should be in English for better maintainability and collaboration.
- // llama和mistral模型不支持工具调用 + // Llama and Mistral models do not support tool calls
126-129: Add rate limiting protectionThe endpoint lacks rate limiting protection which could lead to abuse.
Consider implementing rate limiting using Redis or a similar solution to protect the API from abuse. Example implementation:
import rateLimit from 'express-rate-limit' import RedisStore from 'rate-limit-redis' const limiter = rateLimit({ store: new RedisStore({ // ... redis config }), windowMs: 15 * 60 * 1000, // 15 minutes max: 100 // limit each IP to 100 requests per windowMs });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/api/bedrock.ts(1 hunks)app/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/utils.ts
glayyiyi
changed the title
|
Based on the feedback from the previous PR, I have revised and optimized this PR. The scope has been narrowed and focused as suggested, with the following key improvements:
Enhancements for tool use in both the server-side and client-side implementations for Bedrock using the Bedrock Converse API. Here is a summary of the key changes made:
|
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (2)
app/api/bedrock.ts (1)
42-45: Enhance model support documentation and validationThe function has several issues:
- The comment should be in English for consistency
- The model check is too broad and might include future incompatible models
function supportsToolUse(modelId: string): boolean { - // llama和mistral模型不支持工具调用 + // Llama and Mistral models don't support tool calls + const SUPPORTED_MODELS = [ + 'claude-3-sonnet', + 'claude-3-haiku', + 'claude-3-opus' + ]; - return modelId.toLowerCase().includes("claude-3"); + return SUPPORTED_MODELS.some(model => + modelId.toLowerCase().includes(model)); }app/client/platforms/bedrock.ts (1)
24-33: Document the role mapping behavior for system messages.The
ClaudeMappermaps the "system" role to "user", which is a significant behavior that should be documented. This mapping affects how system prompts are handled.Add a comment explaining this behavior:
const ClaudeMapper = { assistant: "assistant", user: "user", + // Claude doesn't support system messages directly, so we map them to user messages system: "user", } as const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
app/api/[provider]/[...path]/route.ts(2 hunks)app/api/auth.ts(1 hunks)app/api/bedrock.ts(1 hunks)app/client/api.ts(8 hunks)app/client/platforms/bedrock.ts(1 hunks)app/components/settings.tsx(2 hunks)app/config/server.ts(3 hunks)app/constant.ts(8 hunks)app/locales/cn.ts(2 hunks)app/locales/en.ts(2 hunks)app/store/access.ts(5 hunks)app/utils.ts(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- app/api/[provider]/[...path]/route.ts
- app/api/auth.ts
- app/config/server.ts
- app/constant.ts
- app/utils.ts
- package.json
🧰 Additional context used
🪛 Biome
app/components/settings.tsx
[error] 981-981: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 997-997: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1013-1013: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1029-1029: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (15)
app/store/access.ts (3)
7-7: LGTM: URL configuration follows established pattern.
The Bedrock URL configuration follows the same pattern as other providers, maintaining consistency in the codebase.
Also applies to: 31-31
213-213: LGTM: Authorization check properly integrated.
The Bedrock validation is correctly integrated into the authorization flow, maintaining consistency with other providers.
160-163: 🛠️ Refactor suggestion
Enhance Bedrock credential validation.
The current validation only checks for the presence of credentials. Consider implementing more robust validation:
isValidBedrock() {
+ const state = get();
+
+ if (state.awsCognitoUser) {
+ // Add Cognito-specific validation when implemented
+ return true;
+ }
+
+ // Basic format validation
+ const isValidRegion = /^[a-z]{2}-[a-z]+-\d{1,2}$/.test(state.awsRegion);
+ const isValidAccessKey = /^[A-Z0-9]{20}$/.test(state.awsAccessKey);
+ const isValidSecretKey = /^[A-Za-z0-9/+=]{40}$/.test(state.awsSecretKey);
+
+ if (!isValidRegion || !isValidAccessKey || !isValidSecretKey) {
+ console.warn('[Access] Invalid AWS credentials format');
+ return false;
+ }
+
+ return true;
- return ensure(get(), ["awsAccessKey", "awsSecretKey", "awsRegion"]);
}Likely invalid or redundant comment.
app/api/bedrock.ts (2)
35-40: 🛠️ Refactor suggestion
Improve type safety for tools configuration
The tools array uses loose typing which could lead to runtime errors. Consider defining a proper interface for the tool configuration.
tools?: {
name: string;
description?: string;
- input_schema: any;
+ input_schema: {
+ type: string;
+ properties?: Record<string, unknown>;
+ required?: string[];
+ additionalProperties?: boolean;
+ [key: string]: unknown;
+ };
+ version?: string;
}[];Likely invalid or redundant comment.
15-21:
Improve error handling in decrypt function
The current implementation silently fails by returning an empty string, which could mask critical issues and make debugging difficult. Consider throwing an error with a descriptive message or implementing proper error logging.
function decrypt(str: string): string {
try {
return Buffer.from(str, "base64").toString().split("").reverse().join("");
} catch (error) {
- return "";
+ console.error("[Bedrock] Failed to decrypt:", error);
+ throw new Error("Failed to decrypt credentials");
}
}Likely invalid or redundant comment.
app/client/platforms/bedrock.ts (1)
49-56: Consider validating model configuration.
The model configuration is merged from multiple sources without validation of required fields.
Add validation for essential configuration parameters:
const modelConfig = {
...useAppConfig.getState().modelConfig,
...useChatStore.getState().currentSession().mask.modelConfig,
...{
model: options.config.model,
},
};
+if (!modelConfig.max_tokens || !modelConfig.temperature || !modelConfig.top_p) {
+ throw new Error("Invalid model configuration: missing required parameters");
+}app/client/api.ts (2)
15-15: LGTM: Import statement properly placed
The BedrockApi import is correctly placed with other platform-specific imports.
142-144: LGTM: Consistent implementation of Bedrock provider
The Bedrock provider is properly integrated into both the ClientApi constructor and getClientApi function, following the established pattern for other providers.
Also applies to: 363-364
app/locales/cn.ts (2)
84-84: LGTM!
The translation for UploadDocument is consistent with the existing translations and correctly represents the document upload functionality.
346-371: Translate Bedrock section to Chinese and add default endpoint.
The Bedrock section needs to be translated to Chinese for consistency with the rest of the localization file. Also, add the default endpoint value.
Apply this diff to translate the strings and add the default endpoint:
Bedrock: {
Region: {
- Title: "AWS Region",
- SubTitle: "The AWS region where Bedrock service is located",
- Placeholder: "us-west-2",
+ Title: "AWS 区域",
+ SubTitle: "Bedrock 服务所在的 AWS 区域",
+ Placeholder: "us-west-2",
},
AccessKey: {
- Title: "AWS Access Key ID",
- SubTitle: "Your AWS access key ID for Bedrock service",
- Placeholder: "AKIA...",
+ Title: "AWS 访问密钥 ID",
+ SubTitle: "用于 Bedrock 服务的 AWS 访问密钥 ID",
+ Placeholder: "AKIA...",
},
SecretKey: {
- Title: "AWS Secret Access Key",
- SubTitle: "Your AWS secret access key for Bedrock service",
- Placeholder: "****",
+ Title: "AWS 私有访问密钥",
+ SubTitle: "用于 Bedrock 服务的 AWS 私有访问密钥",
+ Placeholder: "****",
},
SessionToken: {
- Title: "AWS Session Token (Optional)",
- SubTitle: "Your AWS session token if using temporary credentials",
- Placeholder: "Optional session token",
+ Title: "AWS 会话令牌（可选）",
+ SubTitle: "如果使用临时凭证，请输入 AWS 会话令牌",
+ Placeholder: "可选的会话令牌",
},
Endpoint: {
- Title: "AWS Bedrock Endpoint",
- SubTitle: "Custom endpoint for AWS Bedrock API. Default: ",
+ Title: "AWS Bedrock 接口地址",
+ SubTitle: "自定义 AWS Bedrock API 接口地址。默认：bedrock.{region}.amazonaws.com",
},
},app/locales/en.ts (2)
85-85: LGTM! Clear and consistent translation added for document upload functionality.
The new translation entry follows existing patterns and is appropriately placed alongside similar upload functionality.
350-375: LGTM! Well-structured AWS Bedrock configuration translations.
The translations for AWS Bedrock configuration:
- Follow the established pattern of other provider configurations
- Include clear titles and helpful subtitles
- Properly handle security-sensitive fields
- Provide appropriate placeholder examples
app/components/settings.tsx (3)
967-969: LGTM - Component declaration follows established patterns.
The Bedrock configuration component is correctly implemented using conditional rendering based on the selected provider.
1795-1795: LGTM - Provider integration is correct.
The Bedrock configuration component is properly integrated into the provider list, maintaining consistency with other providers.
1018-1034: Add session token validation.
Consider adding basic validation for the session token when provided.
Let's check if session tokens are handled consistently across the codebase:
#!/bin/bash
# Description: Check session token handling patterns
rg "sessionToken|session.*token" --type ts🧰 Tools
🪛 Biome
[error] 1029-1029: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/client/platforms/bedrock.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/platforms/bedrock.ts
[error] 599-599: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
🔇 Additional comments (5)
app/client/platforms/bedrock.ts (5)
412-419: 🛠️ Refactor suggestion
Improve logging security.
Logging request details could expose sensitive information in production.
- console.debug("[Bedrock Client] Request:", {
- path: chatPath,
- model: modelConfig.model,
- messages: messages.length,
- stream: shouldStream,
- });
+ if (process.env.NODE_ENV !== 'production') {
+ console.debug("[Bedrock Client] Request:", {
+ model: modelConfig.model.split('.')[0], // Log only model family
+ messageCount: messages.length,
+ stream: shouldStream,
+ });
+ }Likely invalid or redundant comment.
334-341:
Validate indices before slicing to prevent runtime errors.
Using indexOf may return -1 if the character is not found, leading to incorrect slice operations and potential runtime exceptions.
const colonIndex = url.indexOf(":");
const semicolonIndex = url.indexOf(";");
const comma = url.indexOf(",");
+if (colonIndex === -1 || semicolonIndex === -1 || comma === -1) {
+ throw new Error("Invalid image URL format");
+}
const mimeType = url.slice(colonIndex + 1, semicolonIndex);
const encodeType = url.slice(semicolonIndex + 1, comma);
const data = url.slice(comma + 1);Likely invalid or redundant comment.
828-834: 🛠️ Refactor suggestion
Improve error handling in stream processing.
The error handling exposes raw error details and lacks proper cleanup.
} catch (err) {
- console.error("[Bedrock Stream Error]:", err);
+ console.error(
+ "[Bedrock Stream]:",
+ err instanceof Error ? err.message : "Stream processing failed"
+ );
- throw err;
+ throw new Error("Failed to process stream response");
}Likely invalid or redundant comment.
47-88:
Improve AWS credentials handling and validation.
The current implementation has several security concerns:
- AWS credentials are exposed in headers
- No validation before use
- No error handling for encryption failures
Consider implementing proper validation and secure credential handling:
async function getBedrockHeaders(
modelId: string,
chatPath: string,
finalRequestBody: any,
shouldStream: boolean,
): Promise<Record<string, string>> {
const accessStore = useAccessStore.getState();
+ // Validate credentials
+ if (!accessStore.awsAccessKey || !accessStore.awsSecretKey || !accessStore.awsRegion) {
+ throw new Error("Missing required AWS credentials");
+ }
const bedrockHeaders = isApp
? await sign({
method: "POST",
url: chatPath,
region: accessStore.awsRegion,
accessKeyId: accessStore.awsAccessKey,
secretAccessKey: accessStore.awsSecretKey,
body: finalRequestBody,
service: "bedrock",
headers: {},
isStreaming: shouldStream,
})
: getHeaders();
if (!isApp) {
const { awsRegion, awsAccessKey, awsSecretKey, encryptionKey } = accessStore;
+ if (!encryptionKey) {
+ throw new Error("Missing encryption key");
+ }
const bedrockHeadersConfig = {
XModelID: modelId,
XEncryptionKey: encryptionKey,
ShouldStream: String(shouldStream),
- Authorization: await createAuthHeader(
- awsRegion,
- awsAccessKey,
- awsSecretKey,
- encryptionKey,
- ),
+ Authorization: await createAuthHeader({
+ region: awsRegion,
+ accessKey: awsAccessKey,
+ secretKey: awsSecretKey,
+ encryptionKey,
+ }).catch(error => {
+ console.error("[Bedrock] Failed to create auth header:", error);
+ throw new Error("Failed to create authorization header");
+ }),
};
Object.assign(bedrockHeaders, bedrockHeadersConfig);
}
return bedrockHeaders;
}Likely invalid or redundant comment.
586-592: 🛠️ Refactor suggestion
Implement proper error handling for unsupported operations.
The methods return dummy values without indicating that the operations are unsupported.
+/**
+ * Usage tracking is not supported for Bedrock API.
+ * @throws {Error} Always throws an error indicating the operation is not supported.
+ */
async usage() {
- return { used: 0, total: 0 };
+ throw new Error(
+ "Usage tracking is not supported for Bedrock API. Use AWS CloudWatch metrics instead."
+ );
}
+/**
+ * Model listing is not supported for Bedrock API.
+ * @throws {Error} Always throws an error indicating the operation is not supported.
+ */
async models() {
- return [];
+ throw new Error(
+ "Model listing is not supported for Bedrock API. Configure available models in AWS Console."
+ );
}Likely invalid or redundant comment.
| formatRequestBody(messages: ChatOptions["messages"], modelConfig: any) { | ||
| const model = modelConfig.model; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type safety to modelConfig parameter.
The method accepts any type for modelConfig which reduces type safety. Consider adding proper typing:
-formatRequestBody(messages: ChatOptions["messages"], modelConfig: any) {
+interface ModelConfig {
+ model: string;
+ max_tokens?: number;
+ temperature?: number;
+ top_p?: number;
+ top_k?: number;
+}
+formatRequestBody(messages: ChatOptions["messages"], modelConfig: ModelConfig) {📝 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.
| formatRequestBody(messages: ChatOptions["messages"], modelConfig: any) { | |
| const model = modelConfig.model; | |
| interface ModelConfig { | |
| model: string; | |
| max_tokens?: number; | |
| temperature?: number; | |
| top_p?: number; | |
| top_k?: number; | |
| } | |
| formatRequestBody(messages: ChatOptions["messages"], modelConfig: ModelConfig) { | |
| const model = modelConfig.model; |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/utils/aws.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/utils/aws.ts
[error] 388-388: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 418-418: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 418-418: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (4)
app/utils/aws.ts (4)
88-90:
Remove sensitive error logging.
Logging encryption errors could expose sensitive information about the encryption operation.
} catch (error) {
- console.error("[Encryption Error]:", error);
throw new Error("Failed to encrypt AWS credentials");
}Likely invalid or redundant comment.
645-646:
Remove detailed error logging.
Detailed error logging could expose sensitive information.
- console.error("[AWS Extract Error] extractMessage Empty response");
return "";Likely invalid or redundant comment.
558-559:
Remove sensitive error logging.
Error logging could expose sensitive information about the request processing.
- console.error("[Bedrock Request] parse error", e);
+ console.warn("Failed to process Bedrock message");Likely invalid or redundant comment.
224-234: 🛠️ Refactor suggestion
Add input validation for signing parameters.
The sign function should validate all required parameters before processing to prevent runtime errors.
+function validateSignParams(params: SignParams): void {
+ const required = ['method', 'url', 'region', 'accessKeyId', 'secretAccessKey', 'service'];
+ const missing = required.filter(param => !params[param as keyof SignParams]);
+ if (missing.length > 0) {
+ throw new Error(`Missing required parameters: ${missing.join(', ')}`);
+ }
+ if (!params.url.startsWith('https://')) {
+ throw new Error('URL must use HTTPS protocol');
+ }
+}
+
export async function sign({
method,
url,
region,
accessKeyId,
secretAccessKey,
body,
service,
headers: customHeaders = {},
isStreaming = true,
}: SignParams): Promise<Record<string, string>> {
+ validateSignParams({ method, url, region, accessKeyId, secretAccessKey, service });Likely invalid or redundant comment.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/client/platforms/bedrock.ts(1 hunks)app/utils/aws.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/platforms/bedrock.ts
[error] 600-600: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
app/utils/aws.ts
[error] 388-388: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 418-418: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 418-418: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (9)
app/utils/aws.ts (4)
88-90:
Remove sensitive error logging.
Logging encryption errors could expose sensitive information.
} catch (error) {
- console.error("[Encryption Error]:", error);
throw new Error("Failed to encrypt AWS credentials");
}Likely invalid or redundant comment.
57-64: 🛠️ Refactor suggestion
Add encryption key validation.
The encryption function should validate the encryption key length to ensure sufficient security.
export async function encrypt(
data: string,
encryptionKey: string,
): Promise<string> {
if (!data) return "";
if (!encryptionKey) {
throw new Error("Encryption key is required for AWS credential encryption");
}
+ if (encryptionKey.length < 32) {
+ throw new Error("Encryption key must be at least 32 characters long");
+ }Likely invalid or redundant comment.
224-234: 🛠️ Refactor suggestion
Add input validation for signing parameters.
The sign function should validate all required parameters before processing.
+function validateSignParams(params: SignParams): void {
+ const required = ['method', 'url', 'region', 'accessKeyId', 'secretAccessKey'];
+ const missing = required.filter(param => !params[param as keyof SignParams]);
+ if (missing.length > 0) {
+ throw new Error(`Missing required parameters: ${missing.join(', ')}`);
+ }
+ if (!params.url.startsWith('https://')) {
+ throw new Error('URL must use HTTPS protocol');
+ }
+}
export async function sign({
method,
url,
region,
accessKeyId,
secretAccessKey,
body,
service,
headers: customHeaders = {},
isStreaming = true,
}: SignParams): Promise<Record<string, string>> {
+ validateSignParams({ method, url, region, accessKeyId, secretAccessKey, service });Likely invalid or redundant comment.
9-10: 🛠️ Refactor suggestion
Replace any type with a specific type definition.
Using Record<string, any> reduces type safety. Consider creating a union type that covers all possible event data structures.
-type ParsedEvent = Record<string, any>;
+type ParsedEvent = {
+ output?: {
+ message?: {
+ content?: Array<{
+ text?: string;
+ toolUse?: {
+ name: string;
+ input: unknown;
+ };
+ }>;
+ };
+ };
+ choices?: Array<{
+ message?: {
+ content?: string;
+ tool_calls?: Array<{
+ id: string;
+ function?: {
+ name: string;
+ arguments: string;
+ };
+ }>;
+ };
+ }>;
+ delta?: {
+ text?: string;
+ type?: string;
+ partial_json?: string;
+ };
+ bytes?: string;
+ body?: string | Record<string, unknown>;
+ type?: string;
+};Likely invalid or redundant comment.
app/client/platforms/bedrock.ts (5)
587-593: 🛠️ Refactor suggestion
Implement proper error handling for unsupported operations.
The methods return dummy values without indicating that the operations are unsupported.
async usage() {
- return { used: 0, total: 0 };
+ throw new Error(
+ "Usage tracking is not supported for Bedrock API. Use AWS CloudWatch metrics instead."
+ );
}
async models() {
- return [];
+ throw new Error(
+ "Model listing is not supported for Bedrock API. Configure available models in AWS Console."
+ );
}Likely invalid or redundant comment.
412-419: 🛠️ Refactor suggestion
Improve logging security.
Debug logging could expose sensitive information in production.
if (process.env.NODE_ENV !== "production") {
console.debug("[Bedrock Client] Request:", {
- path: chatPath,
- model: modelConfig.model,
+ model: modelConfig.model.split('.')[0], // Log only model family
messages: messages.length,
stream: shouldStream,
});
}Likely invalid or redundant comment.
38-44: 🛠️ Refactor suggestion
Improve type safety in Tool interface.
The parameters field using any type reduces type safety.
interface Tool {
function?: {
name?: string;
description?: string;
- parameters?: any;
+ parameters?: {
+ type: string;
+ properties?: Record<string, unknown>;
+ required?: string[];
+ [key: string]: unknown;
+ };
};
}Likely invalid or redundant comment.
829-835: 🛠️ Refactor suggestion
Improve stream error handling and cleanup.
The error handling exposes raw error details and lacks proper cleanup.
} catch (err) {
- console.error("[Bedrock Stream Error]:", err);
- throw err;
+ console.error(
+ "[Bedrock Stream]:",
+ err instanceof Error ? err.message : "Stream processing failed"
+ );
+ throw new Error("Failed to process stream response");
+ } finally {
+ reader?.releaseLock();
+ finish();
}Likely invalid or redundant comment.
596-608: 🛠️ Refactor suggestion
Improve type safety in bedrockStream function.
The function uses Function type and any which reduces type safety.
+interface StreamOptions {
+ onUpdate?: (text: string, chunk: string) => void;
+ onFinish?: (text: string, response: Response) => void;
+ onError?: (error: Error) => void;
+ onBeforeTool?: (tool: ChatMessageTool) => void;
+ onAfterTool?: (tool: ChatMessageTool & { content?: string; isError: boolean }) => void;
+}
+type ToolFunction = (args: Record<string, unknown>) => Promise<unknown>;
function bedrockStream(
modelId: string,
chatPath: string,
- requestPayload: any,
+ requestPayload: Record<string, unknown>,
headers: Record<string, string>,
- funcs: Record<string, Function>,
+ funcs: Record<string, ToolFunction>,
controller: AbortController,
processToolMessage: (
- requestPayload: any,
- toolCallMessage: any,
- toolCallResult: any[],
+ requestPayload: Record<string, unknown>,
+ toolCallMessage: { tool_calls: ChatMessageTool[] },
+ toolCallResult: Array<{ tool_call_id: string; content: string }>,
) => void,
- options: any,
+ options: StreamOptions,
)Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 600-600: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 9
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/utils/aws.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/utils/aws.ts
[error] 388-388: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 418-418: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 418-418: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
| export function processMessage( | ||
| data: ParsedEvent, | ||
| remainText: string, | ||
| runTools: any[], |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any[] with specific type for tools array.
Using any[] for the tools array reduces type safety.
+interface Tool {
+ id: string;
+ type: 'function';
+ function: {
+ name: string;
+ arguments: string;
+ };
+}
- runTools: any[],
+ runTools: Tool[],Also applies to: 568-568
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/utils/aws.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/utils/aws.ts
[error] 387-387: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 417-417: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 417-417: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (3)
app/utils/aws.ts (3)
223-233: 🛠️ Refactor suggestion
Add input validation for signing parameters.
The sign function should validate all required parameters before processing.
+function validateSignParams(params: SignParams): void {
+ const required = ['method', 'url', 'region', 'accessKeyId', 'secretAccessKey'];
+ const missing = required.filter(param => !params[param as keyof SignParams]);
+ if (missing.length > 0) {
+ throw new Error(`Missing required parameters: ${missing.join(', ')}`);
+ }
+ if (!params.url.startsWith('https://')) {
+ throw new Error('URL must use HTTPS protocol');
+ }
+}
+
export async function sign({
method,
url,
region,
accessKeyId,
secretAccessKey,
body,
service,
headers: customHeaders = {},
isStreaming = true,
}: SignParams): Promise<Record<string, string>> {
+ validateSignParams({ method, url, region, accessKeyId, secretAccessKey, service });Likely invalid or redundant comment.
387-388: 🛠️ Refactor suggestion
Fix assignment in condition expression.
Using assignments in condition expressions can lead to confusion and potential bugs.
- while ((match = eventRegex.exec(text)) !== null) {
+ let match;
+ while (match = eventRegex.exec(text)) {Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 387-387: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
12-13: Consider using a more unique separator.
Using a tilde as a separator could be problematic if it appears in base64 encoded data. Consider using a more unique string or implementing proper escaping.
-const SEPARATOR = "~";
+// Use a unique string that's unlikely to appear in base64
+const SEPARATOR = "::BASE64_SEPARATOR::";| export function maskSensitiveValue(value: string): string { | ||
| if (!value) return ""; | ||
| if (value.length <= 6) return value; | ||
| const masked = "*".repeat(value.length - 6); | ||
| return value.slice(0, 3) + masked + value.slice(-3); | ||
| } |
There was a problem hiding this comment.
Improve sensitive value masking security.
The current implementation could be vulnerable to timing attacks. Use constant-time operations when handling sensitive data.
export function maskSensitiveValue(value: string): string {
+ if (typeof value !== 'string') {
+ throw new TypeError('Input must be a string');
+ }
if (!value) return "";
if (value.length <= 6) return value;
- const masked = "*".repeat(value.length - 6);
- return value.slice(0, 3) + masked + value.slice(-3);
+ // Use constant-time operations
+ const masked = Buffer.alloc(value.length - 6, '*').toString();
+ return Buffer.concat([
+ Buffer.from(value.slice(0, 3)),
+ Buffer.from(masked),
+ Buffer.from(value.slice(-3))
+ ]).toString();
}📝 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.
| export function maskSensitiveValue(value: string): string { | |
| if (!value) return ""; | |
| if (value.length <= 6) return value; | |
| const masked = "*".repeat(value.length - 6); | |
| return value.slice(0, 3) + masked + value.slice(-3); | |
| } | |
| export function maskSensitiveValue(value: string): string { | |
| if (typeof value !== 'string') { | |
| throw new TypeError('Input must be a string'); | |
| } | |
| if (!value) return ""; | |
| if (value.length <= 6) return value; | |
| // Use constant-time operations | |
| const masked = Buffer.alloc(value.length - 6, '*').toString(); | |
| return Buffer.concat([ | |
| Buffer.from(value.slice(0, 3)), | |
| Buffer.from(masked), | |
| Buffer.from(value.slice(-3)) | |
| ]).toString(); | |
| } |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/utils/aws.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/utils/aws.ts
[error] 630-645: Illegal use of an export declaration not at the top level
move this declaration to the top level
(parse)
[error] 646-688: Illegal use of an export declaration not at the top level
move this declaration to the top level
(parse)
[error] 688-688: expected } but instead the file ends
the file ends here
(parse)
[error] 387-387: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 417-417: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 417-417: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (2)
app/utils/aws.ts (2)
432-432: 🛠️ Refactor suggestion
Replace any[] with specific type for tools array.
Using any[] for the tools array reduces type safety.
+interface Tool {
+ id: string;
+ type: 'function';
+ function: {
+ name: string;
+ arguments: string;
+ };
+}
- runTools: any[],
+ runTools: Tool[],Likely invalid or redundant comment.
128-133:
Improve sensitive value masking security.
The current implementation could be vulnerable to timing attacks.
export function maskSensitiveValue(value: string): string {
+ if (typeof value !== 'string') {
+ throw new TypeError('Input must be a string');
+ }
if (!value) return "";
if (value.length <= 6) return value;
- const masked = "*".repeat(value.length - 6);
- return value.slice(0, 3) + masked + value.slice(-3);
+ // Use constant-time operations
+ const masked = Buffer.alloc(value.length - 6, '*').toString();
+ return Buffer.concat([
+ Buffer.from(value.slice(0, 3)),
+ Buffer.from(masked),
+ Buffer.from(value.slice(-3))
+ ]).toString();
}Likely invalid or redundant comment.
| export async function sign({ | ||
| method, | ||
| url, | ||
| region, | ||
| accessKeyId, | ||
| secretAccessKey, | ||
| body, | ||
| service, | ||
| headers: customHeaders = {}, | ||
| isStreaming = true, | ||
| }: SignParams): Promise<Record<string, string>> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for signing parameters.
The sign function should validate all required parameters before processing.
+function validateSignParams(params: SignParams): void {
+ const required = ['method', 'url', 'region', 'accessKeyId', 'secretAccessKey'];
+ const missing = required.filter(param => !params[param as keyof SignParams]);
+ if (missing.length > 0) {
+ throw new Error(`Missing required parameters: ${missing.join(', ')}`);
+ }
+ if (!params.url.startsWith('https://')) {
+ throw new Error('URL must use HTTPS protocol');
+ }
+}
+
export async function sign({
method,
url,
region,
accessKeyId,
secretAccessKey,
body,
service,
headers: customHeaders = {},
isStreaming = true,
}: SignParams): Promise<Record<string, string>> {
+ validateSignParams({ method, url, region, accessKeyId, secretAccessKey, service });📝 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.
| export async function sign({ | |
| method, | |
| url, | |
| region, | |
| accessKeyId, | |
| secretAccessKey, | |
| body, | |
| service, | |
| headers: customHeaders = {}, | |
| isStreaming = true, | |
| }: SignParams): Promise<Record<string, string>> { | |
| function validateSignParams(params: SignParams): void { | |
| const required = ['method', 'url', 'region', 'accessKeyId', 'secretAccessKey']; | |
| const missing = required.filter(param => !params[param as keyof SignParams]); | |
| if (missing.length > 0) { | |
| throw new Error(`Missing required parameters: ${missing.join(', ')}`); | |
| } | |
| if (!params.url.startsWith('https://')) { | |
| throw new Error('URL must use HTTPS protocol'); | |
| } | |
| } | |
| export async function sign({ | |
| method, | |
| url, | |
| region, | |
| accessKeyId, | |
| secretAccessKey, | |
| body, | |
| service, | |
| headers: customHeaders = {}, | |
| isStreaming = true, | |
| }: SignParams): Promise<Record<string, string>> { | |
| validateSignParams({ method, url, region, accessKeyId, secretAccessKey, service }); |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
app/utils/aws.ts (1)
Line range hint
833-839: Improve error handling in stream processing.The error handling exposes raw error details and could be more informative while maintaining security.
} catch (err) { - console.error("[Bedrock Stream Error]:", err); - throw err; + console.error( + "[Bedrock Stream]:", + err instanceof Error ? err.message : "Stream processing failed" + ); + throw new Error("Failed to process stream response"); }🧰 Tools
🪛 Biome (1.9.4)
[error] 387-387: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 417-417: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 417-417: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/client/platforms/bedrock.ts(1 hunks)app/utils.ts(2 hunks)app/utils/aws.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/utils.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/platforms/bedrock.ts
[error] 604-604: Don't use 'Function' as a type.
Prefer explicitly define the function shape. This type accepts any function-like value, which can be a common source of bugs.
(lint/complexity/noBannedTypes)
app/utils/aws.ts
[error] 387-387: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 417-417: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 417-417: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (6)
app/utils/aws.ts (2)
591-597: 🛠️ Refactor suggestion
Implement proper error handling for unsupported operations.
The methods return dummy values without indicating that the operations are unsupported.
+/**
+ * Usage tracking is not supported for Bedrock API.
+ * @throws {Error} Always throws an error indicating the operation is not supported.
+ */
async usage() {
- return { used: 0, total: 0 };
+ throw new Error(
+ "Usage tracking is not supported for Bedrock API. Use AWS CloudWatch metrics instead."
+ );
}
+/**
+ * Model listing is not supported for Bedrock API.
+ * @throws {Error} Always throws an error indicating the operation is not supported.
+ */
async models() {
- return [];
+ throw new Error(
+ "Model listing is not supported for Bedrock API. Configure available models in AWS Console."
+ );
}Likely invalid or redundant comment.
128-133: 🛠️ Refactor suggestion
Improve sensitive value masking security.
The current implementation could be vulnerable to timing attacks and lacks input validation.
export function maskSensitiveValue(value: string): string {
+ if (typeof value !== 'string') {
+ throw new TypeError('Input must be a string');
+ }
if (!value) return "";
if (value.length <= 6) return value;
- const masked = "*".repeat(value.length - 6);
- return value.slice(0, 3) + masked + value.slice(-3);
+ // Use constant-time operations
+ const masked = Buffer.alloc(value.length - 6, '*').toString();
+ return Buffer.concat([
+ Buffer.from(value.slice(0, 3)),
+ Buffer.from(masked),
+ Buffer.from(value.slice(-3))
+ ]).toString();
}Likely invalid or redundant comment.
app/client/platforms/bedrock.ts (4)
578-588: 🛠️ Refactor suggestion
Improve error handling type safety and security.
The error handling uses type assertions and exposes raw error details.
} catch (e) {
- console.error("[Bedrock Client] Chat error:", e);
- options.onError?.(e as Error);
+ if (e instanceof Error) {
+ console.error("[Bedrock Client] Chat error:", e.message);
+ options.onError?.(e);
+ } else {
+ console.error("[Bedrock Client] Unknown error occurred");
+ options.onError?.(new Error("Unknown error occurred"));
+ }
}Likely invalid or redundant comment.
111-112: 🛠️ Refactor suggestion
Add type safety to modelConfig parameter.
The method accepts any type for modelConfig which reduces type safety.
-formatRequestBody(messages: ChatOptions["messages"], modelConfig: any) {
+interface ModelConfig {
+ model: string;
+ max_tokens?: number;
+ temperature?: number;
+ top_p?: number;
+ top_k?: number;
+}
+formatRequestBody(messages: ChatOptions["messages"], modelConfig: ModelConfig) {Likely invalid or redundant comment.
411-418: 🛠️ Refactor suggestion
Improve logging security.
Logging request details could expose sensitive information in production.
- console.debug("[Bedrock Client] Request:", {
- path: chatPath,
- model: modelConfig.model,
- messages: messages.length,
- stream: shouldStream,
- });
+ if (process.env.NODE_ENV !== 'production') {
+ console.debug("[Bedrock Client] Request:", {
+ model: modelConfig.model.split('.')[0], // Log only model family
+ messageCount: messages.length,
+ stream: shouldStream,
+ });
+ }Likely invalid or redundant comment.
47-88:
Improve AWS credentials handling and validation.
The current implementation has several security concerns:
- AWS credentials are exposed in headers
- No validation before use
- No error handling for encryption failures
async function getBedrockHeaders(
modelId: string,
chatPath: string,
finalRequestBody: any,
shouldStream: boolean,
): Promise<Record<string, string>> {
const accessStore = useAccessStore.getState();
+ // Validate credentials
+ if (!accessStore.awsAccessKey || !accessStore.awsSecretKey || !accessStore.awsRegion) {
+ throw new Error("Missing required AWS credentials");
+ }
+
const bedrockHeaders = isApp
? await sign({
method: "POST",
url: chatPath,
region: accessStore.awsRegion,
accessKeyId: accessStore.awsAccessKey,
secretAccessKey: accessStore.awsSecretKey,
body: finalRequestBody,
service: "bedrock",
headers: {},
isStreaming: shouldStream,
})
: getHeaders();
if (!isApp) {
const { awsRegion, awsAccessKey, awsSecretKey, encryptionKey } = accessStore;
+ if (!encryptionKey) {
+ throw new Error("Missing encryption key");
+ }
const bedrockHeadersConfig = {
XModelID: modelId,
XEncryptionKey: encryptionKey,
ShouldStream: String(shouldStream),
- Authorization: await createAuthHeader(
- awsRegion,
- awsAccessKey,
- awsSecretKey,
- encryptionKey,
- ),
+ Authorization: await createAuthHeader({
+ region: awsRegion,
+ accessKey: awsAccessKey,
+ secretKey: awsSecretKey,
+ encryptionKey,
+ }).catch(error => {
+ console.error("[Bedrock] Failed to create auth header:", error);
+ throw new Error("Failed to create authorization header");
+ }),
};
Object.assign(bedrockHeaders, bedrockHeadersConfig);
}
return bedrockHeaders;
}Likely invalid or redundant comment.
| const eventRegex = /:event-type[^\{]+(\{[^\}]+\})/g; | ||
| let match; | ||
|
|
||
| while ((match = eventRegex.exec(text)) !== null) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix assignment in condition expression.
Using assignments in condition expressions can be confusing and may lead to unintended side effects.
- while ((match = eventRegex.exec(text)) !== null) {
+ let match;
+ while (match = eventRegex.exec(text)) {Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 387-387: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
| type ParsedEvent = Record<string, any>; | ||
| type EventResult = ParsedEvent[]; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any type with specific type definitions.
Using any type reduces type safety and can lead to runtime errors. Consider creating a specific type for the event data structure.
-type ParsedEvent = Record<string, any>;
+type ParsedEvent = {
+ output?: {
+ message?: {
+ content?: Array<{
+ text?: string;
+ toolUse?: {
+ name: string;
+ input: unknown;
+ };
+ }>;
+ };
+ };
+ choices?: Array<{
+ message?: {
+ content?: string;
+ tool_calls?: Array<{
+ id: string;
+ function?: {
+ name: string;
+ arguments: string;
+ };
+ }>;
+ };
+ }>;
+ delta?: {
+ text?: string;
+ type?: string;
+ partial_json?: string;
+ };
+};Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
app/constant.ts (1)
241-248:⚠️ Potential issueFix region handling and remove redundant constant
Several issues need to be addressed:
- The comment mentions getting the region from access store, but
getEndpoint()uses the default region.BEDROCK_BASE_URLis redundant as noted in previous reviews since we haveBedrock.getEndpoint().Apply this diff to fix the issues:
export const Bedrock = { ChatPath: "model", ApiVersion: "2023-11-01", getEndpoint: (region: string = "us-west-2") => `https://bedrock-runtime.${region}.amazonaws.com`, }; -// Get the region from access store for BEDROCK_BASE_URL -export const BEDROCK_BASE_URL = Bedrock.getEndpoint();
🧹 Nitpick comments (1)
app/constant.ts (1)
331-350: Enhance model documentation for maintainabilityConsider adding version release dates and capabilities as comments for better maintainability. This is particularly important for models that have version numbers in their names.
Apply this diff to improve documentation:
const bedrockModels = [ // Amazon nova Models + // Released: 2024-03, Optimized for: general text generation "us.amazon.nova-micro-v1:0", "us.amazon.nova-lite-v1:0", "us.amazon.nova-pro-v1:0", // Claude Models + // Released: 2024-03, Features: improved reasoning, reduced hallucination "anthropic.claude-3-haiku-20240307-v1:0", "anthropic.claude-3-5-haiku-20241022-v1:0", "anthropic.claude-3-sonnet-20240229-v1:0",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
app/constant.ts(7 hunks)app/utils.ts(2 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/utils.ts
- package.json
🔇 Additional comments (3)
app/constant.ts (3)
54-54: LGTM: API path follows established pattern
The Bedrock API path follows the consistent pattern used for other providers.
123-123: LGTM: Provider enums follow established pattern
The Bedrock provider is correctly added to both ServiceProvider and ModelProvider enums, maintaining consistency with other providers.
Also applies to: 148-148
580-590: LGTM: Model integration follows established pattern
The integration of Bedrock models into DEFAULT_MODELS follows the consistent pattern used for other providers, maintaining the proper structure and sorting.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/constant.ts (1)
605-615: Consider adding knowledge cutoff dates for Bedrock modelsThe KnowledgeCutOffDate record (around line 280) should be updated to include the cutoff dates for Bedrock models, especially for the Claude and Llama models which have known training cutoff dates.
Consider adding entries like:
export const KnowledgeCutOffDate: Record<string, string> = { default: "2021-09", + "anthropic.claude-3-opus-20240229-v1:0": "2024-01", + "anthropic.claude-3-sonnet-20240229-v1:0": "2024-01", + "us.meta.llama3-2-90b-instruct-v1:0": "2024-02", // ... existing entries };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
app/constant.ts(7 hunks)app/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/utils.ts
🔇 Additional comments (3)
app/constant.ts (3)
54-54: LGTM: API path follows consistent pattern
The Bedrock API path follows the established pattern and maintains alphabetical ordering.
123-123: LGTM: Provider enums are consistent
The Bedrock provider is correctly added to both ServiceProvider and ModelProvider enums, maintaining alphabetical ordering and consistency with other providers.
Also applies to: 148-148
347-366: LGTM: Well-organized model definitions
The Bedrock models are well-organized and grouped by provider, making it easy to maintain and update.
💻 变更类型 | Change Type
🔀 变更说明 | Description of Change
This PR introduces AWS Bedrock to support the leading large language models such as Claude, Llama, Mistral, and others.
API Enhancements:
app/api/bedrock.ts)app/client/api.ts)app/client/platforms/bedrock.ts)app/utils/aws.ts)Localization:
app/locales/cn.ts)app/locales/en.ts)Configuration:
app/constant.ts)Documentation:
📝 补充信息 | Additional Information
These changes leveraging Amazon Bedrock provides access to cutting-edge language models, offering several advantages:
Access to State-of-the-Art Models: Users can now utilize the latest versions of Llama 3.2, Mistral Large, and Claude 3.5, which are known for their advanced natural language understanding and generation capabilities.
Summary by CodeRabbit
Release Notes
New Features
PasswordInputcomponent to include an optional prop for masking sensitive values.FullScreencomponent for toggling fullscreen mode in the UI.Bug Fixes
Documentation
Chores